fix(button): adds pending button, fixes #3162 #3163
Conversation
Tachometer resultsChromeaction-bar permalink
action-button permalink
action-group permalink
action-menu permalink
alert-dialog permalink
button-group permalink
button permalink
dialog permalink
infield-button permalink
menu permalink
meter permalink
number-field permalink
overlay permalink
picker-button permalink
picker permalink
popover permalink
radio permalink
search permalink
slider permalink
split-button permalink
tags permalink
toast permalink
tooltip permalink
Firefoxaction-bar permalink
action-button permalink
action-group permalink
action-menu permalink
alert-dialog permalink
button-group permalink
button permalink
dialog permalink
infield-button permalink
menu permalink
meter permalink
number-field permalink
overlay permalink
picker-button permalink
picker permalink
popover permalink
radio permalink
search permalink
slider permalink
split-button permalink
tags permalink
toast permalink
tooltip permalink
|
packages/button/src/Button.ts
Outdated
| if (value) { | ||
| this.pendingCooldown = window.setTimeout(() => { | ||
| if (!this.disabled) { | ||
| this.disabledByPending = true; | ||
| } | ||
| this.disabled = true; | ||
| }, 1000); | ||
| } else { | ||
| window.clearTimeout(this.pendingCooldown); | ||
| if (this.disabledByPending) { | ||
| this.disabled = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Disabling the button does slightly different things to the accessibility tree delivered than I think that we want to here. Name you get "Button, dimmed". We likely want to surface a secondary label as an attribute that we can apply when when pending. We're looking at this in #3110 for Picker, so we should make sure the approaches are aligned.
There was a problem hiding this comment.
Switch to using is-pending state, and modified the spectrum-css conversion config to add the right is-pending attribute where it finds disabled. This is working with a bit of a hack, and I'm not sure of the 'right' fix here, though I think the hack has a low chance of failing?
packages/button/src/Button.ts
Outdated
| <sp-progress-circle | ||
| indeterminate | ||
| static="white" | ||
| size="s" | ||
| ></sp-progress-circle> |
There was a problem hiding this comment.
Should this always be available in the doc? If that's the approach we want, then we'll need something like aria-hidden=${ifDefined(!this.pending ? 'true' : undefined)} to keep it off of the accessibility tree.
Also, it doesn't look like this registration is ever loaded. Should it be static, dynamic, in the docs?
There was a problem hiding this comment.
yeah my feeling is it should be in the docs only (you don't want to lazy load this it might not show up for long as you're only supposed to use these pending states for < 5s loads). Maybe a dev mode warning if its used and pending-circle isn't registered?
There was a problem hiding this comment.
Modified this to only add the element when needed, this avoids the aria issue. Have still not made a decision on importing, I'm leaning towards just documentation of the need for sp-progress-circle being a required dependency when using pending state though? This would then avoid unnecessary payload. Maybe add a dev warning if the element is not registered?
packages/button/src/button.css
Outdated
|
|
||
| :host([pending][disabled]) slot[name='icon'], | ||
| :host([pending][disabled]) #label { | ||
| visibility: hidden; |
There was a problem hiding this comment.
Can't remember what visibly hiding this content does to the accessibility tree right off, so making a note to confirm.
There was a problem hiding this comment.
Checked in voiceover and the label seems to be hidden? Is there some other way I can test?
packages/button/src/Button.ts
Outdated
| <sp-progress-circle | ||
| indeterminate | ||
| static="white" | ||
| size="s" |
There was a problem hiding this comment.
We likely want to match the t-shirt size approach to that of #3110 as well.
There was a problem hiding this comment.
Yeah i had a play with this, was 'tricky', one step down in size was not always the rule, and the sizes didn't seem to align well.
There was a problem hiding this comment.
Did some more testing here and there's no good sizes here that work. s size progress-circle on s sized button is too big, s on m button is about right, but m on l button is too big.
Not sure what to do here, we could apply scale on the transform for the progress-circle to tweak it at each t-shirt size perhaps? But it'll likely be a custom value per button size.
packages/button/test/button.test.ts
Outdated
| await elementUpdated(el); | ||
|
|
||
| expect(el.disabled).to.be.true; | ||
| expect(el.pending).to.be.true; |
There was a problem hiding this comment.
Using an accessibility snap shot here could support us in guaranteeing a specific entry in the a11y tree.
There was a problem hiding this comment.
Updated to use the a11y snapshot testing for the aria-label changing to the pending-label value.
|
@benjamind are you able to look into getting this over the line? |
Only a month later.....(my SWC notifications are a mess)...yes I can try and find some time now to push this over. |
benjamind
force-pushed
the
delarre/pending-button
branch
from
a29b77d to
5009c7a
Compare
benjamind
force-pushed
the
delarre/pending-button
branch
from
5009c7a to
305c7e6
Compare
packages/button/src/Button.ts
Outdated
| this.addEventListener( | ||
| 'click', | ||
| (event: Event): void | boolean => { | ||
| if (this.pending) { | ||
| event.preventDefault(); | ||
| event.stopImmediatePropagation(); | ||
| event.stopPropagation(); | ||
| return false; | ||
| } | ||
| }, | ||
| { | ||
| capture: true, | ||
| } | ||
| ); |
There was a problem hiding this comment.
Thoughts on this versus using pointer-events: none on the attribute selector?
If we use the event listener, I think we should only add it when pending rather than always have the listener attached.
There was a problem hiding this comment.
pointer-events:none should be better in this case so as to avoid any event propagation at all during pending state.
Note: we are overriding the click function to avoid clicking on pending either way so even with pointer-events:none we won't run into cases where users tries to click the button programatically while in pending state!
packages/button/src/Button.ts
Outdated
| @property({ type: Boolean, reflect: true, attribute: 'is-pending' }) | ||
| public isPending = false; | ||
|
|
||
| @property({ type: String, attribute: 'pending-label' }) | ||
| public pendingLabel = 'Pending'; | ||
|
|
||
| @property({ type: Boolean, reflect: true, attribute: true }) | ||
| public pending = false; |
There was a problem hiding this comment.
I don't really understand the use of pending and isPending. Can you add some comments to clarify the workflow that's being acheived?
There was a problem hiding this comment.
Yeah pending is the indication that this is a pending state button, while is-pending is added/removed during the pending state. Agree needs comments.
There was a problem hiding this comment.
That timing seems weird, though it does make the code make so much more sense.
Based on this documentation, it feels like pending should be an immediate state. Was there something specific to your application requirements that lead to this relationship?
There was a problem hiding this comment.
Well it was more a matter of convenience at point of use. If the pending state is added as an indicator of future behavior, then the click handler behaves like a normal button, with the click event firing on first click. The button then adds its own isPending state to track its timeout period and disable further clicks during that time.
If the consumer has to add the pending state when click occurs, thats a little more work for the consumer to deal with? Not sure, I could go both ways on this, but it felt convenient to have it this way, and it marks the button in the consumers render template as a pending state button so its quite clear to see that the behavior will be distinct here, rather than a side effect of the buttons click handler?
There was a problem hiding this comment.
This is an interesting approach, but not seemingly what the code says?
if (changedProperties.has('pending')) {
if (this.pending) {
this.pendingCooldown = window.setTimeout(() => {
if (!this.disabled) {
this.isPending = true;
this.setAttribute('aria-label', this.pendingLabel);
}
}, 1000);
} else if (this.pending === false && this.isPending) {
window.clearTimeout(this.pendingCooldown);
this.isPending = false;
this.setAttribute('aria-label', this.label || '');
}
}
This may be updated functionality, but this says that as soon as you set pending you start the clock for isPending.
I discussed why this might be in with the design team, and they outlined a seemingly valid use case of no wanting to go to "visually pending" and then "processed" in flicker speed progression. It's valid, but it's valid at any point in the process where you might switch to pending, which makes me wonder if it. would be better to require an application manage any staged loading they do local to where there can be some understanding of the time required to do the actual task in question.
I can be convinced of either way, with clear comments that outline a workflow that we all see the same way, but right now I'm leaning towards it being better to only offer pending and it being immediate.
There was a problem hiding this comment.
Ah yes sorry forgot that nuance (its been a while since i started this one)....
I think if we leave management of the visually pending state to the consumer, like as not it will get implemented poorly in many places and result in a multitude of flickering bugs in a variety of places with solutions to it varying across the board.
But yes the state flow as I recall now is:
- Enabled button
- Click
- Click handler - consumer assigns
pendingstate isPendingtimeout beginsisPending&&pendingcauses display of 'visually pending' state- Time goes by
- Consumer removes
pendingstate isPendingis removed- Enabled button.
There was a problem hiding this comment.
Yeah, this is ugly pretty much regardless... @TarunAdobe can you test this approach which would simplify to a single state variable.
- Enable button
- Click
- Consumer assignes
pending - The 'visually pending' state is applied via an immediate transition or animation that has a delay of
1000msbut is customizable via a CSS Custom Property - Time goes by
- Consumer removes
pending - The 'visually pending' state is removed via an immediate transition or animation that has no delay
- Button is enable
packages/button/src/Button.ts
Outdated
| <sp-progress-circle | ||
| indeterminate | ||
| static="white" | ||
| size="s" |
There was a problem hiding this comment.
Is size="s" the right size for all Button sizes?
There was a problem hiding this comment.
In internal discussion with the Spectrum-CSS team, specifically @KayiuCarlson we concluded that there are no good t-shirt sizes right now to use for the progress circle inside of a button in these cases. So we concluded that we should fudge the sizes manually here to match the workflow-icon sizes.
There was a problem hiding this comment.
now using tshirt sizing for progress-circle.
packages/button/src/button.css
Outdated
| sp-progress-circle { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
Is there any time when not pending that this would be rendered to the DOM so as to need this rule?
There was a problem hiding this comment.
checked and found this is redundant!
There was a problem hiding this comment.
With the removal of isPending variable in the button. this is now back and useful again as we want to show the progress-circle only when the is-pending attribute is set after the animation
packages/button/src/button.css
Outdated
| ::slotted([slot='icon']) { | ||
| inset: unset; | ||
| } |
There was a problem hiding this comment.
same for this one... It doesn't seem to do anything in context of what we are trying to achieve.
packages/button/src/button.css
Outdated
| #container { | ||
| position: relative; | ||
| display: flex; | ||
| } |
There was a problem hiding this comment.
This element no longer seems to exist?
There was a problem hiding this comment.
I don't think so... maybe it did during the initial phase of this PR however now it's useless. so im removing it.
| left: 50%; | ||
| transform: translate(-50%, 0); |
There was a problem hiding this comment.
For reference: left here is technically correct, in that translate() doesn't really support logical properties, but CSS attempts not to use it when possible.
|
@Westbrook I have added the tshirt sizing for the progress-circle... Does this PR need more work? |
packages/button/src/Button.ts
Outdated
| <sp-progress-circle | ||
| indeterminate | ||
| static="white" | ||
| label=${this.pendingLabel + 'progress circle'} |
There was a problem hiding this comment.
Do we need the progress circle part? That forces un-translated content into the label.
Separately, is it even possible to get to the Progress Circle in the accessibility tree? Would it be better to use aria-hidden="true", as this is namely a visible affordance for the change in label on the Button?
There was a problem hiding this comment.
you're right! this is just a visible affordance so it makes much more sense to have aria-hidden="true"
| @keyframes show-progress-circle { | ||
| 0% { | ||
| visibility: hidden; | ||
| } | ||
|
|
||
| 100% { | ||
| visibility: visible; | ||
| } | ||
| } | ||
|
|
||
| @keyframes hide-icons-label { | ||
| 0% { | ||
| visibility: visible; | ||
| } | ||
|
|
||
| 100% { | ||
| visibility: hidden; | ||
| } | ||
| } |
There was a problem hiding this comment.
Really wanted that to work nicely 😞 Seems like it's possible, but the actual complexities of it out weight the benefits. Thanks for checking!
packages/button/src/Button.ts
Outdated
| changedProperties.get('pending') !== undefined && | ||
| !this.pending |
There was a problem hiding this comment.
What do you see us addressing by the more complex condition here?
There was a problem hiding this comment.
If a consumer only provides the aria-label property for a button and doesn't set the label then this condition would make sure we won't override the aria-label value to this.label which is undefined in this case.
Note: sp-split-button element currently does the same thing... we provide the aria-label to the buttons but not the label.
There was a problem hiding this comment.
Help me understand. How I currently understand the code as written:
if ("pending has changed") {
if ("is pending") {
if ("is not disabled") {
// Apply pending label without regard for whether it is present or not
}
} else if ("is NOT pending AND NOT previously `undefined`, which happens at construction time when creating the `pending` property to begin with") {
// Apply `label` or ''
}
}
I like the idea of covering for when the author has applied aria-label, but I'm not sure that this code does. I think doing so would look more like:
if ("pending has changed") {
if ("is pending") {
if ("is not disabled") {
// Assume the applied `aria-label`, no matter who set it, is correct (this _might_ hit some issues if a Button was created as `pending` to start with, depending on code order)
this.cachedLabel = this.getAttribute('aria-label');
this.setAttribute('aria-label', this.pendingLabel);
} else if (this.cachedLabel) {
// If there was previously an applied `aria-label` re-apply it, or do nothing
this.setAttribute('aria-label', this.cachedLabel);
}
}
}
This logic then duplicates some complexity with the change logic for disabled, which might benefit from being merged in, but is more annoying.
Either way, this got complex fast, it will be important to confirm the value of the arial-label attribute across these various states in the unit tests.
- pending===false => pending===true
- disabled===true && pending===false => disabled===true && pending===false
- disabled===false && pending===true => disabled===true && pending===true
- disabled===true && pending===true => disabled===false && pending===true
- All the same again with
aria-labelapplied from the outside - All the same again with
labelapplied from the outside
There may be others, so think through the logic your creating and make sure the various workflows are fully managed so that the output you're expecting at the various points is specifically under test in the tests you add.
There was a problem hiding this comment.
I've updated the logic and added some new tests. I think they cover all of the possibilities (?) However the logic is now a bit complicated so if you need help in understanding anything or if I missed some edge-cases do let me know.
There was a problem hiding this comment.
@Westbrook I've updated the code based on our discussion. Have a look whenever you get time
packages/button/src/Button.ts
Outdated
| if (this.pending && !this.disabled) { | ||
| this.setAttribute('aria-label', this.pendingLabel); | ||
| } |
There was a problem hiding this comment.
if (this.pending && this.disabled) should we remove the aria-label in this case?
Westbrook
changed the title
packages/button/src/Button.ts
Outdated
| @@ -127,7 +146,6 @@ export class Button extends SizedMixin(ButtonBase, { noDefaultSize: true }) { | |||
| public set quiet(quiet: boolean) { | |||
| this.treatment = quiet ? 'outline' : 'fill'; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Can we add back this line.
|
Amazing work @TarunAdobe thanks for making this a real thing! |
Description
Adds the 'pending' button state to the
<sp-button>element.The
pendingattribute drives the state, and internally starts the 1s cooldown timer to set the button state todisabled. The spinner will then render when the button is bothdisabledandpending.Main question outstanding I think is how to manage the import here, I don't think we want all buttons automatically requiring
sp-progress-circleits unnecessary page weight. My suggestion is we leave including the spinner up to the end user and maybe in dev we add a warning if its not been registered when using pending state?Related issue(s)
Motivation and context
Pending buttons are part of the Spectrum design spec, though they should be used sparingly they are occasionally useful.
How has this been tested?
Added a unit test to confirm the button is being driven into the disabled state at the right time. I assume VRT would confirm the rest (though I have not yet updated VRT).
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.