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 bug where mwc-snackbar labelText could not be set #412

Merged
merged 6 commits into from Aug 26, 2019
Merged

Conversation

aomarks
Copy link
Collaborator

@aomarks aomarks commented Aug 22, 2019

This adds a lit directive implementation of @material/mdc-snackbar/util.ts#announce, which does some tricks to ensure that snackbar labels will be handled correctly by screen readers.

The existing MDC announce util function is difficult to use directly here, because Lit can crash when DOM that it is managing changes outside of its purvue. In this case, we were rendering our labelText as the text content of the label div, but the MDC announce function was then clearing that text content, and resetting it after a timeout. We do the same thing here, but now in a way that fits into lit's lifecycle.

I also made the isOpen property update more eagerly. Previously, isOpen was only true if the MDC opening animation had finished, but I think it makes more sense for it to just be a bit that toggles independently of animations. There is an MDC event the user should watch if they actually want to know when the animation finishes.

Fixes #367

cc @justinfagnani This is my first lit directive. It's kind of a weird one. WDYT?

This adds a lit directive implementation of
@material/mdc-snackbar/util.ts#announce, which does some tricks to
ensure that snackbar labels will be handled correctly by screen readers.

The existing MDC announce util function is difficult to use directly
here, because Lit can crash when DOM that it is managing changes outside
of its purvue. In this case, we would render our labelText as the text
content of the label div, but the MDC announce function then clears that
text content, and resets it after a timeout. We do the same thing here,
but in a way that fits into Lit's lifecycle.

Fixes #367
/**
* Maps an accessibleLabel container part to its label Element.
*/
const labelElements = new WeakMap<NodePart, Element>();

Choose a reason for hiding this comment

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

Consider consolidating the maps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

if (maybeLabelEl === undefined) {
// Stamp the label element once, the first time we open.
const labelPart = new NodePart(containerPart.options);
labelPart.setValue(html`<div class="mdc-snackbar__label"

Choose a reason for hiding this comment

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

the hard-coded template makes this pretty non-reusable. Is there a way to abstract the functionality further so other elements could apply it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed offline. Maybe -- but my problem right now is that I don't have a test harness set up that lets me verify that any change we make that changes anything about the DOM layout and the specific technique here will still work, so I'm very inclined to sticky precisely to the approach used in the existing MDC util.

I did add a TODO to explain that we should investigate simplifying this.

@aomarks
Copy link
Collaborator Author

aomarks commented Aug 26, 2019

PTAL. I also simplified the initialization to directly create an element instead of creating a NodePart.

Copy link
Collaborator

@dfreedm dfreedm left a comment

Choose a reason for hiding this comment

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

LGTM

@aomarks aomarks merged commit aa06041 into master Aug 26, 2019
@aomarks aomarks deleted the snack-text branch August 26, 2019 22:49
@aomarks
Copy link
Collaborator Author

aomarks commented Aug 26, 2019

Filed #423 to track simplifying this implementation (which is blocked on accessibility testing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snackbar: Cannot set labelText property <mwc-snackbar>
5 participants