This is a recent regression, perhaps from bug 719177.

The error really is already in bug 524925. It's the documentElement's frame that is notified of the nsChangeHint_UpdateOverflow and no other... So, apart from the fact that nsTextFrame::UpdateOverflow() is missing, we really need to traverse all frame descendants too, right? I'm leaning towards backing out the whole lot from Aurora. :-(

That would fix the testcase at hand... still needs work though...

(In reply to Mats Palmgren [:mats] from comment #3) > The error really is already in bug 524925. It's the documentElement's frame > that is > notified of the nsChangeHint_UpdateOverflow and no other... > > So, apart from the fact that nsTextFrame::UpdateOverflow() is missing, > we really need to traverse all frame descendants too, right? Hmm. Maybe the thing to do is to have nsFrameManager::ReResolveStyleContext generate an entry in aChangeList for every frame that gets nsChangeHint_UpdateOverflow. That should work since ReResolveStyleContext does get called for every nsIContent whose style changes. (Separately, we might want to do something so that frames that will get marked dirty for reflow don't also have to do UpdateOverflow.) > I'm leaning towards backing out the whole lot from Aurora. :-( I sure don't want to have to do that!

It seems like using UpdateOverflow hints for text-shadow doesn't match what UpdateOverflow hints were intended to do -- the intent is pretty clearly documented in nsChangeHint.h -- the frame's effect on its *ancestors* overflow areas has changed. It's not intended to handle cases where a frame's own overflow area changes. Why can't we just back *that* out? That said, there may be a real problem if there's ancestor/descendant coalescing where the style system assumes that doing an X changehint on an ancestor implies doing it for descendants -- I don't remember if we assume that or not.

I don't see how that could possibly be the intent since the original case in bug 524925, transforms, definitely affects a frame's *own* overflow areas. All properties we use the UpdateOverflow hint for affects its own overflow areas, so we should just change the documentation.

Transforms are a tricky case since in a sense they only affect the frame's overflow area in its parent's coordinate space, but we also want to use UpdateOverflow for 'outline' changes which definitely affect the frame's own overflow area. So I agree we should change the comment in nsChangeHint to reflect that. (In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #7) > That said, there may be a real problem if there's ancestor/descendant > coalescing where the style system assumes that doing an X changehint on an > ancestor implies doing it for descendants -- I don't remember if we assume > that or not. We do by default. Basically ReResolveStyleContext adds the change-hint for its frame to aMinChange and passes that aMinChange when calling ReResolveStyleContext on children. CaptureChange only adds to the style change list if the calculated difference isn't already present in aMinChange. At the start of nsFrameManager::ReResolveStyleContext, we clear out some bits from aMinChange for same cases where new style change list entries will need to be added. I think we should unconditionally clear out UpdateOverflow there too.

If you want to fix the comment, you also need to fix the code in nsCSSFrameConstructor::ProcessRestyledFrames to call UpdateOverflow on the target frame in addition to its ancestors. This also means you'll need to support UpdateOverflow on leaf frame types; the initial implementation only required it for containers, which simplified the task substantially, I think.

(In reply to Mats Palmgren [:mats] from comment #8) > I don't see how that could possibly be the intent since the original case in > bug 524925, transforms, definitely affects a frame's *own* overflow areas. > All properties we use the UpdateOverflow hint for affects its own overflow > areas, so we should just change the documentation. Also, the comment could probably be improved to express the subtlety that we redo the computation starting with the *pre-transform* overflow area on the element. But redoing from any point earlier than that does require implementing UpdateOverflow correctly for all leaf frame classes.

To be clear, I think we should move forward by doing the following ASAP: * backing out bug 719177 (or at least the parts of it that don't meet the existing invariants) * fixing the last paragraph of comment 9 and adding a test for it (with nested transforms) * improving the comment in nsChangeHint.h per comment 11 Then in the future we can perhaps move forward with additional use of UpdateOverflow hints, but more carefully.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9) > At the start of nsFrameManager::ReResolveStyleContext, we clear out some > bits from aMinChange for same cases where new style change list entries will > need to be added. I think we should unconditionally clear out UpdateOverflow > there too. That didn't work because CalcStyleDifference doesn't even call nsStyleText::CalcDifference in this case because it has the same rule node: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp#414 Something like this might work instead: leave UpdateOverflow in aMinChange and have CaptureChange() propagate that bit to CalcStyleDifference() to signal we want an explicit compare for style structs that can result in that hint. If the result contains UpdateOverflow, there is a change and an entry is appended to aChangeList.

Comment on attachment 594463 [details] [diff] [review] append descendants with UpdateOverflow to change list Note: for inherited-by-default properties like text-shadow, the the CalcStyleDifference is very likely to result in UpdateOverflow so we will likely add all descendants to aChangeList... I think traversing the frame descendants is better for that case, so perhaps we should have two separate UpdateOverflow hints?

(In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #10) > If you want to fix the comment, you also need to fix the code in > nsCSSFrameConstructor::ProcessRestyledFrames to call UpdateOverflow on the > target frame in addition to its ancestors. This also means you'll need to > support UpdateOverflow on leaf frame types; the initial implementation only > required it for containers, which simplified the task substantially, I think. What leaf frames require nontrivial handling of UpdateOverflow? nsTextFrame and nsTextBoxFrame. Off the top of my head I can't think of any others, but then I only just got out of bed... Most leaf frames don't have their own overflow other than what FinishAndStoreOverflow gives them. (In reply to Mats Palmgren [:mats] from comment #13) > That didn't work because CalcStyleDifference doesn't even call > nsStyleText::CalcDifference in this case because it has the same rule node: > http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext. > cpp#414 So, because nsStyleText is an inheriting style struct we are implicitly requiring that every hint returned by nsStyleText::CalcDifference must automatically update all descendant frames? If so, let's document that somewhere, say nsChangeHint.h and nsStyleStruct.h. (In reply to Mats Palmgren [:mats] from comment #14) > Note: for inherited-by-default properties like text-shadow, the > the CalcStyleDifference is very likely to result in UpdateOverflow > so we will likely add all descendants to aChangeList... > I think traversing the frame descendants is better for that case, > so perhaps we should have two separate UpdateOverflow hints? Yeah, if I'm right in the previous comment then we should have a different UpdateOverflow hint that updates all descendants and can be used in inherited structs. Or possibly we should not bother and just use reflow; maybe it's not worth optimizing changes to these few properties. (In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #12) > * backing out bug 719177 (or at least the parts of it that don't meet the > existing invariants) > * fixing the last paragraph of comment 9 and adding a test for it (with > nested transforms) Yes, we need to do this.

> if I'm right in the previous comment then we should have a different UpdateOverflow > hint that updates all descendants and can be used in inherited structs. Fwiw, I implemented that with a flag to UpdateOverflow() to make it recurse descendants, so the UpdateOverflow hint for inherited-by-default properties would only have one aChangeList entry that traverse the frame tree, and not-inherited properties would create aChangeList entries for *changed* descendants and not recurse into descendants in UpdateOverflow(). Seems to work fine. > Or possibly we should not bother and just use reflow; maybe it's not worth optimizing > changes to these few properties. Yeah, maybe not, it's just text-shadow. > Yes, we need to do this. I've filed bug 724432 to handle the mechanics of the backout.

WFM, I landed a reftest: https://hg.mozilla.org/integration/mozilla-inbound/rev/e0ecec88ed3d