Skip to content

LG-4682: Resolve confirmation step button scroll-to-top, role#5705

Merged
aduth merged 5 commits intomainfrom
aduth-lg-4682-confirmation-links
Dec 16, 2021
Merged

LG-4682: Resolve confirmation step button scroll-to-top, role#5705
aduth merged 5 commits intomainfrom
aduth-lg-4682-confirmation-links

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Dec 14, 2021

Why: Since "Print" and "Copy" are not navigable, they should not be rendered as links, since (a) this has the effect of scrolling the user to the top of the page when clicking the buttons and (b) they are announced to assistive technology as links. Since they behave as buttons, they should be rendered as such.

Implementation notes:

This refactors a fair bit of how these elements are rendered:

  1. Replaces BassCSS grid with USWDS
  2. Use margins instead of line breaks
  3. Implements and renders a new ClipboardButtonComponent

Notes on ClipboardButtonComponent:

  • This will be useful for upcoming implementation in LG-5456 for success indicators for clipboard buttons
  • While this introduces a (seemingly-redundant) new dependency for clipboard API, it would be my goal that this would eventually replace the existing clipboard dependency. The idea behind this is that we can ideally rely on native browser APIs, potentially removing the polyfill altogether if/when support for Internet Explorer is dropped.
  • The two new ViewComponent implementations may be a bit excessive, but (a) I did not feel it would be appropriate for a ClipboardButtonComponent to handle button details such as rendering the outline variant, (b) with a potential future goal of extracting design system components to a library gem, it would be expected that such an implementation would exist, and (c) it was a useful exercise in understanding how component inheritance may work.
  • While this would be a perfect use-case for a customized built-in element of the button element, the general consensus I've read is that customized built-in elements are effectively dead-on-arrival, due to Apple's refusal to support them in Safari.
    • A possible alternative is to avoid wrapping the <button> element and assign all necessary behaviors of a button to lg-clipboard-button (role="button", click, and keyboard handlers), but this would be rather cumbersome, and a wrapper is much more straightforward to implement.

**Why**: Since "Print" and "Copy" are not navigable, they should not be rendered as links, since (a) this has the effect of scrolling the user to the top of the page when clicking the buttons and (b) they are announced to assistive technology as links. Since they behave as buttons, they should be rendered as such.
It might've been nice to keep the template and somehow reference the superclass's "call" result, but it wasn't immediately clear how to achieve this.
**Why**: Generally, a custom element should be responsive to attribute changes (see "attributeChangedCallback"). It could be possible that someone would want to alter the clipboard text after the component has initialized.
@aduth aduth force-pushed the aduth-lg-4682-confirmation-links branch from d355cc0 to bd80d83 Compare December 15, 2021 16:52
end

def call
content_tag(:'lg-clipboard-button', super, data: { clipboard_text: clipboard_text })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possible future awkwardness we may run into is that because we're just wrapping the base markup, things like tag_options[:class] will apply to the child element, not the root. A small difference, but personally I'd expect tag_options to apply to the root element.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


it 'allows a user to copy the code into the confirmation modal' do
click_on t('links.copy')
copied_text = page.evaluate_async_script('navigator.clipboard.readText().then(arguments[0])')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's arguments here?

Copy link
Contributor Author

@aduth aduth Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's arguments here?

From what I understand, evaluate_async_script wraps the given script in a function, called with a callback function argument (last argument?).

So it ends up being something like...

(function(/* callback */) {
  navigator.clipboard.readText().then(arguments[0])
})(callback);

arguments being the JavaScript keyword to reference the given function arguments.

Copy link
Contributor

@zachmargolis zachmargolis Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat, I wish *they'd call it it something clearer like resolve lol 😂

*they = the webdriver/capybara devs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I'd hoped it would infer the promise value and treat it as something more like...

copied_text = page.evaluate_async_script('navigator.clipboard.readText()')
(() => navigator.clipboard.readText())().then(callback);

Comment on lines -24 to -26
<br>
<br>
<br>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aduth aduth merged commit 7bc86df into main Dec 16, 2021
@aduth aduth deleted the aduth-lg-4682-confirmation-links branch December 16, 2021 17:56
jmhooper pushed a commit that referenced this pull request Dec 28, 2021
* LG-4682: Resolve confirmation step button scroll-to-top, role

**Why**: Since "Print" and "Copy" are not navigable, they should not be rendered as links, since (a) this has the effect of scrolling the user to the top of the page when clicking the buttons and (b) they are announced to assistive technology as links. Since they behave as buttons, they should be rendered as such.

* Reference super.call for ClipboardButtonComponent inherited render

It might've been nice to keep the template and somehow reference the superclass's "call" result, but it wasn't immediately clear how to achieve this.

* Avoid persisting initial clipboard text to instance

**Why**: Generally, a custom element should be responsive to attribute changes (see "attributeChangedCallback"). It could be possible that someone would want to alter the clipboard text after the component has initialized.

* Remove unused clipboard script from personal key page

* Add spec to verify copied text
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants