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

Rework progress bar markup and styles #36831

Merged
merged 21 commits into from Nov 29, 2022
Merged

Rework progress bar markup and styles #36831

merged 21 commits into from Nov 29, 2022

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Jul 24, 2022

Logically moves the various role and aria- attributes to the .progress element itself, leaving the .progress-bar to be used purely for the visual presentation. This fixes the problem documented in #36736 that in certain browser/AT combinations (NVDA, VoiceOver), zero-value/zero-width progress bars are completely ignored and not announced.

For multiple/stacked progress bars, this PR introduces a new wrapper and class .progress-stacked, to accommodate for the fact that with the more logical structure above, we need full .progress elements with child .progress-bar elements, and can't get away with the fudge we had before of having a single .progress with multiple .progress-bars.

Note that the old markup structures still work with this change, so this could be considered a non-breaking change - though one we definitely want to highlight as it's more accessible (as it now guarantees that zero-value/zero-width progress bars, whether on their own or as part of a multi/stacked bar, are actually announced)

Closes #36736

Logically moves the various `role` and `aria-` attributes to the `.progress` element itself, leaving the `.progress-bar` to be used purely for the visual presentation. This fixes the problem #36736 that in certain browser/AT combinations, zero-value/zero-width progress bars are completely ignored and not announced.

For multiple/stacked progress bars, this PR introduces a new wrapper and class `.progress-stacked`, to accommodate for the fact that with the more logical structure above, we need full `.progress` elements with child `.progress-bar` elements, and can't get away with the fudge we had before of having a single `.progress` with multiple `.progress-bar`s.

Note that the old markup structures still work with this change, so this could be considered a non-breaking change - though one we definitely want to highlight as it's more accessible (as it now guarantees that zero-value/zero-width progress bars, whether on their own or as part of a multi/stacked bar, are actually announced)
@patrickhlauke
Copy link
Member Author

Video using NVDA/Chrome - note that, compared to the video in #36736, the zero value progress bar is now correctly announced.

bootstrap-pull36831.mp4

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Jul 30, 2022

Also added a tentative note in the migration guide for this change https://deploy-preview-36831--twbs-bootstrap.netlify.app/docs/5.2/migration/

@patrickhlauke patrickhlauke force-pushed the patrickhlauke-issue36736 branch 4 times, most recently from 6931884 to 0fa4c79 Compare July 30, 2022 17:04
@patrickhlauke
Copy link
Member Author

@mdo as suggested in Slack, I added two callouts showing the old structure and explaining why we changed them https://deploy-preview-36831--twbs-bootstrap.netlify.app/docs/5.2/components/progress/

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Really like the non-breaking approach! Just two comments here on my side.

site/content/docs/5.2/components/progress.md Outdated Show resolved Hide resolved
scss/_progress.scss Outdated Show resolved Hide resolved
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

LGTM! @mdo I let you double check the migration guide modification.

@mdo
Copy link
Member

mdo commented Nov 14, 2022

Reworked the docs changes to keep the differences to the migration guide (which we'll also include in our blog post). Let me know how this reads @patrickhlauke @julien-deramond.


The problem with this structure is that progress bars with a zero value are completely ignored, and not announced, by assistive technologies. While the legacy structure will still be displayed correctly, we strongly recommend updating to the new structure.
{{< /callout >}}
### Width
Copy link
Member

Choose a reason for hiding this comment

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

I understand why this is modified like that but I've found it weird when I read it to have a "Sizing > Height" section that explains how to change the height of the entire progress bar and a "Sizing > Width" section that doesn't explain how to reduce the width of the progress bar but rather its content; its value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can see what you mean, but in a sense both are about sizing the width and height of the bars themselves...just that the height is set for the whole of the progress element. Maybe changing the first heading to "Bar sizing"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Made a tweak to the sizing section @julien-deramond @mdo to make it clearer

Copy link
Member

Choose a reason for hiding this comment

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

OK for me 👌

@mdo mdo merged commit 26a3ef1 into main Nov 29, 2022
@mdo mdo deleted the patrickhlauke-issue36736 branch November 29, 2022 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Zero-width/zero-value progress bars not announced consistently by browser/assistive technology combinations
3 participants