Bug 152473 – Web Inspector: Storage tab navigation bar should fit on a single line

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

[Nikita Vasilyev]

Created attachment 267736 [details]
[Animated GIF] Bug

See the attached animated GIF.

[Devin Rousso]

Created attachment 267997 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/24023435>

[Timothy Hatcher]

Comment on attachment 267997 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:374
> +            // Since ScopeBar is also a flexbox element, it has its own way of calculating its width.
> +            if (item instanceof WebInspector.ScopeBar)
> +                totalItemWidth += item.minimumWidth;
> +            else
> +                totalItemWidth += item.element.realOffsetWidth;

It would be best to bake this into NavigationItem's minimumWidth and have ScopeBar override it. That way we don't need instanceof check. Then other items can do custom things easily.

> Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:55
> +            this.element.style.flexWrap = "";

Would "none" or something explicit be better here? I am surprised this works.

> Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:57
> +            this.element.style.flexWrap = flexWrap;

It would be best to set this to null, so it is removed not set inline. That way the rule cascade with the original flex-wrap will be used again.

[Devin Rousso]

Created attachment 268212 [details]
Patch

[Timothy Hatcher]

Comment on attachment 268212 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/MultipleScopeBarItem.js:163
> +    displayWidestItem()
> +    {
> +        let widestLabel = null;
> +        let widestSize = 0;
> +        for (let option of Array.from(this._selectElement.options)) {
> +            this._titleElement.textContent = option.label;
> +            if (this._titleElement.realOffsetWidth > widestSize) {
> +                widestSize = this._titleElement.realOffsetWidth;
> +                widestLabel = option.label;
> +            }
> +        }
> +        this._titleElement.textContent = widestLabel;
> +    }

Clever, I hadn't thought about wide items that were not selected yet! Though I think a better approach would be to not expose displayWidestItem() (or displaySelectedItem()) since they are not really something you want to call normally.

A getter on MultipleScopeBarItem to get the width of the widest item is all you need. You could implement that getter by doing this code internally and reverting it to the selected item at the end.

> Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:56
> +            if (this._multipleItem)
> +                this._multipleItem.displayWidestItem();

See comment above.

[Devin Rousso]

Comment on attachment 268212 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/MultipleScopeBarItem.js:163
>> +    }
> 
> Clever, I hadn't thought about wide items that were not selected yet! Though I think a better approach would be to not expose displayWidestItem() (or displaySelectedItem()) since they are not really something you want to call normally.
> 
> A getter on MultipleScopeBarItem to get the width of the widest item is all you need. You could implement that getter by doing this code internally and reverting it to the selected item at the end.

So my only issue with this is that, from what info I have been able to find, getBoundingClientRect (which is what realOffsetWidth uses) doesn't return a width which includes the margins of the element.  If the margins aren't included in the calculations, it is possible (haven't confirmed it) that the real width is greater than the calculated sum returned by this and the other scope bar items (which would also be a more lengthy process, as it would require looping through every scope bar item).  The only other way that I thought of to go around this issue was to add another prototype method similar to realOffsetWidth that includes the margin info, but due to the way that margins can overlap, I wasn't sure that that was the right answer either.  I agree that this isn't something we want to expose, but I'm not sure what the right way to go about doing this is... Thoughts?

[Timothy Hatcher]

Comment on attachment 268212 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/MultipleScopeBarItem.js:163
>>> +    }
>> 
>> Clever, I hadn't thought about wide items that were not selected yet! Though I think a better approach would be to not expose displayWidestItem() (or displaySelectedItem()) since they are not really something you want to call normally.
>> 
>> A getter on MultipleScopeBarItem to get the width of the widest item is all you need. You could implement that getter by doing this code internally and reverting it to the selected item at the end.
> 
> So my only issue with this is that, from what info I have been able to find, getBoundingClientRect (which is what realOffsetWidth uses) doesn't return a width which includes the margins of the element.  If the margins aren't included in the calculations, it is possible (haven't confirmed it) that the real width is greater than the calculated sum returned by this and the other scope bar items (which would also be a more lengthy process, as it would require looping through every scope bar item).  The only other way that I thought of to go around this issue was to add another prototype method similar to realOffsetWidth that includes the margin info, but due to the way that margins can overlap, I wasn't sure that that was the right answer either.  I agree that this isn't something we want to expose, but I'm not sure what the right way to go about doing this is... Thoughts?

I wasn't suggesting you change the way you get the width. I just meant you should encapsulate the measurement and text swapping into a simple getter that returns the number you were already getting. That way the caller does not need these two functions, the swap is internal.

[Devin Rousso]

Comment on attachment 268212 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Views/MultipleScopeBarItem.js:163
>>>> +    }
>>> 
>>> Clever, I hadn't thought about wide items that were not selected yet! Though I think a better approach would be to not expose displayWidestItem() (or displaySelectedItem()) since they are not really something you want to call normally.
>>> 
>>> A getter on MultipleScopeBarItem to get the width of the widest item is all you need. You could implement that getter by doing this code internally and reverting it to the selected item at the end.
>> 
>> So my only issue with this is that, from what info I have been able to find, getBoundingClientRect (which is what realOffsetWidth uses) doesn't return a width which includes the margins of the element.  If the margins aren't included in the calculations, it is possible (haven't confirmed it) that the real width is greater than the calculated sum returned by this and the other scope bar items (which would also be a more lengthy process, as it would require looping through every scope bar item).  The only other way that I thought of to go around this issue was to add another prototype method similar to realOffsetWidth that includes the margin info, but due to the way that margins can overlap, I wasn't sure that that was the right answer either.  I agree that this isn't something we want to expose, but I'm not sure what the right way to go about doing this is... Thoughts?
> 
> I wasn't suggesting you change the way you get the width. I just meant you should encapsulate the measurement and text swapping into a simple getter that returns the number you were already getting. That way the caller does not need these two functions, the swap is internal.

I'm not entirely sure what you mean here by "the number you were already getting".  I don't actually use the number calculated inside displayWidestItem() since I calculate the minimum width based on the entire scope bar.  If I just used the number inside displayWidestItem() then I would need to loop over the items in the ScopeBar and sum all their minimum widths (which _could_ prove to be an issue due to the margin issue I mentioned earlier).  Would it be a valid solution to make this function accept a ScopeBar object and perform all the logic inside ScopeBar.js:48-62 and then reset the title element (this way the changing of the title element is kept internal, but it seems like a layering violation since it's performing changes on its parent)?

[Timothy Hatcher]

Oh, you are right. My bad!

[WebKit Commit Bot]

Comment on attachment 268212 [details]
Patch

Clearing flags on attachment: 268212

Committed r194879: <http://trac.webkit.org/changeset/194879>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikita Vasilyev]

Possibly related bug:
https://bugs.webkit.org/show_bug.cgi?id=153412
Web Inspector: Navigation bar in sidebars should always fit on a single line

[Nikita Vasilyev]

Created attachment 271629 [details]
[Animated GIF] Bug

I'm still seeing the bug on ToT (r196742) WebKit.

[Nikita Vasilyev]

Devin, could you take a look?

[Devin Rousso]

Created attachment 271636 [details]
Patch (reopened)

(In reply to comment #14)
> Devin, could you take a look?

So, I took a look and it seems that this is reproducible when the user makes the inspector window very wide, increases the sidebar width to its minimum, shrinks the inspector window, and then tries to shrink the sidebar width.

The fix in <https://bugs.webkit.org/show_bug.cgi?id=153412> wasn't as good as I thought because of this case:

User makes inspector wide
 - Sidebar Maximum: 500px (1/3 width of window)
 - Sidebar Minimum: 220px
User shrinks inspector
 - Sidebar Maximum: 170px (1/3 width of window)
 - Sidebar Minimum: 220px

In this case, Number.constrain will be called with 220 as min and 170 as max.  The fix in the bug above would switch these two values, making the range [170, 220], but this is an issue as we can't really allow any widths less than 220 as that is the absolute minimum.  As a result, instead of flipping the range, if max < min, just return min since we already can't have any value that is less than the minimum.

[Nikita Vasilyev]

(In reply to comment #15)
> Created attachment 271636 [details]
> Patch (reopened)
> 
> (In reply to comment #14)
> > Devin, could you take a look?
> 
> So, I took a look and it seems that this is reproducible when the user makes
> the inspector window very wide, increases the sidebar width to its minimum,
> shrinks the inspector window, and then tries to shrink the sidebar width.

I can reproduce it on the detached inspector with minimum width.
If I close it and open it again, the problem is still there.

[Nikita Vasilyev]

Comment on attachment 271636 [details]
Patch (reopened)

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:860
> +

This fixes the bug. However, I don't think we should change Number.constrain.
I would expect Number.constrain(1, 2, 0) to return 1, as 1 is within the range from 2 to 0.
I find returning 2 to be counter intuitive.

[Devin Rousso]

(In reply to comment #17)
> Comment on attachment 271636 [details]
> Patch (reopened)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271636&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Utilities.js:860
> > +
> 
> This fixes the bug. However, I don't think we should change Number.constrain.
> I would expect Number.constrain(1, 2, 0) to return 1, as 1 is within the
> range from 2 to 0.
> I find returning 2 to be counter intuitive.

While I do agree with your point, I think that if we are planning on using the names of the variables inside Number.constrain to indicate their values, then that use case already doesn't make much sense.  When I read "Number.constrain(num, min, max)" I expect that the function would treat the value for min as the lower bound and max as the upper bound, regardless of their values.  In any circumstance, the return value should never be less than the value given by min, which is why I changed it in the patch.  This issue right here is a perfect example of why I think that we should make the function treat the parameters literally according to their names (min is always the minimum and max is always the maximum).

[Timothy Hatcher]

Comment on attachment 271636 [details]
Patch (reopened)

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:858
> +        if (max < min)

Makes sense. We should also add if (min > max) return max;

[Devin Rousso]

Comment on attachment 271636 [details]
Patch (reopened)

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

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:858
>> +        if (max < min)
> 
> Makes sense. We should also add if (min > max) return max;

Isn't (min > max) == (max < min)?

[Timothy Hatcher]

Comment on attachment 271636 [details]
Patch (reopened)

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

>>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:858
>>> +        if (max < min)
>> 
>> Makes sense. We should also add if (min > max) return max;
> 
> Isn't (min > max) == (max < min)?

Yes, sorry not thinking clearly.

[WebKit Commit Bot]

Comment on attachment 271636 [details]
Patch (reopened)

Clearing flags on attachment: 271636

Committed r196750: <http://trac.webkit.org/changeset/196750>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 271636 [details]
Patch (reopened)

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

>>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:860
>>> +
>> 
>> This fixes the bug. However, I don't think we should change Number.constrain.
>> I would expect Number.constrain(1, 2, 0) to return 1, as 1 is within the range from 2 to 0.
>> I find returning 2 to be counter intuitive.
> 
> While I do agree with your point, I think that if we are planning on using the names of the variables inside Number.constrain to indicate their values, then that use case already doesn't make much sense.  When I read "Number.constrain(num, min, max)" I expect that the function would treat the value for min as the lower bound and max as the upper bound, regardless of their values.  In any circumstance, the return value should never be less than the value given by min, which is why I changed it in the patch.  This issue right here is a perfect example of why I think that we should make the function treat the parameters literally according to their names (min is always the minimum and max is always the maximum).

I wrote tests for Number.constrain in LayoutTests/inspector/unit-tests/number-utilities.html and I assume the strict `num, min, max` order.

New tests should be added where `max` is less than `min` to test this change and provide sample usage.