[webkit-reviews] review granted: [Bug 211118] Web Inspector: Computed: shouldn't display focus outline on click : [Attachment 397900] 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 211118: Web Inspector: Computed: shouldn't display focus outline on click
https://bugs.webkit.org/show_bug.cgi?id=211118

Attachment 397900: Patch

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




--- Comment #11 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 397900
  --> https://bugs.webkit.org/attachment.cgi?id=397900
Patch

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

r=me, with a few comments/NITs

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:35
> +    min-height: calc(var(--disclosure-button-size) + 1px); /* The magic 1px
is needed so the element doesn't move 1px down when .disclosure-button is
focused. */

Is there no other way around this?  This will end up "wasting" a lot of space
:(

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:51
> +    outline-offset: -3px; /* Make focus outline smaller than usual so it
doesn't get clipped here. */

This should be `calc(var(--focus-ring-outline-offset) - 1px)`

> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:67
> +	   if (this._disclosureButton)
> +	       this._disclosureButton.ariaExpanded = isExpanded;

NIT: if you use `Element.prototype.setAttribute`, you can do this on one line
with optional chaining =D
```
    this._disclosureButton?.setAttribute("aria-expanded", isExpanded);
```