RESOLVED FIXED 235395
Web Inspector: Make alignment editor accessible
https://bugs.webkit.org/show_bug.cgi?id=235395
Summary Web Inspector: Make alignment editor accessible
Nikita Vasilyev
Reported 2022-01-19 23:21:47 PST
Currently, VoiceOver: - doesn't read anything when switching alignment by clicking on one of the icons - can't focus on any alignment editor icons
Attachments
Patch (3.62 KB, patch)
2022-01-20 00:20 PST, Nikita Vasilyev
no flags
[Image] With patch applied, VoiceOver on (85.05 KB, image/png)
2022-01-20 00:20 PST, Nikita Vasilyev
no flags
Patch (3.21 KB, patch)
2022-01-21 12:10 PST, Nikita Vasilyev
no flags
Nikita Vasilyev
Comment 1 2022-01-20 00:20:04 PST
Nikita Vasilyev
Comment 2 2022-01-20 00:20:42 PST
Created attachment 449558 [details] [Image] With patch applied, VoiceOver on
Radar WebKit Bug Importer
Comment 3 2022-01-20 15:04:57 PST
Patrick Angle
Comment 4 2022-01-21 11:49:12 PST
Comment on attachment 449557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449557&action=review > Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:40 > + // FIXME: <https://webkit.org/b/233055> Web Inspector: adapt :focus-visible internally Nit: This link is for a different bug, not the one you call out. 224026 is the correct bug, I think? Also it's not clear to me how we are blocked by /that/ bug here. It sounds like the proposed style from that bug was added to the UA in 221925, and :focus-visible appears to be on by default now. Shouldn't we just add the appropriate style instead of stopping propagating the mouse event? > Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:134 > - previousGlyphElement?.classList.remove("selected"); > + if (previousGlyphElement) { > + previousGlyphElement.classList.remove("selected"); > + previousGlyphElement.removeAttribute("aria-checked"); > + } Why not just stick with optional chaining here? > Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:143 > - glyphElement?.classList.add("selected"); > + if (glyphElement) { > + glyphElement.classList.add("selected"); > + glyphElement.setAttribute("aria-checked", true); > + } Ditto :131-134
Nikita Vasilyev
Comment 5 2022-01-21 11:53:04 PST
Comment on attachment 449557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449557&action=review >> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:40 >> + // FIXME: <https://webkit.org/b/233055> Web Inspector: adapt :focus-visible internally > > Nit: This link is for a different bug, not the one you call out. 224026 is the correct bug, I think? > > Also it's not clear to me how we are blocked by /that/ bug here. It sounds like the proposed style from that bug was added to the UA in 221925, and :focus-visible appears to be on by default now. Shouldn't we just add the appropriate style instead of stopping propagating the mouse event? Oh oops. Yes, we should. >> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:134 >> + } > > Why not just stick with optional chaining here? Sounds good.
Nikita Vasilyev
Comment 6 2022-01-21 12:08:01 PST
Comment on attachment 449557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449557&action=review >>> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:40 >>> + // FIXME: <https://webkit.org/b/233055> Web Inspector: adapt :focus-visible internally >> >> Nit: This link is for a different bug, not the one you call out. 224026 is the correct bug, I think? >> >> Also it's not clear to me how we are blocked by /that/ bug here. It sounds like the proposed style from that bug was added to the UA in 221925, and :focus-visible appears to be on by default now. Shouldn't we just add the appropriate style instead of stopping propagating the mouse event? > > Oh oops. Yes, we should. "... the proposed style from that bug was added to the UA in 221925, and :focus-visible appears to be on by default now." I just verified it, and it works. We don't even need to add any :focus-visible styling at all, the UA defaults are great now!
Nikita Vasilyev
Comment 7 2022-01-21 12:10:53 PST
Devin Rousso
Comment 8 2022-01-21 13:52:35 PST
Comment on attachment 449557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449557&action=review >>> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:134 >>> + } >> >> Why not just stick with optional chaining here? > > Sounds good. I don't have a strong opinion on this here, but I personally would've done the same to avoid a second falsy check. It also makes it easier to add additional things in the future without having to restructure the code then.
Patrick Angle
Comment 9 2022-01-21 14:38:04 PST
(In reply to Devin Rousso from comment #8) > Comment on attachment 449557 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449557&action=review > > >>> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:134 > >>> + } > >> > >> Why not just stick with optional chaining here? > > > > Sounds good. > > I don't have a strong opinion on this here, but I personally would've done > the same to avoid a second falsy check. It also makes it easier to add > additional things in the future without having to restructure the code then. I'm happy either way - you make a good point. I'll leave it up to Nikita.
Patrick Angle
Comment 10 2022-01-21 14:38:41 PST
Comment on attachment 449682 [details] Patch r=me
EWS
Comment 11 2022-01-21 15:41:32 PST
Committed r288385 (246282@main): <https://commits.webkit.org/246282@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449682 [details].
Note You need to log in before you can comment on or make changes to this bug.