Bug 208163 – Web Inspector: AXI: buttons should be focusable when navigating by pressing Tab

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

It should be possible to Tab/Shift-Tab onto a button. In this context, any clickable icon is a button. The Inspect element (the bullseye icon) and Force Dark Appearance are buttons. Currently, it isn't possible to reach those by pressing Tab.

[Radar WebKit Bug Importer]

<rdar://problem/59745448>

[Nikita Vasilyev]

Created attachment 391596 [details]
Patch

[Nikita Vasilyev]

Created attachment 391598 [details]
[Video] With patch applied

[Blaze Burg]

Comment on attachment 391596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391596&action=review

r=me

> Source/WebInspectorUI/ChangeLog:13
> +        you were â the console prompt. This behavior matches MacOS.

nit: macOS

> Source/WebInspectorUI/ChangeLog:17
> +        triggers "click" event.

Interesting!

> Source/WebInspectorUI/ChangeLog:25
> +        Reset the default styles.

Why?

> Source/WebInspectorUI/ChangeLog:30
> +        Before this patch, focused button looked the same as activated buttons. For example, the focused (non-active) bullseye icon looked exactly the same as unfocused active bullseye icon, which was misleading.

Please line-break.

> Source/WebInspectorUI/UserInterface/Views/ActivateButtonNavigationItem.js:74
> +        this.tooltip = flag ? this._activatedToolTip : this._defaultToolTip;

In the interest of preventing programming errors, can we coerce `flag` to a bool at the top of the function? i.e., !!flag. This wasn't as important before but now the code sets ariaPressed to the value of flag directly.

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:159
> +        // Clicking on a button should NOT focus on it.

I think you should leave it at this one line comment. The full justification in the changelog would be easily accessible.

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:297
> +                // Generally, clicking on a button should not move focus.

"If clicking on a tab, stop the event from being handled by the button element. Instead, pass focus to the selected tab. Otherwise, let the button become activated normally."

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:406
> +        this.selectedNavigationItem?.element.focus();

Wat (okay, I looked it up)

[Nikita Vasilyev]

Comment on attachment 391596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391596&action=review

>> Source/WebInspectorUI/ChangeLog:25
>> +        Reset the default styles.
> 
> Why?

Reset the default styles so buttons that are just clickable icons (e.g. Force Dark Appearance) don't have the default border, padding, margin, and other style properties that vary between operation systems.

[Devin Rousso]

Comment on attachment 391596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391596&action=review

>>> Source/WebInspectorUI/ChangeLog:25
>>> +        Reset the default styles.
>> 
>> Why?
> 
> Reset the default styles so buttons that are just clickable icons (e.g. Force Dark Appearance) don't have the default border, padding, margin, and other style properties that vary between operation systems.

I don't think we should do this.  We've never done this in the past, and given the finicky nature of the CSS cascade I fear that this could easily cause other problems.

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:31
> +        role = role || "button";
> +        super(identifier, role);

Style: missing newline

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:163
> +        // keep you focused where you were â the console prompt.

Please avoid adding non UTF-8 characters to the source (especially JavaScript), as it can balloon the size of the backing store.

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.css:49
> -    outline: none;
> +    outline-offset: -4px;

Why?

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:-292
> -        // Only keep the tabIndex if already focused from keyboard navigation. This matches Xcode.

What about this?

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:295
> +            let isFocused = document.activeElement && this.element.contains(document.activeElement);

You should be able to drop the `document.activeElement` check, as passing `null` to `Node.prototype.contains` should always return `false`.  As such, you could also inline this.

> Source/WebInspectorUI/UserInterface/Views/RadioButtonNavigationItem.css:35
> +    outline-offset: -3px;

Why?

[Nikita Vasilyev]

Comment on attachment 391596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391596&action=review

>>>> Source/WebInspectorUI/ChangeLog:25
>>>> +        Reset the default styles.
>>> 
>>> Why?
>> 
>> Reset the default styles so buttons that are just clickable icons (e.g. Force Dark Appearance) don't have the default border, padding, margin, and other style properties that vary between operation systems.
> 
> I don't think we should do this.  We've never done this in the past, and given the finicky nature of the CSS cascade I fear that this could easily cause other problems.

What are you suggesting? Specifying `margin: 0; padding: 0; border: 0;`?
Or not use <button> at all and add a keydown event listener for Space and Enter?

[Nikita Vasilyev]

Created attachment 391722 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 391596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391596&action=review

>>>>> Source/WebInspectorUI/ChangeLog:25
>>>>> +        Reset the default styles.
>>>> 
>>>> Why?
>>> 
>>> Reset the default styles so buttons that are just clickable icons (e.g. Force Dark Appearance) don't have the default border, padding, margin, and other style properties that vary between operation systems.
>> 
>> I don't think we should do this.  We've never done this in the past, and given the finicky nature of the CSS cascade I fear that this could easily cause other problems.
> 
> What are you suggesting? Specifying `margin: 0; padding: 0; border: 0;`?
> Or not use <button> at all and add a keydown event listener for Space and Enter?

I excluded all changes related to converting <div role=button> to <button> from my patch since it isn't strictly required for this bug.

>> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:-292
>> -        // Only keep the tabIndex if already focused from keyboard navigation. This matches Xcode.
> 
> What about this?

The NavigationBar element itself doesn't have tabIndex. This hasn't been working as described for quite some time.

[WebKit Commit Bot]

Comment on attachment 391722 [details]
Patch

Clearing flags on attachment: 391722

Committed r257411: <https://trac.webkit.org/changeset/257411>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.