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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alice0775 White]

Attachment: screenshot, Nightly54.0a1 .VS. Aurora53.0a2

[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.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bdc513580a45e6392816a8de4f3da0c8da5e72b0&tochange=b4730a967e2c796fba51576da7d9e91c4ae557c8

Regressed by: Bug 1209697

Comment 2

[Timothy Nikkel (:tnikkel)]

Does the patch from bug 1336708 fix this?

Comment 3

[Alice0775 White]

(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

Comment 4

[Daniel Holbert [:dholbert]]

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.)

Comment 5

[Daniel Holbert [:dholbert]]

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 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1338053: Make nsFlexContainerFrame::MarkIntrinsicISizesDirty() also call its parent class's method.

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

Comment 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(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

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 10

[Marcia Knous [:marcia]]

Tracking 54+ for this visible regression.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8835323 [details]
Bug 1338053: Make nsFlexContainerFrame::MarkIntrinsicISizesDirty() also call its parent class's method.

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

Comment 12

[Daniel Holbert [:dholbert]]

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.)

Comment 13

[Daniel Holbert [:dholbert]]

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

Comment 14

[Daniel Holbert [:dholbert]]

(setting ni=me to remember to land this when Try completes.)

Comment 15

[Daniel Holbert [:dholbert]]

Try runs produced the expected results. Landing.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67bbef772796
Make nsFlexContainerFrame::MarkIntrinsicISizesDirty() also call its parent class's method. r=emilio

Comment 17

[Daniel Holbert [:dholbert]]

(Thank you for the bug report, Alice0775 White!)

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67bbef772796

Comment 19

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 20

[Gerry Chang [:gchang]]

Hi :emilio,
firefox53 is marked as unaffected, why do we need to uplift this to Aurora53?

Comment 21

[Emilio Cobos Álvarez (:emilio)]

If we only uplift the patches in bug 1209697 without this one, Aurora would be affected, similarly for the other related uplift.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking affected since we intend to uplift all this together.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d5486b5487e2