Closed Bug 944291 Opened 6 years ago Closed 6 years ago

Cropping of element with css transform + animation applied, starting in Firefox 25

Categories

(Core :: Web Painting, defect)

24 Branch
x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla29
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: gwilym, Assigned: roc)

References

Details

(Keywords: dev-doc-needed, regression, site-compat)

Attachments

(4 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9) AppleWebKit/537.71 (KHTML, like Gecko) Version/7.0 Safari/537.71

Steps to reproduce:

I’ve noticed that an element on my recently launched website is being displayed incorrectly on Firefox 25. If you go to http://dice.gwil.co with Firefox 25 and scroll the red/green buttons you’ll see the odd behaviour.


Actual results:

The speech bubble elements, upon removal of a ‘.hidden’ class, have a CSS transform applied to them in combination with a CSS transition, making them visible.

In Firefox 25, the bubble pops up correctly but once the transition is complete the bubble is cropped so that only a portion of the content is displayed.


Expected results:

In all prior versions of Firefox (tested down as far as 6), the speech bubbles animate and then display as intended, without any cropping, leading me to believe that this might be an issue with Firefox 25.
wfm - 2013-05-25-06-25-25-mozilla-central-firefox-24.0a1.en-US.linux-x86_64 218785bab4bd
bug - 2013-05-26-03-10-46-mozilla-central-firefox-24.0a1.en-US.linux-x86_64 0fed3377c839

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=218785bab4bd&tochange=0fed3377c839

Could you try making a minimized testcase, please?
Keywords: testcase-wanted
OS: Mac OS X → All
Version: 25 Branch → 24 Branch
Flags: needinfo?(gwilym)
I've been stripping down the elements of the page and styling trying to find what could be causing this - the culprit seems to be a CSS animation applied to the cropped element's parent intended to wobble the element with CSS rotation transforms. Removing  the 'wobble' CSS animation or moving the animation to the child element stops the weird cropping behaviour - but spoils the intended effect.

Further strange behaviour: opening the speech bubble element in the Firefox Web Inspector causes the animations and transitions to work as intended.

Here's the minimised version: http://gwil.co/firefox-test.html (sorry if it doesn't seem particularly minimal, but this is a very particular bug)
Flags: needinfo?(gwilym)
Component: Untriaged → Layout
Product: Firefox → Core
Hmm.  On Mac I see it even before the regression range from comment 1, on the testcase of comment 2.

Aleksej, does the 2013-05-25 Linux nightly show the problem for you on the comment 2 testcase?
Flags: needinfo?(deletesoftware+moz)
Yes (and still not with the original site).

With http://gwil.co/firefox-test.html:
w: 2012-07-27-03-05-08-mozilla-central-firefox-17.0a1.en-US.linux-x86_64
…
w: 2012-12-10-03-07-47-mozilla-central-firefox-20.0a1.en-US.linux-x86_64 725eb8792d27
      https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=725eb8792d27&tochange=4dfe323a663d
b: 2012-12-11-03-08-55-mozilla-central-firefox-20.0a1.en-US.linux-x86_64 4dfe323a663d
…
b: 2013-03-20-03-11-03-mozilla-central-firefox-22.0a1.en-US.linux-x86_64 8156df33b757 — the external part splits off
      https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8156df33b757&tochange=a73a2b5c423b
b: 2013-03-21-09-07-06-mozilla-central-firefox-22.0a1.en-US.linux-x86_64 a73a2b5c423b — the external part disappears
b: 2013-05-25-06-25-25-mozilla-central-firefox-24.0a1.en-US.linux-x86_64
b: 2013-12-05-03-02-07-mozilla-central-firefox-28.0a1.ru.linux-x86_64
Flags: needinfo?(deletesoftware+moz)
Hey sorry, just a heads up - the original site will look normal now as I changed the element the wobble effect is applied to. The original setup is still on the second link I provided, though.
Aleksej: Thanks!

Matt has some layers changes in the second range from comment 4.  Nothing in the first range obviously jumps out at me, though there are some layer changes in there too (by roc).
Status: UNCONFIRMED → NEW
Component: Layout → Layout: View Rendering
Ever confirmed: true
Flags: needinfo?(matt.woodrow)
Basically we have an actively-transformed element that contains another element that is zooms in and out with a break in between. When the latter element loses its transform temporarily (becoming an inactive layer), it appears to be drawn clipped. Looks like some kind of FrameLayerBuilder bug for sure.
Assignee: nobody → roc
For some reason, in the bad state, we have computed an incorrect visible rect for the yellow div:
      BackgroundColor 0x7fffdb3fc020(Block(div)(1)) bounds(-3000,-3000,6000,6000) visible(-600,-600,3600,3600) componentAlpha(0,0,0,0) clip()  uniform (opaque -3000,-3000,6000,6000)(rgba 255,255,0,255) layer=0x7fffcac2ddb0
This is actually looking like a layout bug in overflow area calculation:
              Block(div)(1)@0x7fffd09aa738 {12000,0,15000,12000} vis-overflow=-600,-384,15600,12601 scr-overflow=-600,-384,15600,12601 [state=004012a000110200] transformed [content=0x7fffab8842e0] [sc=0x7fffd09a9bf0]<
                line 0x7fffd09aacb8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x300] {0,0,0,0} <
                  Placeholder(div)(1)@0x7fffd09aab00 {0,0,0,0} [state=0000008000200000] [content=0x7fffab884380] [sc=0x7fffd09a86c8:-moz-non-element] outOfFlowFrame=Block(div)(1)@0x7fffd09aaa68
                >
                AbsoluteList 0x7fffd0995d80 <
                  Block(div)(1)@0x7fffd09aaa68 {-3000,-3000,6000,6000} [state=000016e000d10300] [content=0x7fffab884380] [sc=0x7fffd09a9cb0]<

The transform on 0x7fffd09aa738 is the "wobble" transform so the relative-to-self visual overflow of 0x7fffd09aa738 should be nearly the same as -600,-384,15600,12601, which is nowhere near correct.
Flags: needinfo?(matt.woodrow)
Blocks: 938555
This fixes bug 938555 too. There's also a different HTML-only testcase over there.
roc, can you also update the comment documenting mInitial?
Comment on attachment 8346996 [details] [diff] [review]
Part 2: Mark parent frames whose child frames' overflow have changed as always needing UpdateOverflow called

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

f+ since this seems correct to me.

It might be easier to understand this function if we reversed the meaning of mInitial and called it mNeedsUpdateOverflow instead.

We could then comment that since our overflow changed, then we need to update the overflow of our parent. So we make sure our parent both exists in the tree, and is marked appropriately.
Attachment #8346996 - Flags: feedback+
Comment on attachment 8346996 [details] [diff] [review]
Part 2: Mark parent frames whose child frames' overflow have changed as always needing UpdateOverflow called

By setting mInitial false you lose the property that 'parent' will
unconditionally update *its* parent (through the "|| entry->mInitial").

Consider the similar fix (instead of the assignment):
parent->Properties().Delete(nsIFrame::PreTransformOverflowAreasProperty());
which will also bypass the optimization and call UpdateOverflow(),
but does not depend on the return value of that call.

Is this an intentional change?

I suspect there was a reason for the "We always update the parent for
initial frames." as the comment says.
(In reply to Mats Palmgren (:mats) from comment #15)
> I suspect there was a reason for the "We always update the parent for
> initial frames." as the comment says.

I don't remember what it was. Do you, Matt?

I think instead of mInitial we should have a bool mChildrenChanged. That seems to make the code a lot easier to understand.
OK I know what it was now :-).
Attached patch fix v2Splinter Review
I think this code is much easier to understand now :-)
Attachment #8347204 - Flags: review?(matspal)
Attachment #8347204 - Flags: feedback?(matt.woodrow)
Comment on attachment 8347204 [details] [diff] [review]
fix v2

Yes, this is much clearer, thanks.  r=mats


>   void AddFrame(nsIFrame* aFrame) {
>     uint32_t depth = aFrame->GetDepthInFrameTree();
>     if (mEntryList.empty() ||
>-        !mEntryList.contains(Entry(aFrame, depth, true))) {
>-      mEntryList.insert(new Entry(aFrame, depth, true));
>+        !mEntryList.contains(Entry(aFrame, depth))) {

While we're here - remove the redundant "mEntryList.empty() ||".

>+          // We can't tell if the overflow changed, so be conservative

End the sentence with a period.

>+      // If the frame style changed (e.g. positioning offsets)
>+      // then we need to update the parent with the overflow areas of its
>+      // children.

Please reformat this comment; it looks like it should fit on two lines.
Attachment #8347204 - Flags: review?(matspal) → review+
Attachment #8346996 - Attachment is obsolete: true
Attachment #8346996 - Flags: review?(matspal)
Comment on attachment 8347204 [details] [diff] [review]
fix v2

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

Yep, much easier to follow.
Attachment #8347204 - Flags: feedback?(matt.woodrow) → feedback+
Comment on attachment 8346995 [details] [diff] [review]
Part 1: Add SplayTree::find

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

Seems like it might be better to only have this method, and remove contains() (which is identical except that it returns a bool in that final line, not the pointer).  The only user is layout/base/RestyleTracker.h (there's sadly a second SplayTree that accounts for the other SplayTree.h hit in the tree, I need to fix that at some point), so it's trivial to do a few s/contains/find/ and add the preceding ! as needed.
Attachment #8346995 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/e8ba8d037365
https://hg.mozilla.org/mozilla-central/rev/5e992c8ea14f
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment on attachment 8347204 [details] [diff] [review]
fix v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unsure
User impact if declined: Broken rendering on certain Web pages (difficult to predict when it might happen)
Testing completed (on m-c, etc.): Just landed
Risk to taking this patch (and alternatives if risky): a little bit risky but the new code is a lot easier to understand
String or IDL/UUID changes made by this patch: None

Not requesting Beta approval since we've already shipped this in a few releases
Attachment #8347204 - Flags: approval-mozilla-aurora?
Depends on: 944886
Blocks: 944886
No longer depends on: 944886
Summary: Cropping of element with css transform + animation applied in Firefox 25 only → Cropping of element with css transform + animation applied, starting in Firefox 25
Comment on attachment 8347204 [details] [diff] [review]
fix v2

we can take a little risk on Aurora still so let's get this uplifted.
Attachment #8347204 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed using the testcases on 29.0a1 (2014-01-07), win 7 x64.
Status: RESOLVED → VERIFIED
Blocks: 953055
Blocks: 946920
Also fixes bug 953055 and bug 946920 (which are in release and have been for a while I think) if that affects any uplifting decisions.
Verified as fixed on latest Aurora 28.0a2 as well using Ubuntu 12.04 x32 and Windown 8.1 x32
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.