724502 - UpdateTransformLayer and UpdateOpacityLayer change hints incorrectly merged by style change processing

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

ReResolveStyleContext assumes, in general, that when we generate a style change list entry for a frame with a given hint, there is no need to generate style change list entries for descendant frames with the same hint.

This is wrong for UpdateTransformLayer and UpdateOpacityLayer and I think it explains a bug Nick Cameron showed me the other day.

Comment 1

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

Attachment: testcase

Load the testcase. After you see that the lime DIV has faded a bit, click away to another window to force the page to be fully repainted. You'll see the yellow DIV suddenly snap to its correct rendering.

Comment 2

[Boris Zbarsky [:bzbarsky]]

We have two options here.

Option 1 is to add a clause to the code at the beginning of ReResolveStyleContext that handles this situation for the reflow flags to remove the layer flags from aMinChange.

Option 2 is to change processing of the layer hints to walk all the kids (like the repaint hint does, say).

I'm guessing #1 is better?

Comment 3

[Nick Cameron [:nrc]]

Attachment: another test case

Hover over the yellow div to see the bug at the end of the transition

Comment 4

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

Attachment: fix

Comment 5

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

Attachment: correct patch

Sorry about that.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 594886 [details] [diff] [review]
correct patch

Add a comment about how those don't get automatically applied to descendants, and r=me

Comment 7

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

Thanks. I'll wait for Mats to land his stuff before landing this.

Comment 8

[Mats Palmgren (inactive)]

FYI, I landed my stuff directly on mozilla-central so make sure it is merged first
if you want to land on mozilla-inbound.

Also, is there any situation where a child has opacity:inherit and the rule nodes
are shared so that nsStyleDisplay::CalcDifference isn't called, like we saw in
bug 723669.... Actually, never mind, I see that nsStyleDisplay has:
  static bool ForceCompare() { return true; }
so that covers both UpdateTransformLayer and UpdateOpacityLayer since they are only
used by nsStyleDisplay::CalcDifference.  Can we document this dependency somehow?

Comment 9

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

It is documented in nsStyleStruct.h:
   Structs that can return a hint that doesn't guarantee that the
// change will be applied to all descendants must return true from
// ForceCompare(), so that we will make sure to compare those structs in
// nsStyleContext::CalcStyleDifference.

I'll document it in nsChangeHint.h as well.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd5cba44fa8e

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bd5cba44fa8e