Bug 158700 – REGRESSION(r201177): Web Inspector: ContentBrowser navigation bar should fit on a single line

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

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)

[Radar WebKit Bug Importer]

<rdar://problem/26772058>

[Matt Baker]

*** Bug 158885 has been marked as a duplicate of this bug. ***

[Matt Baker]

Regressed in http://trac.webkit.org/changeset/201177.

[Nikita Vasilyev]

This is similar to bug 157950.

[Nikita Vasilyev]

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.

[Joseph Pecoraro]

Comment on attachment 281669 [details]
WIP

Curious, why is the height -1?

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 281769 [details]
[Animated GIF] --navigation-bar-height vs --navigation-bar-height - 1px

[Nikita Vasilyev]

Created attachment 281770 [details]
Patch

[Blaze Burg]

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.

[Joseph Pecoraro]

Comment on attachment 281770 [details]
Patch

r=me, seems fine.

[Nikita Vasilyev]

(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.

[WebKit Commit Bot]

Comment on attachment 281770 [details]
Patch

Clearing flags on attachment: 281770

Committed r202294: <http://trac.webkit.org/changeset/202294>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

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%`.