Closed (fixed)
Project:
Drupal core
Version:
8.7.x-dev
Component:
layout_builder.module
Priority:
Major
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
18 Mar 2019 at 18:38 UTC
Updated:
4 Apr 2019 at 14:19 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
tim.plunkettBefore:
After:
Comment #3
cboyden commentedThe
aria-labelledbyattribute is supposed to contain the ID of the element that contains the label. If the label is visible on the screen somewhere, continue usingaria-labelledbybut fill it with the ID of the labeling element, as in<div aria-labelledby="section-1-label">or some such. If the text is not visible on the screen, use<div aria-label="Section 1">.Comment #4
cboyden commentedUpdated patch to use aria-label. If the label is visible on the screen, this approach isn't the best. In that case the code will need to know (or generate) an ID for the element that contains the label.
Comment #5
tim.plunkettIn this case, the label is not ever visible on the screen.
I misunderstood the usage of labelledby, but now it makes perfect sense.
Thanks @cboyden!
Comment #6
xjmShouldn't this have test coverage?
Comment #7
tim.plunkettSure
Comment #8
phenaproximaWasn't going to raise this, but discussed briefly with @tim.plunkett in Slack. Apparently we do want to test that the order of both arrays is the same, so this needs to be assertSame().
That's it, though. RTBC after that.
Comment #9
tim.plunkettSwitching to a strict comparison just in case
Comment #10
phenaproximaOkay. RTBC assuming the test passes.
Comment #12
xjmThis looks good to me. I think it would probably be good to confirm with @andrewmacpherson that this implements the most important item of #3019487: [plan] Describe layout builder UI to assistive technology but otherwise I think this is ready for commit after the commit freeze.
Comment #13
rainbreaw commentedJust noting here that I promised on the #ux meeting that I would take a look at this. I will be doing so my tomorrow AM (Wed AM PT).
Comment #14
rainbreaw commentedHi all, I did a test of this using screen reader + keyboard only (no eyes, mouse, or trackpad), and it worked pretty well.
I think there is probably further work that could be done to make this even better, but it is usable. The only frustration was already raised on the UX call and is a separate issue: when you close the side panel, your focus is sent back to the save layout button at the top instead of where you just where.
Another separate (somewhat subjective) accessibility issue that perhaps merits its own entry and prioritization for future work: the Save Layout and Discard Changes buttons are only at the top of the custom layout, which means that the user will have to backwards tab all the way to the beginning of the customizable section in order to save their layout. I would suggest replicating these buttons at the bottom of the customizable layout area, as well, or providing the keyboard user some other means of getting to these buttons quickly once they have reached the final section of their layout.
Comment #15
tim.plunkettThanks so much @rainbreaw!
Tagging for the followup, assigning credit
Comment #17
xjmAwesome, thanks @rainbreaw!
Committed to 8.8.x. Leaving RTBC for 8.7.x backport once the commit freeze is over (plus getting those two followups filed).
Comment #19
xjmBackported to 8.7.x too. Just open for the followups; it can be marked fixed once they are filed.
Comment #20
tim.plunkettThanks! Opened #3042107: Layout Builder actions are inconvenient to access via keyboard navigation for #14