Bug 211118 – Web Inspector: Computed: shouldn't display focus outline on click

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 397813 [details]
[Video] Bug

In the computed panel, when clicking on the property it shows a clipped outline.

There're two issues here:
1. Focus outline is clipped.
2. We shouldn't display focus outline on clicking at all. We should only display focus outline around the disclosure triangle when tabbing into it.

[Radar WebKit Bug Importer]

<rdar://problem/62491002>

[Nikita Vasilyev]

Created attachment 397816 [details]
Patch

[Nikita Vasilyev]

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

[Devin Rousso]

Comment on attachment 397816 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:-40
> -    overflow: hidden;
> -    text-overflow: ellipsis;

I think this will have problems if a CSS property has a long non-word-breakable value, such as with `background-image`.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:49
> +    outline-offset: -2px;

This should be updated to use `var(--focus-ring-outline-offset)`.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:50
> +    border-radius: 6px;

How is `6px` determined?

> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:32
> +        this._disclosureButton = null;

Why is this needed?

[Nikita Vasilyev]

Comment on attachment 397816 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:-40
>> -    text-overflow: ellipsis;
> 
> I think this will have problems if a CSS property has a long non-word-breakable value, such as with `background-image`.

You're right. This is limiting the size of the focus outline around the disclosure triangle but I can work with that.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:50
>> +    border-radius: 6px;
> 
> How is `6px` determined?

It's very odd to have square outline around the triangle. I don't think every pixel value should be justified. What are you suggesting?

I can't find any focusable disclosure triangles on macOS Catalina right now — I remember previously seeing blue circle (filled in the middle, not a ring) for these cases.

>> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:32
>> +        this._disclosureButton = null;
> 
> Why is this needed?

Oops, it isn't. Left over from an unpublished patch.

[Nikita Vasilyev]

Comment on attachment 397816 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:32
>>> +        this._disclosureButton = null;
>> 
>> Why is this needed?
> 
> Oops, it isn't. Left over from an unpublished patch.

^ ignore my previous comment. I wanted to have `this._disclosureButton` always defined on the class - isn't what we usually want to do?

[Devin Rousso]

Comment on attachment 397816 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:50
>>> +    border-radius: 6px;
>> 
>> How is `6px` determined?
> 
> It's very odd to have square outline around the triangle. I don't think every pixel value should be justified. What are you suggesting?
> 
> I can't find any focusable disclosure triangles on macOS Catalina right now — I remember previously seeing blue circle (filled in the middle, not a ring) for these cases.

I meant more "how did you come up with `6px`?".  Is it based on the size of the triangle somehow?  Is it something we could `calc` based on an existing variable?

>>>> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:32
>>>> +        this._disclosureButton = null;
>>> 
>>> Why is this needed?
>> 
>> Oops, it isn't. Left over from an unpublished patch.
> 
> ^ ignore my previous comment. I wanted to have `this._disclosureButton` always defined on the class - isn't what we usually want to do?

Yes and no.  We want to have it defined if we expect it to be defined later in the object's lifetime (we especially want to make sure the type of the member stays the same), but we don't often bother creating a member variable if it is only defined once.  In this case, `this._disclosureButton` is not defined (only used) after the object is constructed, so we shouldn't need to define it regardless.  This is a similar case to what we do with `WI.NavigationItem` in many `WI.ContentView` subclasses.

[Nikita Vasilyev]

Created attachment 397897 [details]
[Video] Jumping text

(In reply to Nikita Vasilyev from comment #5)
> Comment on attachment 397816 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397816&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:-40
> >> -    text-overflow: ellipsis;
> > 
> > I think this will have problems if a CSS property has a long non-word-breakable value, such as with `background-image`.
> 
> You're right. This is limiting the size of the focus outline around the
> disclosure triangle but I can work with that.

Somehow `overflow: hidden` there makes text jump down 1px when focused. This is happening without my patch already. I don't understand where this's coming from.

[Nikita Vasilyev]

Created attachment 397900 [details]
Patch

[Nikita Vasilyev]

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

[Devin Rousso]

Comment on attachment 397900 [details]
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);
```

[Nikita Vasilyev]

Comment on attachment 397900 [details]
Patch

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

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

I don't think we should do this. If --focus-ring-outline-offset changes, it may break here.

[Nikita Vasilyev]

Created attachment 398137 [details]
Patch

I finally figured out a workaround for the jiggly elements. Now only the expanded element has the half a pixel borders.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #13)
> Created attachment 398137 [details]
> Patch
> 
> I finally figured out a workaround for the jiggly elements. Now only the
> expanded element has the half a pixel borders.

...and it doesn't take any more space than before the patch.

[EWS]

Committed r260978: <https://trac.webkit.org/changeset/260978>

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