Bug 208283 – Web Inspector: AXI: disabled buttons shouldn't be focusable

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

[Nikita Vasilyev]

When pressing Tab/Shift-Tab, it shouldn't focus on disabled buttons.

[Radar WebKit Bug Importer]

<rdar://problem/59832150>

[Nikita Vasilyev]

Created attachment 391831 [details]
Patch

[Devin Rousso]

Comment on attachment 391831 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:44
> +        if (this._role === "button")

Do we ever actually pass in anything that isn't `"button"`?  I see this type of logic all over the place with navigation related things and I don't remember it ever actually being used anywhere.  Perhaps we can just remove the `role` parameter entirely and always use `"button"`?

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:208
> +        if (this._role === "button")

Shouldn't we `this.element.removeAttribute("tabindex")` in the `else`?

[Nikita Vasilyev]

Comment on attachment 391831 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:44
>> +        if (this._role === "button")
> 
> Do we ever actually pass in anything that isn't `"button"`?  I see this type of logic all over the place with navigation related things and I don't remember it ever actually being used anywhere.  Perhaps we can just remove the `role` parameter entirely and always use `"button"`?

WI.RadioButtonNavigationItem passes "tab".
(Perhaps WI.RadioButtonNavigationItem should be renamed to something like WI.TabButtonNavigationItem. This is out of the scope of this bug.)

>> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:208
>> +        if (this._role === "button")
> 
> Shouldn't we `this.element.removeAttribute("tabindex")` in the `else`?

It wouldn't make a difference. It would only run for WI.RadioButtonNavigationItem during the class instantiation, and RadioButtonNavigationItem doesn't have any tabIndex set at that time.
RadioButtonNavigationItem is never disabled so _updateTabIndex would never get called from there.

[Nikita Vasilyev]

I don't want to do this in this patch, but perhaps it's worth refactoring RadioButtonNavigationItem.

Currently, RadioButtonNavigationItem extends ButtonNavigationItem, which extends NavigationItem.
Perhaps it would be less confusing if RadioButtonNavigationItem extended NavigationItem directly.

[Devin Rousso]

Comment on attachment 391831 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:208
>>> +        if (this._role === "button")
>> 
>> Shouldn't we `this.element.removeAttribute("tabindex")` in the `else`?
> 
> It wouldn't make a difference. It would only run for WI.RadioButtonNavigationItem during the class instantiation, and RadioButtonNavigationItem doesn't have any tabIndex set at that time.
> RadioButtonNavigationItem is never disabled so _updateTabIndex would never get called from there.

What about if an instance of `WI.RadioButtonNavigationItem` called `.enabled = false;` and then `.enabled = true`?  It would get called then, and the `tabIndex` wouldn't be reset.

[Nikita Vasilyev]

Created attachment 392838 [details]
Patch

[Devin Rousso]

Comment on attachment 392838 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:164
> +        if (this._role === "button")
> +            this.element.tabIndex = 0;
> +        else
> +            this.element.tabIndex = -1;

Why not just inline this `this.element.tabIndex = (this._role === "button") ? 0 : -1;`?

> Source/WebInspectorUI/UserInterface/Views/RadioButtonNavigationItem.js:72
> +        if (!this._enabled) {

Style: in subclasses, we shouldn't use the private variables of superclasses, so please use `this.enabled`

> Source/WebInspectorUI/UserInterface/Views/RadioButtonNavigationItem.js:80
> +        if (this.selected)
> +            this.element.tabIndex = 0;
> +        else
> +            this.element.tabIndex = -1;

Why not just have this as part of the `if` in `super.updateTabIndex`?  All basic `WI.ButtonNavigationItem` will already pass `this._role === "button"`, and everyone else should only be tabbabale if they're selected.

At the very least, this deserves an explanation (or at least a warning) that you're not calling `super.updateTabIndex`.  I'd really prefer it if you changed it so that it doesn't completely override `super.updateTabIndex`, as that can make it harder to adjust things in the future as someone reading the base class won't necessarily know about the fact that this was overriding it and would therefore not be affected by any of the aforementioned changes.

[Nikita Vasilyev]

Comment on attachment 392838 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/RadioButtonNavigationItem.js:72
>> +        if (!this._enabled) {
> 
> Style: in subclasses, we shouldn't use the private variables of superclasses, so please use `this.enabled`

Right, thanks.

>> Source/WebInspectorUI/UserInterface/Views/RadioButtonNavigationItem.js:80
>> +            this.element.tabIndex = -1;
> 
> Why not just have this as part of the `if` in `super.updateTabIndex`?  All basic `WI.ButtonNavigationItem` will already pass `this._role === "button"`, and everyone else should only be tabbabale if they're selected.
> 
> At the very least, this deserves an explanation (or at least a warning) that you're not calling `super.updateTabIndex`.  I'd really prefer it if you changed it so that it doesn't completely override `super.updateTabIndex`, as that can make it harder to adjust things in the future as someone reading the base class won't necessarily know about the fact that this was overriding it and would therefore not be affected by any of the aforementioned changes.

Only selected radio button should be tabbable.

I can call super.updateTabIndex for consistency. It would make no difference here.

[Nikita Vasilyev]

Created attachment 392847 [details]
Patch

[Devin Rousso]

Comment on attachment 392847 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/RadioButtonNavigationItem.js:74
> +        if (!this.enabled) {

It'd be nice to avoid doing this check twice.  Right now, we also could potentially end up setting the `tabIndex` twice in this function, which is also unfortunate.  Perhaps we could create a protected `get tabbable` that _can_ be overridden by subclasses.

ButtonNavigationItem.js
```
    updateTabIndex()
    {
        if (!this._enabled) {
            this.element.tabIndex = -1;
            return;
        }

        this.element.tabIndex = this.tabbable ? 0 : -1;
    }

    get tabbable()
    {
        // Can be overridden by subclasses.
        return this._role === "button";
    }
```

RadioButtonNavigationItem.js
```
    get tabbable()
    {
        return this.selected;
    }
```

I guess if you do end up removing `WI.ButtonNavigationItem` as a superclass of `WI.RadioButtonNavigationItem`, then this would kinda have to be needed tho.

[Nikita Vasilyev]

Comment on attachment 392847 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/RadioButtonNavigationItem.js:74
>> +        if (!this.enabled) {
> 
> It'd be nice to avoid doing this check twice.  Right now, we also could potentially end up setting the `tabIndex` twice in this function, which is also unfortunate.  Perhaps we could create a protected `get tabbable` that _can_ be overridden by subclasses.
> 
> ButtonNavigationItem.js
> ```
>     updateTabIndex()
>     {
>         if (!this._enabled) {
>             this.element.tabIndex = -1;
>             return;
>         }
> 
>         this.element.tabIndex = this.tabbable ? 0 : -1;
>     }
> 
>     get tabbable()
>     {
>         // Can be overridden by subclasses.
>         return this._role === "button";
>     }
> ```
> 
> RadioButtonNavigationItem.js
> ```
>     get tabbable()
>     {
>         return this.selected;
>     }
> ```
> 
> I guess if you do end up removing `WI.ButtonNavigationItem` as a superclass of `WI.RadioButtonNavigationItem`, then this would kinda have to be needed tho.

I like the `tabbable` getter!

[Nikita Vasilyev]

Created attachment 394014 [details]
Patch

[EWS]

Committed r258730: <https://trac.webkit.org/changeset/258730>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394014 [details].

[Devin Rousso]

Comment on attachment 394014 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:179
> +        return this._role === "button";

NIT: this probably could've had a `// Can be overridden by subclasses.` comment :P