Bug 208283

<rdar://problem/59832150>

Created attachment 391831 [details]
Patch

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`?

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.

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.

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.

Created attachment 392838 [details]
Patch

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.

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.

Created attachment 392847 [details]
Patch

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.

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!

Created attachment 394014 [details]
Patch

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

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

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