1187851 - dynamic changes to 'filter' (and 'perspective') don't change whether element is a position:fixed containing block for existing descendants

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[sven.kasten]

Attachment: testcase.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/600.7.12 (KHTML, like Gecko) Version/8.0.7 Safari/600.7.12

Steps to reproduce:

Since Firefox 39 (on OS X) the footer-div of the attached "testcase.html" doesn't attach to the bottom anymore when scrolling the page.

The problem seems to be the "animatecss"-function. If commented out in the test case, the page loads as expected with the footer at the bottom attached, even when scrolling down.

The curious behavior is: when the animatecss is activated and the footer isn't attached when scrolling, it can be correct when opening up Firebug and deactivate and activate back again the "position"-option on the #footer.

Problem began since update to 39.0. With 38.0.x it worked. Also all other browsers run this script without problems.




Actual results:

The element with id #footer doesn't get attached to the bottom (position: fixed; bottom: 0;) when scrolling down. It is initial loaded correctly but when scrolling down, it scrolls to the top.


Expected results:

The position: fixed; should work on the element with id #footer and also attach the element to the footer even when scrolling. The behavior works right, when removing the animatecss-function from the test-case (directly executing .css and .removeclass). It fails when the animatecss-function is active.

Comment 1

[sven.kasten]

At least this also doesn't work for the current Nightly 42.0a1 (2015-07-27)

Comment 2

[Alice0775 White]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=509dbdfe3323&tochange=fd64e2d0cbee

triggered by:  bug 1125767

Comment 3

[sven.kasten]

Because we use parts of the code of the 'testcase.html' in production web-portals, I have to know when this bug can be fixed? Else we may have to change our code, but I think this should not be the right solution :-)

Comment 4

[Timothy Nikkel (:tnikkel)]

Bug 1125767 caused this bug because it made filter induce a containing block for fixed pos elements. So if you have a fixed pos element that is a child of a filtered element it becomes absolute position relative to the filtered element.

This was because the w3c decided that this is how it should work according to spec, see https://bugzilla.mozilla.org/show_bug.cgi?id=1125767#c5

The fact that it works in other browsers means that they haven't been updated to this new spec yet.

Comment 5

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

I'd think that anything that affects whether nsStylePosition::IsContainerForAbsAndFixedPosDescendants returns true needs to cause either an nsChangeHint_ReconstructFrame hint or an nsChangeHint_AddOrRemoveTransform hint.

https://hg.mozilla.org/mozilla-central/rev/b9951cca6d1f doesn't maintain that invariant for filter, and it was previously broken for perspective as well.

(I didn't actually debug anything; I'm just guessing from looking at the testcase.)

Comment 6

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

(In reply to Timothy Nikkel (:tn) from comment #4)
> Bug 1125767 caused this bug because it made filter induce a containing block
> for fixed pos elements. So if you have a fixed pos element that is a child
> of a filtered element it becomes absolute position relative to the filtered
> element.

But I believe the test removes the filter style after transitioning it.

Comment 7

[sven.kasten]

Attachment: testcase_half_workaround.html

Comment 8

[sven.kasten]

Comment on attachment 8641575 [details]
testcase_half_workaround.html

This isn't directly the animation we wan't but at least at works a bit in FF.
The problem is that it mostly works, the footer sticks to the bottom after loading, but when you do something like a "brute force" of "refresh" + scrolling (Mac: scroll while repeatedly cmd+r) sometimes the footer holds the "absolute" position. You can then scroll and the footer sticks to the page like before.

Comment 9

[sven.kasten]

Okay, after running the "testcase.html" in Chrome and Safari they seem to have implemented it like the spec says, and when the filter is removed, like David Baron said, they assign the "position: fixed;" again. You can see it, if you open the testcase in chrome/safari and do a "cmd+r"+scrolling fast again and again.


(In reply to Timothy Nikkel (:tn) from comment #4)
> Bug 1125767 caused this bug because it made filter induce a containing block
> for fixed pos elements. So if you have a fixed pos element that is a child
> of a filtered element it becomes absolute position relative to the filtered
> element.
> 
> This was because the w3c decided that this is how it should work according
> to spec, see https://bugzilla.mozilla.org/show_bug.cgi?id=1125767#c5
> 
> The fact that it works in other browsers means that they haven't been
> updated to this new spec yet.

Comment 10

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

Attachment: MozReview Request: Bug 1187851 - Make dynamic changes to filter and perspective change fixed position containing block for descendants. r?roc

Bug 1187851 - Make dynamic changes to filter and perspective change fixed position containing block for descendants.  r?roc

Note that this now uses AddAndRemoveTransform hints for changes that are
other than adding and removing a transform.  Since there's still a
little bit of transform-related stuff there too (which I did make
conditional), I figure it's probably best to leave the name as-is,
although I'd be open to renaming it as well.

As expected, without the patch, the filter and perspective tests fail,
but the added transform test passes.  All the tests pass locally with
the patch.

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=471820f56cf0

Comment 12

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

Comment on attachment 8641914 [details]
MozReview Request: Bug 1187851 - Make dynamic changes to filter and perspective change fixed position containing block for descendants.  r?roc

https://reviewboard.mozilla.org/r/14659/#review13241

::: layout/style/nsStyleStruct.cpp:1310
(Diff revision 1)
> +    NS_UpdateHint(hint, nsChangeHint_AddOrRemoveTransform);

Yes, I do think we should change the name to nsChangeHint_UpdateContainingBlock or something like that.

Comment 13

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

Comment on attachment 8641914 [details]
MozReview Request: Bug 1187851 - Make dynamic changes to filter and perspective change fixed position containing block for descendants.  r?roc

https://reviewboard.mozilla.org/r/14659/#review13243

Ship It!

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f24dbdeeaef1
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dcb38c7f1b8

Comment 15

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

It's too late to fix this for 40 given that 40 is shipping in just over a week.  And given that we didn't hear about it for 5 weeks in 39, we can probably have it shipping in release for another 7.

I'll request approval for 41, though.

Comment 16

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

Comment on attachment 8641914 [details]
MozReview Request: Bug 1187851 - Make dynamic changes to filter and perspective change fixed position containing block for descendants.  r?roc

Approval Request Comment
[Feature/regressing bug #]: bug 1125767
[User impact if declined]: incorrectly positioned elements due to use of wrong containing block
[Describe test coverage new/current, TreeHerder]: added reftests
[Risks and why]:  relatively low risk; it applies change handling that we already use for 'transform' changes to changes of 'perspective' and 'filter', which have the same containing block effects as 'transform'
[String/UUID change made/needed]: no

Comment 17

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

Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a48ff1155af
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4d2b60cd412
because two gaia integration tests that count the number of reflows started failing, and I'm reasonably confident it was this bug and not the other bug in my push.

This means that gaia is depending on this bug to not cause frame reconstruction and reflow in some cases, which is bad.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/144378d2a7ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb53182d1bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/48d5d4b398c2

Comment 19

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

I relanded most but not all of the patch (after splitting it up differently).  What remains is the part that actually enables the fix:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/25bb621735f0/filter-perspective-dynamic-fixed-pos-enable

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/144378d2a7ce
https://hg.mozilla.org/mozilla-central/rev/7bb53182d1bc
https://hg.mozilla.org/mozilla-central/rev/48d5d4b398c2

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7911162321be
https://hg.mozilla.org/integration/mozilla-inbound/rev/c515743c6a7d

Comment 22

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

And I actually realized that I could land the perspective changes too, based on:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f0f1b77e812
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab930e4b2231
so all that's left to land is now:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c3df7102509d/filter-dynamic-fixed-pos-enable

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7911162321be
https://hg.mozilla.org/mozilla-central/rev/c515743c6a7d

Comment 24

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

And before I lose the data, the gaia test failures were:

In Gij(19) on B2G Desktop and Mulet:
 AssertionError: we got 1 reflows instead of 0
 AssertionError: we got 1 reflows instead of 0
 AssertionError: we got 1 reflows instead of 0
 AssertionError: we got 1 reflows instead of 0
 AssertionError: we got 1 reflows instead of 0
 AssertionError: we got 1 reflows instead of 0 
 TEST-UNEXPECTED-FAIL | apps/system/test/marionette/edges_gesture_test.js | Edges gesture > Swiping between apps left to right, no reflow
 AssertionError: we got 1 reflows instead of 0 

In Gij(20) on B2G Desktop and Mulet:
 AssertionError: we got 3 reflows instead of 2
 AssertionError: we got 3 reflows instead of 2
 AssertionError: we got 3 reflows instead of 2
 AssertionError: we got 3 reflows instead of 2
 AssertionError: we got 3 reflows instead of 2
 AssertionError: we got 3 reflows instead of 2 
 TEST-UNEXPECTED-FAIL | apps/system/test/marionette/homescreen_navigation_test.js | Homescreen navigation > Going to the homescreen and back to a warm app
 AssertionError: we got 3 reflows instead of 2

Comment 25

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

Bug 1227766, plus some fancier change-hint generation, plus use of will-change, might help avoid this problem.

Comment 26

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

So now that Firefox OS isn't Tier-1 any more, I'm going to reland the final patch here and actually fix the bug.

It's worth a note here about what the regression is, though.  The gaia test that was being regressed was a gaia test testing performance characteristics of the homescreen.  Gaia was depending on the presence of this bug to achieve those performance characteristics.

If we wanted to get those performance characteristics back in gaia, we could, however:

 * fix bug 1251075 (which I just filed as a followup)

 * make the appropriate gaia code use 'will-change: filter', which will cause a containing block all the time rather than just some of the time (though this might not even be needed depending on what other properties it currently uses)

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c88f281bf19ae0a9b115a7a127e62758555e02f3
Bug 1187851 patch 6 - Make dynamic changes to filter change fixed position containing block for descendants.  r=roc

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd84d2a8fe7c488456b52d65087f993de5d262bc
Bug 1187851 followup - Use versioned specification link to make CSSWG test system happy.  No review.

Comment 30

[Carsten Book [:Tomcat]]

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

Comment 31

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e50efb59fc7f3853bbee3a51aa46b9f078d0f5
Bug 1187851 followup - Fix DEBUG-only code to print name of nsChangeHint_UpdateContainingBlock.  No review.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12e50efb59fc