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

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[gwilym]

Attachment: Screen Shot 2013-11-28 at 09.32.48.png

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.

Comment 1

[[:Aleksej]]

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?

Comment 2

[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)

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[[:Aleksej]]

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

Comment 5

[gwilym]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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).

Comment 7

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

Attachment: Somewhat minimized testcase

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.

Comment 8

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

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

Comment 9

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

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.

Comment 10

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

Attachment: Part 1: Add SplayTree::find

Comment 11

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

Attachment: Part 2: Mark parent frames whose child frames' overflow have changed as always needing UpdateOverflow called

Comment 12

[Jonathan Watt [:jwatt]]

This fixes bug 938555 too. There's also a different HTML-only testcase over there.

Comment 13

[Jonathan Watt [:jwatt]]

roc, can you also update the comment documenting mInitial?

Comment 14

[Matt Woodrow (:mattwoodrow)]

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.

Comment 15

[Mats Palmgren (inactive)]

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.

Comment 16

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

(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.

Comment 17

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

OK I know what it was now :-).

Comment 18

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

Attachment: fix v2

I think this code is much easier to understand now :-)

Comment 19

[Mats Palmgren (inactive)]

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.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8347204 [details] [diff] [review]
fix v2

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

Yep, much easier to follow.

Comment 21

[Jeff Walden [:Waldo]]

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.

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ba8d037365
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e992c8ea14f

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e8ba8d037365
https://hg.mozilla.org/mozilla-central/rev/5e992c8ea14f

Comment 24

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

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

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8347204 [details] [diff] [review]
fix v2

we can take a little risk on Aurora still so let's get this uplifted.

Comment 26

[Paul Silaghi, QA [:pauly]]

Verified fixed using the testcases on 29.0a1 (2014-01-07), win 7 x64.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1c16411289e4
https://hg.mozilla.org/releases/mozilla-aurora/rev/89680c49db3b

Comment 28

[Timothy Nikkel (:tnikkel)]

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.

Comment 29

[Bogdan Maris, Desktop QA]

Verified as fixed on latest Aurora 28.0a2 as well using Ubuntu 12.04 x32 and Windown 8.1 x32