Closed Bug 1377793 Opened 4 years ago Closed 4 years ago

Hamburger menu dropdown is truncated, no scrollbar

Categories

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

55 Branch
Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 + verified
firefox56 - verified

People

(Reporter: alice0775, Assigned: Paolo)

References

Details

(Keywords: regression)

Attachments

(3 files)

[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
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
Blocks: 1009116
Flags: needinfo?(paolo.mozmail)
And I can also reproduce the problem on Nightly56.0a1(2017-07-02) if set browser.photon.structure.enabled to false
Whiteboard: [photon-structure] [triage]
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).
Whiteboard: [photon-structure] [triage]
I'll take a look, this is more likely a result of CSS changes than of the way we do the resizing.
tracking for 55 per comment 4.
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.
Flags: needinfo?(paolo.mozmail)
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: -- → P1
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.
Attachment #8883361 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cb18ada2641
Fix scrolling in the non-Photon main menu. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/4cb18ada2641
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Hi Alice,
Can you help check if this is fixed in latest nightly?
Flags: needinfo?(alice0775)
[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
Status: RESOLVED → REOPENED
Flags: needinfo?(alice0775)
Resolution: FIXED → ---
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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(paolo.mozmail)
(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...
Flags: needinfo?(paolo.mozmail)
(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.
Attachment #8883361 - Attachment is obsolete: true
Attachment #8883361 - Attachment is obsolete: false
Comment on attachment 8884643 [details]
Bug 1377793 - Fix scrolling in the non-Photon main menu on Windows.

https://reviewboard.mozilla.org/r/155498/#review160658
Attachment #8884643 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33d81aa58bca
Fix scrolling in the non-Photon main menu on Windows. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/33d81aa58bca
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Please request Beta approval on this when you get a chance.
Flags: needinfo?(paolo.mozmail)
Alice, can you confirm this is now fixed on Windows too? I'll request uplift to Beta once it's verified.
Flags: needinfo?(alice0775)
(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.
Flags: needinfo?(alice0775)
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
Flags: needinfo?(paolo.mozmail)
Attachment #8883361 - Flags: approval-mozilla-beta?
Attachment #8884643 - Flags: approval-mozilla-beta?
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
Attachment #8883361 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8884643 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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?
Flags: needinfo?(gijskruitbosch+bugs)
(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. :-(
Flags: needinfo?(gijskruitbosch+bugs)
Ni for comment #33.
Flags: needinfo?(mihai.boldan)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(mihai.boldan)
Depends on: 1384899
You need to log in before you can comment on or make changes to this bug.