Bug 158700 - REGRESSION(r201177): Web Inspector: ContentBrowser navigation bar should fit on a single line
Summary: REGRESSION(r201177): Web Inspector: ContentBrowser navigation bar should fit ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
: 158885 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-06-13 11:30 PDT by Matt Baker
Modified: 2016-06-21 14:47 PDT (History)
7 users (show)

See Also:


Attachments
[Image] wrapping (116.39 KB, image/png)
2016-06-13 11:30 PDT, Matt Baker
no flags Details
WIP (1.18 KB, patch)
2016-06-20 13:06 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
[Animated GIF] An attempt to fix the bug (416.09 KB, image/gif)
2016-06-21 12:07 PDT, Nikita Vasilyev
no flags Details
[Animated GIF] --navigation-bar-height vs --navigation-bar-height - 1px (41.58 KB, image/gif)
2016-06-21 13:34 PDT, Nikita Vasilyev
no flags Details
Patch (1.31 KB, patch)
2016-06-21 13:36 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2016-06-13 11:30:02 PDT
Created attachment 281182 [details]
[Image] wrapping

* SUMMARY
ContentBrowser navigation bar should fit on a single line.

* STEPS TO REPRODUCE
1. Inspector > Timelines
2. Select Network timeline, navigation bar path grows
3. Reduce inspector window width to the minimum
  => "Show Details Sidebar" button wraps, controls become misaligned (see screenshot)
Comment 1 Radar WebKit Bug Importer 2016-06-13 11:30:32 PDT
<rdar://problem/26772058>
Comment 2 Matt Baker 2016-06-17 14:40:43 PDT
*** Bug 158885 has been marked as a duplicate of this bug. ***
Comment 3 Matt Baker 2016-06-17 15:16:09 PDT
Regressed in http://trac.webkit.org/changeset/201177.
Comment 4 Nikita Vasilyev 2016-06-20 13:02:49 PDT
This is similar to bug 157950.
Comment 5 Nikita Vasilyev 2016-06-20 13:06:36 PDT
Created attachment 281669 [details]
WIP

This fixes the bug.

I'm looking if there are any other areas affected
and if there's a way to solve this without setting
a fixed height.
Comment 6 Joseph Pecoraro 2016-06-20 13:20:22 PDT
Comment on attachment 281669 [details]
WIP

Curious, why is the height -1?
Comment 7 Nikita Vasilyev 2016-06-20 13:36:28 PDT
(In reply to comment #6)
> Comment on attachment 281669 [details]
> WIP
> 
> Curious, why is the height -1?

Good question. This is how it was before r201177.
It slightly changes vertical alignment of items.
Comment 8 Nikita Vasilyev 2016-06-21 12:07:39 PDT
Created attachment 281766 [details]
[Animated GIF] An attempt to fix the bug

I tried fixing this without setting a fixed item height.
I expected it to be easy, but so far I had no luck.

1. Adding `flex-wrap: nowrap` makes all the items to fit on a single line.
   Unfortunately, it also changes height of navigation bar items in the sidebar.

2. Adding `align-items: center` fixes the height issues.
   Unfortunately, it makes dividers invisible.
Comment 9 Nikita Vasilyev 2016-06-21 13:34:30 PDT
Created attachment 281769 [details]
[Animated GIF] --navigation-bar-height vs --navigation-bar-height - 1px
Comment 10 Nikita Vasilyev 2016-06-21 13:36:34 PDT
Created attachment 281770 [details]
Patch
Comment 11 BJ Burg 2016-06-21 13:41:20 PDT
Comment on attachment 281770 [details]
Patch

I don't understand how the analysis in this bug leads to this fix. Do we know why the bar started wrapping? Why do items in the navigation bar need to be shorter by 1 pixel? Is there a bug in the manual layout that's caused by setting height: auto instead of an explicit size? I don't think fiddling with the flex-* properties will lead to a fix if the bug is in our manual layout. Similarly, adjusting by 1px seems like a brittle fix if we don't know why it's necessary.
Comment 12 Joseph Pecoraro 2016-06-21 13:42:31 PDT
Comment on attachment 281770 [details]
Patch

r=me, seems fine.
Comment 13 Nikita Vasilyev 2016-06-21 13:58:28 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Comment on attachment 281669 [details]
> > WIP
> > 
> > Curious, why is the height -1?
> 
> Good question. This is how it was before r201177.
> It slightly changes vertical alignment of items.

Better answer:

.navigation-bar is 29px tall, but it has a 1px border at the bottom.
Since we use `box-sizing: border-box`, navigation bar items should be
28px tall, one pixel less than the navigation bar.
Comment 14 WebKit Commit Bot 2016-06-21 14:11:31 PDT
Comment on attachment 281770 [details]
Patch

Clearing flags on attachment: 281770

Committed r202294: <http://trac.webkit.org/changeset/202294>
Comment 15 WebKit Commit Bot 2016-06-21 14:11:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Nikita Vasilyev 2016-06-21 14:45:15 PDT
(In reply to comment #11)
> Comment on attachment 281770 [details]
> Patch
> 
> I don't understand how the analysis in this bug leads to this fix. Do we
> know why the bar started wrapping?

Yes, as Matt mentioned, it regressed in r201177.
Specifically, this change:
http://trac.webkit.org/changeset/201177/trunk/Source/WebInspectorUI/UserInterface/Views/NavigationBar.css

> Why do items in the navigation bar need
> to be shorter by 1 pixel? Is there a bug in the manual layout that's caused
> by setting height: auto instead of an explicit size? I don't think fiddling
> with the flex-* properties will lead to a fix if the bug is in our manual
> layout.

I think this is just how flexbox works, even though I find it
not very intuitive.

> Similarly, adjusting by 1px seems like a brittle fix if we don't
> know why it's necessary.

See my comment above.
Comment 17 Nikita Vasilyev 2016-06-21 14:47:31 PDT
Comment on attachment 281770 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.css:37
> +    height: calc(var(--navigation-bar-height) - 1px);

I should have just used `height: 100%`.