Closed
Bug 1338053
Opened 8 years ago
Closed 8 years ago
I cannot access some buttons in Hamburger menu. And scrollbar is missing.
Categories
(Core :: Layout, defect)
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)
386.77 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
[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.
![]() |
Reporter | |
Comment 1•8 years ago
|
||
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
Comment 2•8 years ago
|
||
Does the patch from bug 1336708 fix this?
![]() |
Reporter | |
Comment 3•8 years ago
|
||
(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
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(dholbert)
OS: Windows 10 → All
Hardware: x86 → All
Comment 8•8 years ago
|
||
(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 9•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
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.)
Assignee | ||
Comment 13•8 years ago
|
||
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
Assignee | ||
Comment 14•8 years ago
|
||
(setting ni=me to remember to land this when Try completes.)
Flags: needinfo?(dholbert)
Assignee | ||
Comment 15•8 years ago
|
||
Try runs produced the expected results. Landing.
Flags: needinfo?(dholbert)
Comment 16•8 years ago
|
||
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
Assignee | ||
Comment 17•8 years ago
|
||
(Thank you for the bug report, Alice0775 White!)
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/67bbef772796
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 19•8 years ago
|
||
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?
Comment 20•8 years ago
|
||
Hi :emilio, firefox53 is marked as unaffected, why do we need to uplift this to Aurora53?
Flags: needinfo?(emilio+bugs)
Comment 21•8 years ago
|
||
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)
Comment 22•8 years ago
|
||
Marking affected since we intend to uplift all this together.
Comment 23•8 years ago
|
||
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+
Comment 24•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d5486b5487e2
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•