Bug 152473

Summary: Web Inspector: Storage tab navigation bar should fit on a single line
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, drousso, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: EasyFix, GoodFirstBug, InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[Animated GIF] Bug
none
Patch
none
Patch
none
[Animated GIF] Bug
none
Patch (reopened) none

Description Nikita Vasilyev 2015-12-20 22:18:57 PST
Created attachment 267736 [details]
[Animated GIF] Bug

See the attached animated GIF.
Comment 1 Devin Rousso 2015-12-29 20:34:11 PST
Created attachment 267997 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2015-12-29 20:34:22 PST
<rdar://problem/24023435>
Comment 3 Timothy Hatcher 2016-01-04 10:05:57 PST
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.
Comment 4 Devin Rousso 2016-01-04 11:53:12 PST
Created attachment 268212 [details]
Patch
Comment 5 Timothy Hatcher 2016-01-06 10:42:25 PST
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.
Comment 6 Devin Rousso 2016-01-06 14:38:30 PST
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?
Comment 7 Timothy Hatcher 2016-01-06 20:02:00 PST
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.
Comment 8 Devin Rousso 2016-01-11 00:07:21 PST
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)?
Comment 9 Timothy Hatcher 2016-01-11 16:48:43 PST
Oh, you are right. My bad!
Comment 10 WebKit Commit Bot 2016-01-11 17:40:39 PST
Comment on attachment 268212 [details]
Patch

Clearing flags on attachment: 268212

Committed r194879: <http://trac.webkit.org/changeset/194879>
Comment 11 WebKit Commit Bot 2016-01-11 17:40:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Nikita Vasilyev 2016-02-01 20:36:57 PST
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
Comment 13 Nikita Vasilyev 2016-02-17 22:02:15 PST
Created attachment 271629 [details]
[Animated GIF] Bug

I'm still seeing the bug on ToT (r196742) WebKit.
Comment 14 Nikita Vasilyev 2016-02-17 22:03:05 PST
Devin, could you take a look?
Comment 15 Devin Rousso 2016-02-17 22:28:23 PST
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.
Comment 16 Nikita Vasilyev 2016-02-17 22:35:00 PST
(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.
Comment 17 Nikita Vasilyev 2016-02-17 23:26:58 PST
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.
Comment 18 Devin Rousso 2016-02-17 23:57:34 PST
(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).
Comment 19 Timothy Hatcher 2016-02-18 00:14:58 PST
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;
Comment 20 Devin Rousso 2016-02-18 00:16:47 PST
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)?
Comment 21 Timothy Hatcher 2016-02-18 00:32:14 PST
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.
Comment 22 WebKit Commit Bot 2016-02-18 00:34:40 PST
Comment on attachment 271636 [details]
Patch (reopened)

Clearing flags on attachment: 271636

Committed r196750: <http://trac.webkit.org/changeset/196750>
Comment 23 WebKit Commit Bot 2016-02-18 00:34:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Joseph Pecoraro 2016-02-18 11:34:54 PST
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.