[webkit-reviews] review granted: [Bug 208283] Web Inspector: AXI: disabled buttons shouldn't be focusable : [Attachment 392847] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 208283: Web Inspector: AXI: disabled buttons shouldn't be focusable
https://bugs.webkit.org/show_bug.cgi?id=208283

Attachment 392847: Patch

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




--- Comment #11 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 392847
  --> https://bugs.webkit.org/attachment.cgi?id=392847
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.