990340 - CSS opacity transition broken with parent pseudo :before and overflow auto

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Adam Schwartz]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.152 Safari/537.36

Steps to reproduce:

The following combination of HTML/CSS produces a non-working CSS transition:

http://jsfiddle.net/adamschwartz/h8Nk7/
http://jsfiddle.net/adamschwartz/h8Nk7/show/light/


Actual results:

The opacity value for the inner span changes immediately on hover.


Expected results:

The opacity value should transition.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Presumably we're reframing here for some reason.  I wonder whether the scrollframe confuses things as to whether the ::before should exist or something.

Comment 2

[Mats Palmgren (inactive)]

Attachment: wip

The ReconstructFrame hint is added here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/RestyleManager.cpp?rev=b29352bc495f#2666
It shouldn't be necessary b/c we already have a ::before frame at that
point, nsLayoutUtils::GetBeforeFrame just can't find it.

(This is just a quick hack to demonstrate it works if we fix that
somehow - I'm not suggesting this is the correct fix.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

> nsLayoutUtils::GetBeforeFrame just can't find it

Because it ends up in the scrollbar rathole?

Can you thing of any reason why the ::before frame would not be under the content insertion frame?  Seems to me like this is in fact the right fix, but should be done inside GetBeforeFrame (and GetAfterFrame, I expect).

Comment 4

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Because it ends up in the scrollbar rathole?

Yes.

> Can you thing of any reason why the ::before frame would not be under the
> content insertion frame?

No, let's make that a requirement from now on.

>  Seems to me like this is in fact the right fix,
> but should be done inside GetBeforeFrame (and GetAfterFrame, I expect).

Yes, that's what I had in mind.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Use GetContentInsertionFrame() when searching for ::before/::after frames.

https://tbpl.mozilla.org/?tree=Try&rev=30843d530895

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8409383 [details] [diff] [review]
Use GetContentInsertionFrame() when searching for ::before/::after frames.

What guarantees that the value will actually be < 100 when the timer fires in the test?  Seems like it could fire late enough that the transition completes.

Why can't we just call testbug990340() sync instead of calling it off a timeout?

r=me on the actual code changes, but would like to get the test bit sorted.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Also, am I right that this patch fixes the two bugs this blocks?

Comment 8

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> What guarantees that the value will actually be < 100 when the timer fires
> in the test? Seems like it could fire late enough that the transition
> completes.

30 seconds late?

> Why can't we just call testbug990340() sync instead of calling it off a
> timeout?

OK, no problem.

Comment 9

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #7)
> Also, am I right that this patch fixes the two bugs this blocks?

Yes, AFAICT.

Comment 10

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86119be8c527

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 30 seconds late?

Yes.  We've totally had intermittent oranges on other tests due to timers firing _minutes_ late.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86119be8c527