Closed Bug 1338053 Opened 3 years ago Closed 3 years ago

I cannot access some buttons in Hamburger menu. And scrollbar is missing.

Categories

(Core :: Layout, defect, major)

54 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- fixed
firefox54 + fixed

People

(Reporter: alice0775, Assigned: dholbert)

References

Details

(Keywords: regression, ux-control)

Attachments

(2 files)

[Tracking Requested - why for this release]: cannot access some buttons in Hamburger menu

Reproducible: always

Steps To Reproduce:
1. Reduce Browser height
2. Open Hamburger menu

Actual Results:
Hamburger menu is truncated. And scrollbar is missing.
I cannot access some buttons.
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bdc513580a45e6392816a8de4f3da0c8da5e72b0&tochange=b4730a967e2c796fba51576da7d9e91c4ae557c8

Regressed by: Bug 1209697
Component: Menus → Layout
Flags: needinfo?(emilio+bugs)
Product: Firefox → Core
Blocks: 1209697
Flags: needinfo?(dholbert)
Does the patch from bug 1336708 fix this?
(In reply to Timothy Nikkel (:tnikkel) from comment #2)
> Does the patch from bug 1336708 fix this?

bug 1336708 does not fix this issue.

Tested with latest m-c tinderbox build[1].

[1]
https://hg.mozilla.org/mozilla-central/rev/f505911eb333d5ae8c2bf5c44f7b85add6450b53
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0 ID:20170208162021
I've got a patch which I'll post shortly -- stand by.

(The bug doesn't have anything to do with the caching -- it's that our MarkIntrinsicISizesDirty impl needs to invoke the superclass method, which does some XUL-specific invalidation, which apparently matters in this piece of UI since it must use flex + XUL.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: needinfo?(emilio+bugs)
In my defense, I thought about suggesting that we call the superclass method when I reviewed the patch on bug 1209697, but I was thrown by a comment in the superclass method, which says it "should not be called by other derived classes".
https://dxr.mozilla.org/mozilla-central/rev/5e17f9181c6cb0968966280d1c1d96e725702af1/layout/generic/nsFrame.cpp#4428-4431

It seems that in practice, derived classes *do* call this superclass method (nsBlockFrame and nsGridContainerFrame both do, at least).  And it was being called for nsFlexContainerFrame instances as well (by virtue of being the inherited function implementation) up until bug 1209697 added some extra invalidation code.  So it makes sense that we should continue to invoke it, comment or no-comment.
I'll try to come up with a reftest in the morning. (It might be a bit tricky, due to the XUL dependence here -- I suspect we'll need CSS-flexbox within XUL, with dynamic modifications (causing MarkIntrinsicISizesDirty to be dynamically be invoked), and also with overflow:scroll.  So depending on how much trouble it is, I may end up throwing up my hands and just landing the fix, with the understanding that the fix seems trivial & unlikely to regress again & real web content can never actually run into this bug anyway since it seems to require XUL content.)

Also: for any other interested folks who want to try to reproduce this -- you may need to reduce your display's resolution (to e.g. 700px tall or smaller) before your hamburger menu will actually run up against the edge of the screen in a way that triggers this bug.
Flags: needinfo?(dholbert)
OS: Windows 10 → All
Hardware: x86 → All
(In reply to Daniel Holbert [:dholbert] from comment #7)
> I'll try to come up with a reftest in the morning. (It might be a bit
> tricky, due to the XUL dependence here -- I suspect we'll need CSS-flexbox
> within XUL, with dynamic modifications (causing MarkIntrinsicISizesDirty to
> be dynamically be invoked), and also with overflow:scroll.  So depending on
> how much trouble it is, I may end up throwing up my hands and just landing
> the fix, with the understanding that the fix seems trivial & unlikely to
> regress again & real web content can never actually run into this bug anyway
> since it seems to require XUL content.)

Note that this is tested (to some extent) by the browser code (see bug 1336887, which I bet is the same underlying problem). A stand-alone testcase for this would be neat, but in case that it's too complex...
Comment on attachment 8835323 [details]
Bug 1338053: Make nsFlexContainerFrame::MarkIntrinsicISizesDirty() also call its parent class's method.

https://reviewboard.mozilla.org/r/111012/#review112342

Ouch, good catch. I also thought I shouldn't call that (*shrug*).

I bet this also fixes bug 1336887.
Attachment #8835323 - Flags: review?(emilio+bugs) → review+
Tracking 54+ for this visible regression.
Turns out the reftest was easier than I expected. Added that now.

(Ignore MozReview's interdiff here -- it shows a bunch of C++ changes, which aren't actually changes that I made but rather are simply changes from rebasing on top of current tip. The tree that I was using last night in the initial review was a bit old.)
Try run with *just* the new reftest, without the fix (so the test css-flex-1.xul is expected to fail):
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=57273096d26b

Try run with the reftest and the fix (expected to pass):
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=04b9dc9c7375
(setting ni=me to remember to land this when Try completes.)
Flags: needinfo?(dholbert)
Try runs produced the expected results. Landing.
Flags: needinfo?(dholbert)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67bbef772796
Make nsFlexContainerFrame::MarkIntrinsicISizesDirty() also call its parent class's method. r=emilio
(Thank you for the bug report, Alice0775 White!)
https://hg.mozilla.org/mozilla-central/rev/67bbef772796
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8835323 [details]
Bug 1338053: Make nsFlexContainerFrame::MarkIntrinsicISizesDirty() also call its parent class's method.

See the request in bug 1209697 comment 54.
Attachment #8835323 - Flags: approval-mozilla-aurora?
Hi :emilio,
firefox53 is marked as unaffected, why do we need to uplift this to Aurora53?
Flags: needinfo?(emilio+bugs)
If we only uplift the patches in bug 1209697 without this one, Aurora would be affected, similarly for the other related uplift.
Flags: needinfo?(emilio+bugs)
Marking affected since we intend to uplift all this together.
Comment on attachment 8835323 [details]
Bug 1338053: Make nsFlexContainerFrame::MarkIntrinsicISizesDirty() also call its parent class's method.

Let's get this in aurora for performance improvement & fixing some functionality.
Attachment #8835323 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.