Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure components are disabled consistently #4109

Merged
merged 13 commits into from
Mar 9, 2022

Conversation

jcfranco
Copy link
Member

Related Issue: #2655

Summary

This establishes a pattern to handle the disabling of interactive components.

Implementation of interactive components should follow these steps:

  1. Update the component to implement the InteractiveComponent interface.
  2. Use updateHostInteraction (src/utils/interactive.ts) on the component's componentDidRender lifecycle method – this will update tabindex and aria-disabled attributes, accordingly.
  3. Include the disabled mixin from src/assets/styles/includes.scss – this provides the base styles for disabled components.
  4. If the interactive component has a popover/popper element, make sure to close it when the component is disabled and prevent it from being opened programmatically. Note that no utils are available for this, so this will need to be implemented via watchers or so.
  5. Optionally, set disabled on any supporting, internal components (e.g., <calcite-slider> disables internal <button>s).
  6. The component must be tested with the disabled test helper (src/tests/commonTests).

Notes

  • disabled components set tabIndex on the host and assume it will not be modified by end-users - it is set on the host to prevent tabbing into children:

    From WICG/inert#48:

    From step 3 of https://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation

    [...] an element whose tabindex value is negative must not be appended to NAVIGATION-ORDER.

    Which in other words means that if an element has tabindex=-1 and a shadowRoot, the focus won't be able to reach the element's contents (distributed or not) 🎉

  • disabling a component with an active popover will close it and prevent programmatically opening it

    • reason 1: disabled popovers may end up covering surrounding interactive elements
    • reason 2: setting opacity on the popovers root will create a stacking context that the popover will not be able to escape in a straightforward manner
  • calcite-label's disabled property is deprecated

  • tweaked styles and rendering of calcite-input-date-picker to avoid pointer-events overrides.

  • programmatically disabling a component that owns focus will not affect the focus order. For example, if the color picker's hex input is focused and some extraneous code disables the color picker, a user would still be able to tab within the the color picker's focusable controls. I think this won't be feasible in a performant way until inert is implemented across browsers. FWIW, I don't think this will be an issue for most use cases, but we'll have to see. As a workaround, endusers could focus another element when disabling a focus-owning component.

  • Some components emit one or more click events from implementation, I'll create a separate issue for this.

  • Disabled components get their HTMLElement#click() hijacked to prevent programmatic clicking.

The following components were updated to follow the same disabled behavior:

  • calcite-action.tsx
  • calcite-block.tsx
  • calcite-button.tsx
  • calcite-checkbox.tsx
  • calcite-color-picker.tsx
  • calcite-combobox-item.tsx
  • calcite-combobox.tsx
  • calcite-date-picker-day.tsx
  • calcite-dropdown.tsx
  • calcite-fab.tsx
  • calcite-filter.tsx
  • calcite-inline-editable.tsx
  • calcite-input-date-picker.tsx
  • calcite-input-time-picker.tsx
  • calcite-input.tsx
  • calcite-link.tsx
  • calcite-list-item.tsx
  • calcite-list.tsx
  • calcite-panel.tsx
  • calcite-pick-list.tsx
  • calcite-radio-button.tsx
  • calcite-radio-group.tsx
  • calcite-rating.tsx
  • calcite-select.tsx
  • calcite-slider.tsx
  • calcite-sortable-list.tsx
  • calcite-split-button.tsx
  • calcite-stepper-item.tsx
  • calcite-switch.tsx
  • calcite-tab-title.tsx
  • calcite-tile-select.tsx
  • calcite-tile-select-group.tsx
  • calcite-tile.tsx
  • calcite-value-list.tsx

Pending

  • ✅Wrap up and wire up test helper
  • ✅ Remove remaining obsolete styles (including stacked opacities)
  • ✅ Address and remove remaining TODOS

Questions

  • Does calcite-label need disabled? Is it considered interactive? It only forwards the click on labelable components. – will deprecate as disabled label behavior depends on labelable component
  • Do we need separate utils for interactive components w/ popovers? – possibly, but will wait until floating-ui refactor
  • Do we want calcite-stepper to keep overriding pointer events on items to display the disabled (not-allowed) cursor?
    disabled-stepthe pointer-events override & custom cursor will be removed for consistency

@jcfranco jcfranco requested a review from a team as a code owner February 14, 2022 19:18
@github-actions github-actions bot added this to the Sprint 02/14 - 02/25 milestone Feb 14, 2022
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Feb 14, 2022
@jcfranco
Copy link
Member Author

Figured out why Screener checks weren't working with #4010 and #4108. A new E2E util I added, introduced an import from @stencil/core (newE2EPage) that ended up breaking the storybook build. I moved the storybook utils to .storybook/utils.tsx and moved the html formatter helper to support/formatting.ts.

@jcfranco
Copy link
Member Author

This PR is now stable, so pardon for the modified copypasta from #4010 😅:

@benelan @y0n4 Can you help me test this out? Some things to look out for:

  • focusing via keyboard/mouse is prevented
  • consistent styling across components and their children
  • opacity should not stack (otherwise it looks too faded)
  • additional components to adopt this pattern (handled in follow-up issue)

@macandcheese @asangma Can you take a look at the Screener diffs? 👀

@benelan
Copy link
Member

benelan commented Feb 15, 2022

yup I'll do that tomorrow!

@y0n4
Copy link
Contributor

y0n4 commented Feb 15, 2022

Tested the following components:

  • panel
  • pick-list
  • radio-button
    The radio-button text is not faded when disabled (not sure if that is intended)
Screen Shot 2022-02-15 at 1 48 45 PM Screen Shot 2022-02-15 at 1 49 54 PM
  • radio-group
  • rating

The mouse/keyboard navigation was prevented as expected and the opacity did not look stacked for all

@jcfranco
Copy link
Member Author

The radio-button text is not faded when disabled (not sure if that is intended)

I believe this is working as expected. The disabled element is the radio and not the label in this case.

@y0n4
Copy link
Contributor

y0n4 commented Feb 21, 2022

Tested the following components:

  • select
  • slider
  • sortable-list
  • split button
  • stepper-item
  • switch
  • tab-title
  • tile-select
  • tile-select-group
  • tile
  • value-list
    when it's disabled- it is not visually muted and can still keyboard navigate, focus, and click on the disabled item. Just cannot select the item
    Default (top pic) and disabled (bottom pic)
Screen Shot 2022-02-18 at 4 53 12 PM Screen Shot 2022-02-18 at 4 49 11 PM

@jcfranco
Copy link
Member Author

@y0n4 Thanks! I'll take a look at fixing the value-list. Pick-list may also have the same issue since they share code.

@jcfranco
Copy link
Member Author

@y0n4 Could you share more info on the value-list case you reported? The following works fine for me:

<calcite-value-list disabled>
  <calcite-value-list-item label="Uno" removable value="1"></calcite-value-list-item>
  <calcite-value-list-item label="Dos" value="2" removable selected></calcite-value-list-item>
  <calcite-value-list-item label="Tres" value="3" removable></calcite-value-list-item>
</calcite-value-list>

@y0n4
Copy link
Contributor

y0n4 commented Feb 25, 2022

Sorry yes I failed to mention that it was the calcite-value-list-item on disabled mode that does not work. The readme says I should expect:

When true, the item cannot be clicked and is visually muted

<calcite-value-list>
  <calcite-value-list-item disabled label="Uno" removable value="1"></calcite-value-list-item>
  <calcite-value-list-item label="Dos" value="2" removable selected></calcite-value-list-item>
  <calcite-value-list-item label="Tres" value="3" removable></calcite-value-list-item>
</calcite-value-list>

@jcfranco
Copy link
Member Author

jcfranco commented Mar 3, 2022

@y0n4 The value-list issue you reported should now be fixed. Can you test again?

@y0n4
Copy link
Contributor

y0n4 commented Mar 4, 2022

@jcfranco just tested and value-list-item works great! Thank you

@jcfranco
Copy link
Member Author

jcfranco commented Mar 8, 2022

Last round of fixes installed. Checks are ✅!

@macandcheese Accepted the following Screener changes per our discussion a while back. cc @asangma

Here's a summary of the changes.

action

Screen Shot 2022-03-08 at 9 09 04 AM

tile-select

Screen Shot 2022-03-08 at 9 09 47 AM
Screen Shot 2022-03-08 at 9 09 53 AM
Screen Shot 2022-03-08 at 9 09 58 AM
Screen Shot 2022-03-08 at 9 09 40 AM

Let me know if we need to revisit any of these later on.

@jcfranco jcfranco requested a review from asangma March 8, 2022 18:00
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

💯

@jcfranco jcfranco merged commit 083d283 into master Mar 9, 2022
@jcfranco jcfranco deleted the jcfranco/2655-ensure-consistent-disabled-behavior branch March 9, 2022 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants