1377793 - Hamburger menu dropdown is truncated, no scrollbar

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: UI regression

Reproducible: 55.0beta only (At present, Nightly 56 is a photon structure, so there is a possibility that it is not affected)

Steps To Reproduce:
1. Place many toolbarbutton ino Hamburger menu
2. (if necessary) Reduce browser height and position so the hamburger menu dropdown will be overflowed
3. Open hamburger menu

Actual Results:
Hamburger menu dropdown is truncated, no scrollbar.
Unable to access some buttons(Customize/Help/Exit button etc...)

Expected Results:
Hamburger menu dropdown should appear vertical scrollbar

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=90956bd266a83c70d81fd7806b5d660fb0afba10&tochange=2d16236446afaceb84c2d09c0d8e1056c8001ffd

Regressed by:
2d16236446af	Paolo Amadini — Bug 1009116 - Redo resizing architecture of panelmultiview. r=Gijs

Comment 2

[Alice0775 White]

Attachment: screenshot 55.0b6 vs 54.0.1

Comment 3

[Alice0775 White]

And I can also reproduce the problem on Nightly56.0a1(2017-07-02) if set browser.photon.structure.enabled to false

Comment 4

[:Gijs (he/him)]

I don't think we need to track this as part of Photon (so I'm removing the whiteboard flags), but we should track for 55 and either fix or back out bug 1009116 for 55 (and potentially 56).

Comment 5

[:Paolo Amadini]

I'll take a look, this is more likely a result of CSS changes than of the way we do the resizing.

Comment 6

[Julien Cristau [:jcristau]]

tracking for 55 per comment 4.

Comment 7

[:Paolo Amadini]

So, it turns out this is another case of box-in-block layout issues. I have a patch that works around this by using box layout when the height of the view exceeds the available space.

This was possibly one of the reasons, or maybe the only reason, why the overflow listeners were present in the previous architecture. While ideally this should be handled by the platform layout code, now at least this workaround is marked as such and is self-contained in a few lines.

Comment 8

[:Paolo Amadini]

Attachment: Bug 1377793 - Fix scrolling in the non-Photon main menu.

Review commit: https://reviewboard.mozilla.org/r/154252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154252/

Comment 9

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f37ac4f1c07bd6900622787cb2ca070afcf7ce80

Comment 10

[:Gijs (he/him)]

Comment on attachment 8883361 [details]
Bug 1377793 - Fix scrolling in the non-Photon main menu.

https://reviewboard.mozilla.org/r/154252/#review159568

This looks pretty hacky, but it seems to work OK in my testing.

My only nit is that this breaks whimsy (empty your panel). I don't think that should stop us shipping this though, given it's all going away with Photon.

Comment 11

[:Paolo Amadini]

(In reply to :Gijs from comment #10)
> My only nit is that this breaks whimsy (empty your panel).

That's the weirdest unexplained consequence of a CSS change... I thought everything was fine, and someone intentionally changed the animation to have the whimsycorn charge towards the right side of the panel!

Anyways, I realized that I removed a property that was necessary in customize mode to keep the entire area as a drag-and-drop target, and restoring it fixed the animation as well, for some reason.

Comment 12

[:Paolo Amadini]

Comment on attachment 8883361 [details]
Bug 1377793 - Fix scrolling in the non-Photon main menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154252/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cb18ada2641
Fix scrolling in the non-Photon main menu. r=Gijs

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4cb18ada2641

Comment 15

[Gerry Chang [:gchang]]

Hi Alice,
Can you help check if this is fixed in latest nightly?

Comment 16

[Alice0775 White]

[Tracking Requested - why for this release]: not fixed

No, not fixed.
The problem is still reproduced on the nightly56.0a1[1].

[1]
https://hg.mozilla.org/mozilla-central/rev/018b3829d0a7f6944a5ae1489f3a64c9e2893909
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0 ID:20170706030206

set browser.photon.structure.enabled to false in about:config
Restart Nightly and then follow STR of comment#0

Comment 17

[:Paolo Amadini]

Eh, unfortunately this works on Mac and Linux, but on Windows we don't get the actual main view height until after the panel is shown, even using getBoundingClientRect(). I've tried the "popuppositioned" event but it's still too early.

I tried to use the "popupshown" event, but then the jump between the view states is very visible and occurs every time the panel is opened, so I don't think it's viable.

I wonder how the original code could handle this. Maybe the overflow listener is called after the popup opens and before it is painted, worth checking. Maybe it's just a side effect of looking for the height continuously during the panel opening animation.

Gijs, any other ideas on how to move forward? If the dedicated overflow listener doesn't work, the safest bet might be to back out bug 1009116 in its entirety, although this would reintroduce a number of issues we had in the other panels.

Comment 18

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #17)
> I tried to use the "popupshown" event, but then the jump between the view
> states is very visible and occurs every time the panel is opened, so I don't
> think it's viable.

Is there a jump even when the panel has enough room not to overflow any parts of it?

> I wonder how the original code could handle this. Maybe the overflow
> listener is called after the popup opens and before it is painted, worth
> checking. Maybe it's just a side effect of looking for the height
> continuously during the panel opening animation.

I don't know. The comment in the patch that landed here feels like it's the type of problem that would be solvable with enough modern flexbox. It's not entirely clear to me from the patch why the max-height isn't enough anyway. Potentially relevant: bug 1373968.

Comment 19

[:Paolo Amadini]

(In reply to :Gijs from comment #18)
> Is there a jump even when the panel has enough room not to overflow any
> parts of it?

No, only when it overflows.

> I don't know. The comment in the patch that landed here feels like it's the
> type of problem that would be solvable with enough modern flexbox. It's not
> entirely clear to me from the patch why the max-height isn't enough anyway.

If platform did the right thing when mixing -moz-box and block/flex layout, we wouldn't have this issue...

Comment 20

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #19)
> (In reply to :Gijs from comment #18)
> > Is there a jump even when the panel has enough room not to overflow any
> > parts of it?
> 
> No, only when it overflows.

Actually, there was no magic recipe in the old code, and this jump simply already existed before bug 1009116 landed.

We can just use the "popupshown" event on Windows, and probably we improved the situation for Mac and Linux by using the "popupshowing" event.

Comment 21

[:Paolo Amadini]

Attachment: Bug 1377793 - Fix scrolling in the non-Photon main menu on Windows.

This is a follow-up to the previous patch, required because on Windows we cannot measure the full height of the main view until it is visible.

Review commit: https://reviewboard.mozilla.org/r/155498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155498/

Comment 22

[:Paolo Amadini]

Comment on attachment 8884643 [details]
Bug 1377793 - Fix scrolling in the non-Photon main menu on Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155498/diff/1-2/

Comment 23

[:Gijs (he/him)]

Comment on attachment 8884643 [details]
Bug 1377793 - Fix scrolling in the non-Photon main menu on Windows.

https://reviewboard.mozilla.org/r/155498/#review160658

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33d81aa58bca
Fix scrolling in the non-Photon main menu on Windows. r=Gijs

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33d81aa58bca

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 27

[:Paolo Amadini]

Alice, can you confirm this is now fixed on Windows too? I'll request uplift to Beta once it's verified.

Comment 28

[Alice0775 White]

(In reply to :Paolo Amadini from comment #27)
> Alice, can you confirm this is now fixed on Windows too? I'll request uplift
> to Beta once it's verified.

I confirmed that Nightly56.0a1(2017-07-11) fixed the problem with the photon structure off.

Comment 29

[:Paolo Amadini]

Comment on attachment 8883361 [details]
Bug 1377793 - Fix scrolling in the non-Photon main menu.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1009116
[User impact if declined]: Main application menu cut off on very low resolutions or when the menu button is vertically in the middle of the screen.
[Is this code covered by automated tests?]: There are no tests for this particular issue. The issue does not affect the existing panel tests.
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: Yes. If testing on Nightly, the Photon structure preference should be disabled. Move the window so that the menu button is vertically in the middle of the screen and check that a scrollbar appears in the main application menu. Customize more buttons into the menu if the resolution is too high for this to happen. Should be tested on all platforms separately. See also comment 0.
[List of other uplifts needed for the feature/fix]: There are two patches to uplift in this bug.
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Only applies to the specific case tested here.
[String changes made/needed]: None

Comment 30

[Julien Cristau [:jcristau]]

Comment on attachment 8883361 [details]
Bug 1377793 - Fix scrolling in the non-Photon main menu.

fix for hamburger menu in beta55, should be in 55.0b9

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a598927cab98
https://hg.mozilla.org/releases/mozilla-beta/rev/f06fa691cdfd

Comment 32

[Mihai Boldan, QA [:mboldan]]

I managed to reproduce the issue on Firefox 56.0a1(2017-07-02), under Windows 10x64.
The issue is no longer reproducible on Firefox 55.0b12, or on Firefox 56.0a1 (2017-07-26), under Windows 10x64 or under Ubuntu 16.04x64.

On Firefox 55.0b12, under macOS 10.12.1 the scrollbar is not displayed for the hamburger menu. 

Note that Firefox 56.0a1 (2017-07-26), under macOS, the scrollbar is displayed even if the browser.photon.structure.enabled pref is set to true or false.

Should I reopen this bug for this problem, or it would be better to open a new one?

Comment 33

[:Gijs (he/him)]

(In reply to Mihai Boldan, QA [:mboldan] from comment #32)
> I managed to reproduce the issue on Firefox 56.0a1(2017-07-02), under
> Windows 10x64.
> The issue is no longer reproducible on Firefox 55.0b12, or on Firefox 56.0a1
> (2017-07-26), under Windows 10x64 or under Ubuntu 16.04x64.
> 
> On Firefox 55.0b12, under macOS 10.12.1 the scrollbar is not displayed for
> the hamburger menu. 
> 
> Note that Firefox 56.0a1 (2017-07-26), under macOS, the scrollbar is
> displayed even if the browser.photon.structure.enabled pref is set to true
> or false.
> 
> Should I reopen this bug for this problem, or it would be better to open a
> new one?

Please file a new bug and needinfo Paolo. I expect we may want this to track 55. :-(

Comment 34

[:Gijs (he/him)]

Ni for comment #33.

Comment 35

[Mihai Boldan, QA [:mboldan]]

Logged Bug 1384899 for the issue described in Comment 32.
Since another bug is covering the issue reproducible on macOS, I'm marking this issue Verified Fixed.