Last Comment Bug 724502 - UpdateTransformLayer and UpdateOpacityLayer change hints incorrectly merged by style change processing
: UpdateTransformLayer and UpdateOpacityLayer change hints incorrectly merged b...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-06 03:01 PST by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2012-02-08 09:02 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (406 bytes, text/html)
2012-02-06 03:08 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
another test case (628 bytes, text/html)
2012-02-06 13:06 PST, Nick Cameron [:nrc]
no flags Details
fix (7.75 KB, patch)
2012-02-06 19:14 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
correct patch (1.50 KB, patch)
2012-02-06 19:16 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bzbarsky: review+
Details | Diff | Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-06 03:01:25 PST
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) (Exited; email my personal email if necessary) 2012-02-06 03:08:11 PST
Created attachment 594668 [details]
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 [:bz] 2012-02-06 06:20:19 PST
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] 2012-02-06 13:06:27 PST
Created attachment 594778 [details]
another test case

Hover over the yellow div to see the bug at the end of the transition
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-06 19:14:17 PST
Created attachment 594885 [details] [diff] [review]
fix
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-06 19:16:45 PST
Created attachment 594886 [details] [diff] [review]
correct patch

Sorry about that.
Comment 6 Boris Zbarsky [:bz] 2012-02-06 19:22:16 PST
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) (Exited; email my personal email if necessary) 2012-02-06 21:06:11 PST
Thanks. I'll wait for Mats to land his stuff before landing this.
Comment 8 Mats Palmgren (:mats) 2012-02-07 07:43:59 PST
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) (Exited; email my personal email if necessary) 2012-02-07 14:16:23 PST
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) (Exited; email my personal email if necessary) 2012-02-07 15:13:47 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd5cba44fa8e
Comment 11 Ed Morley [:emorley] 2012-02-08 09:02:05 PST
https://hg.mozilla.org/mozilla-central/rev/bd5cba44fa8e

Note You need to log in before you can comment on or make changes to this bug.