1200585 - when RecomputePosition optimization moves an element, we don't move fixed positioned elements whose hypothetical box comes from that element

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect, P3)

Description

[Tiger Huang]

Attachment: FirefoxLayoutBug.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.157 Safari/537.36

Steps to reproduce:

0. Unzip the file attached.
1. Open res/branches.html in Firefox.
2. Click on the button at the left-top corner (the button is composed by three horizontal white lines).


Actual results:

The top blue tool bar is covered by the left-side menu, so that the user has no chance to hide the left-side menu.


Expected results:

The top blue tool bar is pushed by the left-side menu.
(You can do the steps above in Chrome to see the expected behavior.)

Comment 1

[Loic]

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c5b90c003be8&tochange=993eb76a8bd6

Comment 2

[Alice0775 White]

Pudhlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=69e80cc95763&tochange=f124f6d08ec7

Suspect:  319d452a7049	Ethan Lin — Bug 1125750 - Check the overflow region direction to avoid unnecesary reflow for scrollable frame. r=dbaron

Comment 3

[Alice0775 White]

[Tracking Requested - why for this release]: regression

Comment 4

[Loic]

Attachment: jquery.sidr.min.js

Comment 5

[Loic]

Attachment: testcase.html (reduced testcase)

Comment 6

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

Thanks for the test case, Tiger!

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's possible the regression is really related to etlin's patch, but it's also possible that it's an existing bug that the patch's optimization exposed.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: slightly simpler reduced testcase

... still includes huge scripts, though

Comment 9

[Alice0775 White]

Pushlog with attachment 8656949 [details]:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d624c548c68f&tochange=2e81a5d9952d

Comment 10

[Alice0775 White]

Via local build,
Last Good : 13fb1897c93c
First Bad : 046d7a70ef24

Regressed by: 046d7a70ef24	Robert O'Callahan — Bug 745485. Optimize positioning offset changes whenever the computed size does not change. r=dholbert

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: simple testcase

I wrote this after poking at what the previous testcase does in DOM inspector and trying to write a simple version of it.

I think the underlying bug here is that doing RecomputePosition on an absolutely positioned element A doesn't reposition a fixed-positioned element F when F's position is based on a hypothetical box affected by A's position.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... and if that's the correct analysis, then bug 745485 was just exposing in more cases a bug that really dates back to bug 157681.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: simple testcase

... this time with both fixed width and height on the body, which I think is mostly likely to give the correct regression range (which should be bug 157681).

Comment 14

[Alice0775 White]

Pushlog with attachment 8657258 [details]:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f5a441d6929f&tochange=df6702c41ddd

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A few thoughts here:

1. this applies to both position:absolute and position:relative moving, when they establish a hypothetical box for fixed positioned elements

2. it seems possible that fixed positioned elements combined with hypothetical boxes have similar bugs in other cases as well.  (Consider, e.g., position:sticky.)

3. The thing that makes this work in the reflow codepath is that nsAbsoluteContainingBlock::FrameDependsOnContainer always returns true when we use the hypothetical box (see the first return in the function), which makes nsAbsoluteContainingBlock::Reflow reflow all frames that depend on the hypothetical box.

4. (3) implies that this also doesn't work when reflow roots are involved (if that's ever possible) 

5. As far as fixing, we could take the approach of either (a) trying to bail from the optimization if there are fixed pos elements whose hypothetical box depends on an abs pos or rel pos frame's position or (b) trying to reflow such frames correctly when we do make the optimization.  It's worth considering (2) and (4), though.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

6. What makes this even more interesting is that the fixed positioned elements might not even be attached to the viewport -- they might be on some intermediate frame that establishes a fixed-position containing block (i.e., has transform, perspective, or filters; see nsStyleDisplay::IsFixedPosContainingBlock)

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

While this is a valid bug, it does not seem to be a release blocker for 41. Untracking.

Comment 18

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

David, are you taking this bug or can you help find an owner for it? I'm happy to take a patch at least up to aurora.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and more complicated:

7.  This can happen transitively in interesting ways.  In the simple case, a relatively or absolutely positioned block A could be the containing block for a static positioned block B which establishes the hypothetical box for a fixed positioned element F whose containing block is an ancestor of A.  But there are more interesting cases, like a relatively positioned inline or block A establishing the containing block for an absolutely positioned block B which establishes the hypothetical box for a fixed positioned element F whose containing block is an ancestor of A.  (And these can chain more deeply.)

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the best idea I can think of so far is that if we encounter a fixed-positioned element that depends on its hypothetical box, we set a frame state bit from that fixed-positioned element's placeholder (or its containing block) all the way up the frame tree to the root that says that we can't use the RecomputePosition optimization.

Seems like a bit of a waste of a frame state bit for an obscure case, but I tend to think we probably need to do it, given that I'm worried that additional optimizations (e.g., bug 1159042) will expose this bug in more cases.


I guess I can probably take this bug, although I won't get to it for a few days.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Of course, we're out of frame state bits.  I guess we could share NS_FRAME_HAS_CHILD_WITH_VIEW or something like that.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

We have this regression for a while (at least esr 31), I am not going to track it anymore.

Comment 26

[Matt Woodrow (:mattwoodrow)]

Attachment: Another testcase that demonstrates this

I wrote this as a retained-dl reftest, fails because the inner position:fixed div doesn't move until we reflow.

Comment 27

[Matt Woodrow (:mattwoodrow)]

Frame state bit 45 is unused currently too :)

Comment 28

[Cameron McCormack (:heycam)]

Dan, maybe this is something you can look at?

Comment 29

[u480271]

I will take a look into it.  Thanks.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd note that what's hard about this bug isn't necessarily fixing a particular testcase, but reasoning about what the full set of related problems is, and making sure that we've fixed them all.

Comment 31

[u480271]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #30)
> I'd note that what's hard about this bug isn't necessarily fixing a
> particular testcase, but reasoning about what the full set of related
> problems is, and making sure that we've fixed them all.

David, are you suggesting this isn't a first good bug?

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I guess doing what I suggested in comment 20 is probably a reasonable first bug.  It's probably worth seeing if you can poke holes in it (to find cases that it doesn't cover), though.  (It's probably worth starting by reading the sections of CSS 2.1 chapters 9 and 10 about relative, absolute, and fixed positioning.)

Comment 34

[u480271]

Unassigning myself from this bug as I'm focusing on webrender at the moment.

Comment 36

[Firefox Bug Husbandry Bot]

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Comment 37

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating whiteboard priority tags to program flags.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, we should probably have another bug for reflow roots and for contain... and maybe there are other things in the above comments that can cause similar problems.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The original problem reported here should now be fixed by bug 1456358.

Comment 41

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

Are you intending to work on this for 70 or 71? I notice it's set to P3 so am a bit worried it will be in perpetual backlog.´Maybe it's also worth filing new bugs for the separate issues mentioned above?

Comment 42

[Emilio Cobos Álvarez (:emilio)]

This was fixed by bug 1456358. David, do you know if there's a test-case for contain or such causing trouble in this space? If so we should open a separate bug for that.

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I don't think contain is a problem, since the contain cases we optimize into reflow roots have contain: layout, which establishes a containing block for absolutely positioned and fixed positioned elements.

I need to look into dynamic reflow roots more carefully, though.

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Right now we don't allow frames to be dynamic reflow roots unless they're a fixed-pos containing block. This makes the dynamic reflow root feature not very useful -- but I think means for now we don't have a problem.

So I should think about whether we would have a problem if we didn't have that limit, and if so, we should add more comments to that chunk of code, and maybe also tests.