894931 - web pages broken due to dynamic changes between position:static/relative not changing containing block for position:absolute inside

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Frank Wein [:mcsmurf]]

On trunk (Firefox) the text on a certain website flows out of the usual box and takes up the whole page width instead. In the latest Firefox this was still fine. I'm going to look for the regression range of this (must be a very recent regression).

To reproduce:
1. Go to http://www.nordbayerischer-kurier.de/
2. Observe how the text below the animated pictures/slideshow flows out of the box where the text should be in

Comment 1

[Frank Wein [:mcsmurf]]

This regressed between 5976b9c673f8 and 0888e29c83a3 (yesterday's and today's build), changesets: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5976b9c673f8&tochange=0888e29c83a3

Comment 2

[Frank Wein [:mcsmurf]]

Some further testing: Build from this revision http://hg.mozilla.org/mozilla-central/rev/5f79304fcefe is still fine, this one http://hg.mozilla.org/mozilla-central/rev/fd10ead17ace is broken
=> http://hg.mozilla.org/mozilla-central/rev/fd10ead17ace is to "blame", it's a birch to mozilla-central merge.

Comment 3

[Frank Wein [:mcsmurf]]

Got it: Regression between http://hg.mozilla.org/integration/mozilla-inbound/rev/3d33faf4d81f and http://hg.mozilla.org/integration/mozilla-inbound/rev/e8bf739addfa
=> Bug 868498

Comment 4

[Stephen A Pohl [:spohl] (OOO until 2/27)]

roc, see bug 868498 comment 47. The assertion fix there is causing this regression. Would you happen to know a way to work around this?

Comment 5

[Girish Sharma [:Optimizer]]

So what is visible on http://www.arewefastyet.com/?a=b&view=regress is also this bug or another ?

Comment 6

[Frank Wein [:mcsmurf]]

It's the same bug, I checked with mozilla-inbound builds.

Comment 7

[d.a.]

I believe Huffington Post is also affected by this bug. The "Most Popular" column is rendered on top of the logo, navigaton bar and the top story, making it impossible to click on some sections on the site.

Comment 9

[Frank Wein [:mcsmurf]]

(In reply to d.a. from comment #7)
> I believe Huffington Post is also affected by this bug. The "Most Popular"
> column is rendered on top of the logo, navigaton bar and the top story,
> making it impossible to click on some sections on the site.

You're right, I checked with the two m-i builds. IMHO quite questionable if this patch can stay on trunk/aurora/beta when there is no fast workaround for this.

Comment 10

[Stephen A Pohl [:spohl] (OOO until 2/27)]

Attachment: Patch

Looks like this is a bigger issue than first thought.

roc, I'm setting this patch to r? although you may have a better way to fix this. It turns out that we don't need to move the z-index rule. The position rule is all we need to get the correct behavior when switching scrollbar styles. The position rule seems to only affect overlay scrollbars, so maybe this is safe to do?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 777360 [details] [diff] [review]
Patch

Review of attachment 777360 [details] [diff] [review]:
-----------------------------------------------------------------

This patch isn't correct because it changes the z-ordering of scrollbars.

Comment 12

[Stephen A Pohl [:spohl] (OOO until 2/27)]

Oh, I think I finally understand: you're saying by changing the position we're also changing z-ordering? I missed that.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I know what the bug is. The bug is that we need to reframe when transitioning from position:static to position:relative if there are position:absolute descendants, because they need to change their containing blocks to be the new frame.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can't think of an easy, beta-safe fix right now. I think we're going to have to pull disappearing scrollbars from beta, sorry :-(.

Comment 15

[Stephen A Pohl [:spohl] (OOO until 2/27)]

Attachment: Revert assert fix from bug 868498

Okay, thanks roc! Let's use this bug to revert the assert fix in m-c, aurora and beta. I'll file a separate bug to turn off overlay scrollbars in beta.

Comment 17

[Stephen A Pohl [:spohl] (OOO until 2/27)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f713a828fe36

Comment 18

[Stephen A Pohl [:spohl] (OOO until 2/27)]

Comment on attachment 777478 [details] [diff] [review]
Revert assert fix from bug 868498

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 868498
User impact if declined: Page layout on many websites would be broken.
Testing completed (on m-c, etc.): This is merely a backout of one of the patches in bug 868498. Verified locally that this fixes things.
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 19

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

Comment on attachment 777478 [details] [diff] [review]
Revert assert fix from bug 868498

You don't need approval to back out something that you incorrectly landed this morning.

Comment 20

[Stephen A Pohl [:spohl] (OOO until 2/27)]

Thanks dbaron! Do the tracking flags need to be updated for this to propagate to the right branches? It's getting late here and I won't be able to watch the tree...

Comment 21

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

(And dare I ask why you don't need to back out the *other* patch?)

Comment 22

[Stephen A Pohl [:spohl] (OOO until 2/27)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #21)
> (And dare I ask why you don't need to back out the *other* patch?)

Sure. That's because the patch got lumped in with bug 868498, but it was really fixing bug 870941. I've taken the opportunity to separate the two. I reopened bug 870941 to track progress on the proper fix. Hope that makes sense.

Comment 23

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

(In reply to Stephen Pohl [:spohl] from comment #20)
> Thanks dbaron! Do the tracking flags need to be updated for this to
> propagate to the right branches? It's getting late here and I won't be able
> to watch the tree...

The tracking flags should be updated when it gets backed out to reflect the current state.  But tracking flags don't cause propagation to branches, they just show it.

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/625e6c220d30
https://hg.mozilla.org/releases/mozilla-aurora/rev/668caad64c7a
https://hg.mozilla.org/releases/mozilla-beta/rev/e4f6d58191b9

If this breaks the tree in any way, please back out the other patch from bug 868498.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

BTW, please tell me we have tests for this now? Kinda scary when something can break site layouts as bad as this did without our automated tests catching it...

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: test

Comment 27

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

Comment on attachment 778280 [details] [diff] [review]
test

Maybe use padding-top on the outer div to avoid having to worry about whether margin collapsing makes the test pass in the failure case anyway (given some future margin collapsing changes, at least)?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually padding-top won't work since we need to move the border-box.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think collapsing with the top margin could ever be eliminated, due to compatibility issues, so I'll leave it as-is.

Comment 32

[Stephen A Pohl [:spohl] (OOO until 2/27)]

Test pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be10b6e8dfad

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be10b6e8dfad