WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
152473
Web Inspector: Storage tab navigation bar should fit on a single line
https://bugs.webkit.org/show_bug.cgi?id=152473
Summary
Web Inspector: Storage tab navigation bar should fit on a single line
Nikita Vasilyev
Reported
2015-12-20 22:18:57 PST
Created
attachment 267736
[details]
[Animated GIF] Bug See the attached animated GIF.
Attachments
[Animated GIF] Bug
(159.05 KB, image/gif)
2015-12-20 22:18 PST
,
Nikita Vasilyev
no flags
Details
Patch
(13.39 KB, patch)
2015-12-29 20:34 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(15.30 KB, patch)
2016-01-04 11:53 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
[Animated GIF] Bug
(108.69 KB, image/gif)
2016-02-17 22:02 PST
,
Nikita Vasilyev
no flags
Details
Patch (reopened)
(1.42 KB, patch)
2016-02-17 22:28 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Devin Rousso
Comment 1
2015-12-29 20:34:11 PST
Created
attachment 267997
[details]
Patch
Radar WebKit Bug Importer
Comment 2
2015-12-29 20:34:22 PST
<
rdar://problem/24023435
>
Timothy Hatcher
Comment 3
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.
Devin Rousso
Comment 4
2016-01-04 11:53:12 PST
Created
attachment 268212
[details]
Patch
Timothy Hatcher
Comment 5
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.
Devin Rousso
Comment 6
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?
Timothy Hatcher
Comment 7
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.
Devin Rousso
Comment 8
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)?
Timothy Hatcher
Comment 9
2016-01-11 16:48:43 PST
Oh, you are right. My bad!
WebKit Commit Bot
Comment 10
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
>
WebKit Commit Bot
Comment 11
2016-01-11 17:40:42 PST
All reviewed patches have been landed. Closing bug.
Nikita Vasilyev
Comment 12
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
Nikita Vasilyev
Comment 13
2016-02-17 22:02:15 PST
Created
attachment 271629
[details]
[Animated GIF] Bug I'm still seeing the bug on ToT (
r196742
) WebKit.
Nikita Vasilyev
Comment 14
2016-02-17 22:03:05 PST
Devin, could you take a look?
Devin Rousso
Comment 15
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.
Nikita Vasilyev
Comment 16
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.
Nikita Vasilyev
Comment 17
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.
Devin Rousso
Comment 18
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).
Timothy Hatcher
Comment 19
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;
Devin Rousso
Comment 20
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)?
Timothy Hatcher
Comment 21
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.
WebKit Commit Bot
Comment 22
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
>
WebKit Commit Bot
Comment 23
2016-02-18 00:34:44 PST
All reviewed patches have been landed. Closing bug.
Joseph Pecoraro
Comment 24
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug