Bug 235395 – Web Inspector: Make alignment editor accessible

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Currently, VoiceOver:
- doesn't read anything when switching alignment by clicking on one of the icons
- can't focus on any alignment editor icons

[Nikita Vasilyev]

Created attachment 449557 [details]
Patch

[Nikita Vasilyev]

Created attachment 449558 [details]
[Image] With patch applied, VoiceOver on

[Radar WebKit Bug Importer]

<rdar://problem/87851989>

[Patrick Angle]

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 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 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]

Created attachment 449682 [details]
Patch

[Devin Rousso]

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]

(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 on attachment 449682 [details]
Patch

r=me

[EWS]

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].