Closed Bug 1200585 Opened 9 years ago Closed 5 years ago

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

Categories

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

Product:
Core
Component:
Layout: Positioned
Version:
40 Branch
Platform:
Unspecified
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Project Flags:
Webcompat Priority ?
Tracking Flags:
Tracking Status
firefox40 --- wontfix
firefox41 + wontfix
firefox42 - wontfix
firefox43 - wontfix
firefox-esr31 - wontfix
firefox-esr38 --- wontfix
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: tiger.x.huang, Assigned: emilio, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [webcompat])

Attachments

(5 files, 2 obsolete files)

FirefoxLayoutBug.zip
(deleted), application/zip
Details
jquery.sidr.min.js
(deleted), application/x-javascript
Details
slightly simpler reduced testcase
(deleted), text/html
Details
simple testcase
(deleted), text/html
Details
Another testcase that demonstrates this
(deleted), text/html
Details
Attached file FirefoxLayoutBug.zip (deleted) —
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.)
OS: Unspecified → All
Component: Untriaged → Layout
Product: Firefox → Core
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
Blocks: 1125750
Status: UNCONFIRMED → NEW
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → unaffected
Component: Layout → Untriaged
Ever confirmed: true
Flags: needinfo?(demo99)
Keywords: regressionwindow-wanted → regression
Component: Untriaged → Layout
[Tracking Requested - why for this release]: regression
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
tracking-firefox-esr31: --- → ?
Attached file jquery.sidr.min.js (deleted) —
Attached file testcase.html (reduced testcase) (obsolete) (deleted) —
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.
Attached file slightly simpler reduced testcase (deleted) —
... still includes huge scripts, though
Attachment #8656220 - Attachment is obsolete: true
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
Blocks: 745485
status-firefox-esr31: unaffected → affected
status-firefox-esr38: unaffected → affected
Attached file simple testcase (obsolete) (deleted) —
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.
... 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.
Attached file simple testcase (deleted) —
... 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).
Attachment #8657257 - Attachment is obsolete: true
Summary: Layout problem while moving body element → when RecomputePosition optimization moves an element, we don't move fixed positioned elements whose hypothetical box comes from that element
Blocks: 157681
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.
Component: Layout → Layout: R & A Pos
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)
While this is a valid bug, it does not seem to be a release blocker for 41. Untracking.
tracking-firefox41: ? → -
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.
Flags: needinfo?(dbaron)
Blocks: 1159042
Flags: needinfo?(demo99)
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.)
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.
Of course, we're out of frame state bits. I guess we could share NS_FRAME_HAS_CHILD_WITH_VIEW or something like that.
We have this regression for a while (at least esr 31), I am not going to track it anymore.
status-firefox41: affected → wontfix
status-firefox-esr31: affected → wontfix
status-firefox-esr38: affected → wontfix
tracking-firefox41: - → +
tracking-firefox42: + → -
tracking-firefox43: + → -
Priority: -- → P3
Whiteboard: [webcompat]
I wrote this as a retained-dl reftest, fails because the inner position:fixed div doesn't move until we reflow.
Frame state bit 45 is unused currently too :)
Dan, maybe this is something you can look at?
Flags: needinfo?(dglastonbury)
I will take a look into it. Thanks.
Flags: needinfo?(dglastonbury)
Assignee: nobody → dglastonbury
Status: NEW → ASSIGNED
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.
(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?
Depends on: 1456358
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.)
Unassigning myself from this bug as I'm focusing on webrender at the moment.
Assignee: dglastonbury → nobody
Status: ASSIGNED → NEW

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

Webcompat Priority: --- → ?

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

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Assignee: nobody → emilio

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.

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

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?

status-firefox69: --- → wontfix
status-firefox70: --- → affected
Flags: needinfo?(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.

Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(emilio) → needinfo?(dbaron)
Resolution: --- → FIXED

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.

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.

QA Whiteboard: [qa-70b-p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: