Closed Bug 1187851 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect)

39 Branch
x86_64
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 --- affected
firefox42 --- affected
firefox47 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: sven.kasten, Assigned: dbaron)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached file 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.
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Severity: normal → major
At least this also doesn't work for the current Nightly 42.0a1 (2015-07-27)
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=509dbdfe3323&tochange=fd64e2d0cbee

triggered by:  bug 1125767
Blocks: 1125767
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
Product: Firefox → Core
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 :-)
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.
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.)
(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 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.
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.
Summary: JS-executed css-transition breaks "position: fixed" of an element → dynamic changes to 'filter' (and 'perspective') don't change whether element is a position:fixed containing block for existing descendants
Assignee: nobody → dbaron
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.
Attachment #8641914 - Flags: review?(roc)
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.
Attachment #8641914 - Flags: review?(roc)
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!
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 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
Attachment #8641914 - Flags: approval-mozilla-aurora?
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.
Attachment #8641914 - Flags: approval-mozilla-aurora?
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
Keywords: leave-open
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
Bug 1227766, plus some fancier change-hint generation, plus use of will-change, might help avoid this problem.
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)
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
https://hg.mozilla.org/mozilla-central/rev/c88f281bf19a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1281068
https://hg.mozilla.org/integration/mozilla-inbound/rev/12e50efb59fc7f3853bbee3a51aa46b9f078d0f5
Bug 1187851 followup - Fix DEBUG-only code to print name of nsChangeHint_UpdateContainingBlock.  No review.
No longer depends on: 1281068
Depends on: 1516550
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: