1695617 - The toolbar library bookmarks and history panels have a scrollbar no matter how short they are

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P2)

Description

[Alin Ilea, Desktop Test Engineering]

Attachment: bookmarks scrollbar.png

Tested with:

Nightly 88.0a1 (2021-03-01)

Tested on:

Windows 10
MacOS 10.15
Ubuntu 20.04

Preconditions:

In about:config, set browser.proton.appmenu.enabled = true

Steps:

1. Launch firefox and open the Bookmarks or History (from the Toolbar Library).

Actual result:

There is a scrollbar displayed no matter how long or short the list of recent bookmarks/history is.

Expected result:

The scrollbar should not be displayed if the Bookmarks/History Panel is shorter than the screen length.

Notes:

1. The issue is reproducible with and without Proton enabled.
2. This is not reproducible in Release 86 or Beta 87.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Do you have multiple displays? Are they running at different DPIs? Have you changed your system font sizes? Any kinds of these details will help uncover what is going on here.

Also, are you saying that the bug goes away if you set browser.proton.appmenu.enabled = false?

Comment 2

[Alin Ilea, Desktop Test Engineering]

Attachment: aboutsupport.txt

Hi Jared,
I have dual monitors with the same resolution, but this is happening using one monitor as well. Also it is reproducible on Macbook macOS 10.15 without any other monitor/display connected. I did not changed the system fonts or other OS settings. I can reproduce it with new clean Fx profile.
And for the last question, the issue is reproducible with proton pref disabled (set to false) as well. I attached here the about:support, maybe it helps.

Comment 3

[:Gijs (he/him)]

I can reproduce this on my Windows machine (Win10, 150% dpi, single screen, no text size changes from the default), and this regressed with bug 1688960.

From a very quick look at the patch, I expect 1 of 2 root causes: a rounding error of sorts, or the font size the header inherits is different between the off-screen place where we check its height compared to the main panel where we then insert it (as the new styling uses font-size: inherit in a few different places).

Mike, can you take a look? If you struggle to repro on Windows please let me know and I can take another look myself.

In terms of priority, I think this is a polish issue that we should try and get fixed in time for 88 - it's not critical, but it does look a bit naff as-is.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

[Tracking Requested - why for this release]:

With all that's going on, I don't want to lose track of this. It's a polish-y issue, but in some pretty front-and-center UI, so we should get to this before the uplift.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See What Do You Triage for more information

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1695617 - Add h1 and h2 elements to descriptionHeightWorkaround query. r?Gijs!

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Hey Gijs,

I noticed a similar bug when applying mtigley's patch for bug 1694277. The patch I've attached seemed to fix it. Does it also fix the Library case that you were able to reproduce?

Comment 8

[:Gijs (he/him)]

Unfortunately no. I left a more detailed comment on phab, let's continue there.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Hey rtestard, do you think this is sufficiently important for me to look at (it's reproducible with / without Proton on 88+) instead of Proton work? Or can we ship 88 with this regression?

Comment 10

[Romain Testard [:RT]]

This one seems like a medium P2 so marking [priority:2b]
We should focus on P1 or [priority:2a] priorities instead - OK to let this regression go with 88 even if it's a shame but we have to be realistic on what can be done in a given time :(.

Comment 11

[Timea Cernea [:tbabos][inactive]]

Attachment: MacOS - Bookmarks and History Panel

This is also reproducible on MacOS for the Bookmarks and History panels toggled from the toolbar library button.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Coming back round to this. I too am able to reproduce this in macOS now. Investigating.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

This is pretty strange. It seems that the .panel-subview-body element is getting an intrinsic height that doesn't seem to take into account the h2's padding and non-default font-size. If I reset the font-size to the default, then the scrollbar goes away. It's truly odd, and I can't really explain it.

I've also noticed that there seems to be a minimum height on the subheader of 24px which was not helping.

Interestingly, the problem seems to go away if I make the .panel-subview-body element use the CSS flexbox model instead of the XUL flexbox model. That's not without risk, so I don't think this is something we'd want to consider uplifting.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1695617 - Avoid a needless scrollbar in the History/Bookmark subviews by using CSS flex box model rather than the XUL flex box model. r?Gijs!

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1695617 - Make sure subview subheader elements use the XUL box model. r?Gijs!

It seems that we've hit another instance of the XUL box model interacting poorly
with the HTML box model in mysterious ways. In this particular case, the <h2>
elements for subview subheaders was using the HTML box model while inside of
a tree of XUL box elements, and this was causing an incorrect height calculation
for the containing scrollable node, the panel-subview-body.

Switching the h2 element to use the XUL box model appears to fix this.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b1c1f980faa
Make sure subview subheader elements use the XUL box model. r=Gijs

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/0b1c1f980faa

Comment 19

[Romain Testard [:RT]]

Can this one be uplifted to Beta 89 please?

Comment 20

[:Gijs (he/him)]

Comment on attachment 9220420 [details]
Bug 1695617 - Make sure subview subheader elements use the XUL box model. r?Gijs!

Beta/Release Uplift Approval Request

• User impact if declined: MR1 / proton impact: useless scrollbar in some menus/panels
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0
• List of other uplifts needed: nope
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): 1 line CSS change
• String changes made/needed: Nope

Comment 21

[Pascal Chevrel:pascalc]

Comment on attachment 9220420 [details]
Bug 1695617 - Make sure subview subheader elements use the XUL box model. r?Gijs!

Low risk, approved for 89 beta 10, thanks.

Comment 22

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/26cb4c90d0c2

Comment 23

[Alin Ilea, Desktop Test Engineering]

Verified - Fixed in latest Nightly 90.0a1 (2021-05-07) and Beta 89.0b10 (build id: 20210507073536), using Windows 10, Ubuntu 20.04 and macOS 11. The scrollbar is not displayed anymore in the toolbar library bookmarks and history panels. It is displayed only when the list of bookmarks or recent history is long enough to be scrollable.