WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
[Image] With patch applied, VoiceOver on
(85.05 KB, image/png)
2022-01-20 00:20 PST
,
Nikita Vasilyev
no flags
Details
Patch
(3.21 KB, patch)
2022-01-21 12:10 PST
,
Nikita Vasilyev
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Nikita Vasilyev
Comment 1
2022-01-20 00:20:04 PST
Created
attachment 449557
[details]
Patch
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
<
rdar://problem/87851989
>
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
Created
attachment 449682
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug