Closed Bug 990340 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

28 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: adam.flynn.schwartz, Assigned: mats)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
Presumably we're reframing here for some reason.  I wonder whether the scrollframe confuses things as to whether the ::before should exist or something.
Attached patch wip (obsolete) — Splinter Review
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.)
Assignee: nobody → matspal
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
> 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).
(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.
Attachment #8401512 - Attachment is obsolete: true
Summary: CSS opacity transition broken with parent psuedo :before and overflow auto → CSS opacity transition broken with parent pseudo :before and overflow auto
Blocks: 999643
Blocks: 913588
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.
Attachment #8409383 - Flags: review?(bzbarsky) → review+
Also, am I right that this patch fixes the two bugs this blocks?
Flags: needinfo?(matspal)
(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.
Flags: needinfo?(matspal)
(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.
> 30 seconds late?

Yes.  We've totally had intermittent oranges on other tests due to timers firing _minutes_ late.
https://hg.mozilla.org/mozilla-central/rev/86119be8c527
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.