Currently, VoiceOver: - doesn't read anything when switching alignment by clicking on one of the icons - can't focus on any alignment editor icons
Created attachment 449557 [details] Patch
Created attachment 449558 [details] [Image] With patch applied, VoiceOver on
<rdar://problem/87851989>
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
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.
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!
Created attachment 449682 [details] Patch
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.
(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.
Comment on attachment 449682 [details] Patch r=me
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].