-
Notifications
You must be signed in to change notification settings - Fork 6
Inaccessible content warning: Phase 1 #1765
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
Conversation
Migrates the old `DISPLAY_SETTINGS` features `REDUCED_MOTION` and `PREFER_MATHML` to a new `ACCESSIBILITY` group to achieve this.
…e-content-warning [VRT] Update baselines for feature/vi-inaccessible-content-warning
…e-content-warning [VRT] Update baselines for feature/vi-inaccessible-content-warning
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1765 +/- ##
=======================================
Coverage ? 41.59%
=======================================
Files ? 541
Lines ? 23675
Branches ? 6981
=======================================
Hits ? 9848
Misses ? 13786
Partials ? 41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…e-content-warning [VRT] Update baselines for feature/vi-inaccessible-content-warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several small things, some of them just questions from me, but mostly this looks good :)
I'm a fan of using the @container structure to style the question-metadata according to its own width rather than breakpoints, to adjust to e.g. both the presence or not of a sidebar. It's neat!
src/app/components/elements/list-groups/AbstractListViewItem.tsx
Outdated
Show resolved
Hide resolved
src/app/components/elements/list-groups/ContentSummaryListGroupItem.tsx
Outdated
Show resolved
Hide resolved
src/app/components/elements/panels/UserAccessibilitySettings.tsx
Outdated
Show resolved
Hide resolved
| border-left: 1px solid $color-neutral-200; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When question-metadata gets narrow enough that "Stage and Difficulty" is pushed onto its own line, it still keeps this border-left. Unfortunately, this doesn't happen at a specific screenwidth and instead depends on the widths of the status and stage/difficulty.
It looks a little odd, so maybe we should add a breakpoint to enforce this stacked view at a certain full-page width instead (and remove all borders at this size)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to choose a width (690px) that would always fit all 4, but clearly it needs to be bigger. Do you remember which page this happened on? I think just increasing that (and the 689.98px below) should fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is not so much with a specific page, but when the status is All attempted (some errors), since that's a much wider status than all the alternatives. e.g.

(on Phosphorus: A Nutrient Cycle in Crisis)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking even further into this, we also have similar issues on live already with All attempted (some errors). Tricky!
I know this status wording was decided with content, but the (some errors) part doesn't seem necessary and removing it would certainly be an easy fix. Otherwise I suppose we could set a maximum width of 150px or so to the status section in order to have the text wrap instead? Or that but only at certain container widths, since it would look a little strange when there's space to spare?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha, okay. I gave the status section 200px maximum width, but when displaying All attempted (some errors) it uses just under 227px. This was messing with the width of the subjects div, which broke everything. I've bumped the max status width to 227px and it seems better now.
…e-content-warning [VRT] Update baselines for feature/vi-inaccessible-content-warning
…e-content-warning [VRT] Update baselines for feature/vi-inaccessible-content-warning
sjd210
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this now all looks good aside from the All attempted (some errors) incongruity. I've tested various questions at different configurations and it does seem like the only time there are errors are when that status exists.
A pre-existing problem to some extent, but these changes really do highlight it.
sjd210
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most glaring issue with strange alignment has been solved, although there can still be some unwanted wrapping when dealing with particularly long topic names (looking at you Chemistry | Analytical | Electronic Spectroscopy :/).
I did some searching to find if there was any way to deal with this using container min-height to detect wrapping, but as soon as a container is determined by size rather than inline-size the automatic height breaks down. I think this is something that we ought to still look to fix at some point (using JS and probably ResizeObservers rather than pure CSS), but at this point the problem was pre-existing so I don't see any harm in merging this in now and leaving that for later. It's niche enough to not be important.
…e-content-warning [VRT] Update baselines for feature/vi-inaccessible-content-warning

Adds support for two accessibility warning tags,
access:visualandaccess:motor, to appear around the site.When the new relevant account setting is set, icons with explanations should appear on all relevant ALVIs (QF, site search, builder). Question/concept pages will also show an alert that should be read out by a screenreader with high priority, and metadata on questions should also include this information.
This feature exists under a new accessibility panel under My Account; a new property for
SHOW_INACCESSIBLE_WARNINGnow exists. If the user has not specified a preference for this, it should default to true for teachers and false for students.This does not yet add support for filtering in the question finder by these tags, as this requires additional API work. I have started on the FE side of this, however.