Closed Bug 1194480 Opened 5 years ago Closed 5 years ago

Changes to "box-shadow" & "text-shadow" should just cause an update to overflow areas, instead of a full reflow


(Core :: Layout, defect)

Not set



Tracking Status
firefox43 + wontfix
firefox44 + fixed


(Reporter: dholbert, Assigned: dholbert)


(Depends on 1 open bug, Blocks 1 open bug)



(2 files, 4 obsolete files)

(This is the same as bug 1131371, but for 'box-shadow' / 'text-shadow'

Right now, the function we use to handle box-shadow / text-shadow differences, CalcShadowDifference(), returns NS_STYLE_HINT_REFLOW if there are any differences.

We *should* be able to just update the overflow areas and be done with it -- I don't think we need to reflow.

We tried to make this change a few years ago in bug 719177, but that bug was backed out in bug 724432. (because the original changes caused bug 723669.)

Filing this bug on trying again.

(Note that box-shadow-triggered reflows are causing an author-reported perf issue, bug 1194155. Marking this as blocking this bug, as well as the general "use nsChangeHint_UpdateOverflow more" bug.)
See Also: → 1131371
Attached patch fix v1 (wip) (obsolete) — Splinter Review
Here's a WIP fix, basically cribbing from bug 1131371's patch.
(Note to self: we can test this by extending this mochitest:
...and we should make sure this doesn't break bug 723669's testcase (which mats already checked in as a reftest).
Flags: needinfo?(dholbert)
Hi! Daniel, what info do you need?
Assignee: nobody → dholbert
(In reply to baxxabit from comment #3)
> Hi! Daniel, what info do you need?

(None specifically -- I had just set needinfo=myself as a reminder to circle back to this & finish it off [or figure out if there's some reason we can't finish it off yet], after getting through a backlog of code reviews. Taking a closer look now.)
Blocks: 1198607
Attached patch fix v2 (obsolete) — Splinter Review
This change:
 (1) ...refactors CalcShadowDifference to be just an equality function, now called "AreShadowArraysEqual". (This doesn't lose any information, because it already only could return two possible values. There's an XXX comment about returning more specific values; I think [?] the rest of this patch covers that.)

 (2) ...adjusts the callsite for mBoxShadow (in nsStyleBorder::CalcDifference) to return an "UpdateOverflow"/"SchedulePaint" changehint (just like for changes to 'outline'). Note that we don't immediately return -- we wait to see if we also get a nsChangeHint_BorderStyleNoneChange changehint, which we can't afford to skip because nsChangeHint.h says it may be required to trigger reflows on children. (if I'm understanding correctly)

  (3) ...adjusts the callsite for mTextShadow to preserve existing behavior there. (I tried to fix that one similarly to mBoxShadow, but that causes bug 1198607, so I'm punting mTextShadow to that bug.)
Flags: needinfo?(dholbert)
Attachment #8652725 - Flags: review?(mats)
Attachment #8647775 - Attachment is obsolete: true
Summary: Changes to "box-shadow" & "text-shadow" should just cause an update to overflow areas, instead of a full reflow → Changes to "box-shadow" should just cause an update to overflow areas, instead of a full reflow
Attached patch fix v3 (obsolete) — Splinter Review
Er, 2 minor updates:
 - I'm no longer adjusting nsStyleText's MaxDifference method -- that was a stale modification, from an earlier patch version which tried to handle text-shadow.
 - I rewrote nsStyleBorder's MaxDifference method to list the flags in a more logical order (matching the CalcDifference implementation more closely in terms of order), and to drop my earlier ptach's partial-NS_CombineHint usage there and just use "|" instead.
Attachment #8652725 - Attachment is obsolete: true
Attachment #8652725 - Flags: review?(mats)
Attachment #8652728 - Flags: review?(mats)
Comment on attachment 8652728 [details] [diff] [review]
fix v3

Please ask a Style System peer for review instead.
Attachment #8652728 - Flags: review?(mats)
(In reply to Daniel Holbert [:dholbert] (vacation 8/27-9/13) from comment #5)
>   (3) ...adjusts the callsite for mTextShadow to preserve existing behavior
> there. (I tried to fix that one similarly to mBoxShadow, but that causes bug
> 1198607, so I'm punting mTextShadow to that bug.)

See my comments there; I think when done correctly that problem will not happen.
Flags: needinfo?(dholbert)
Thanks - as you suggested, UpdateSubtreeOverflow does indeed seem to work for nsStyleText, without causing assertions. (and without breaking text-shadow rendering) 

I'll post an updated patch (after figuring out the reftest orange from my Try run) later today.

(This reftest orange -- in input/input-customerror.html -- does look like a box-shadow rendering issue, so it's a real test failure.  But I don't think it failed on Try runs with earlier iterations of this patch, so it's likely not a fundamental problem; hopefully just a typo somewhere.)
Ah, that reftest failure is a case where *only the color* of the box-shadow changes, and that doesn't end up producing a repaint with my current patch. (The overflow area doesn't change, so DLBI doesn't invalidate anything, so nothing is repainted.)

So we just need to send nsChangeHint_RepaintFrame as well, to handle color-changes-without-overflow-region-changes (which is what we do for e.g. 'outline-color' changes).
Flags: needinfo?(dholbert)
Depends on: 1198894
Attached patch fix v4 (obsolete) — Splinter Review
This is nearly there -- this addresses comment 11, and fixes 'text-overflow' again per comments 9 and 10.

Now I'm getting this assertion when handling restyles, though, at least on Mac OS X:
13:18:31     INFO -  Assertion failure: FrameMaintainsOverflow(this) (Non-display SVG do not maintain visual overflow rects), at layout/generic/nsFrame.cpp:5523

Sample log:

...from this Try run:

I'm not yet sure what the frame in question is, or what change was just made to it -- it's not clear from the backtrace. (Seems likely it's a change to box-shadow or text-shadow, on a non-display SVG element or on its ancestor...)
Attachment #8652728 - Attachment is obsolete: true
It looks like the assertion-failure in comment 12 is Mac-specific (or at least, the Try linux boxes didn't hit it), so I won't be able to debug it right away -- I'll take a closer look when I'm back from vacation.  In the meantime, feedback on the patch is welcome.

(I tried to see if I could trigger the assertion locally [with the attached patch] by tweaking "box-shadow" and "text-shadow" on a <feImage> element -- which is a non-display SVG frame. This didn't cause any assertions, though.)
Flags: needinfo?(dholbert)
With this testcase, I can reproduce the Try assertion-failure discussed in comment 12, using a linux debug build with the current patch applied.
Flags: needinfo?(dholbert)
That testcase actually triggers the same assertion-failure if I tweak 'text-decoration' instead of 'text-shadow'.  This lead me to realize that the assertion-failure from comment 12 is actually an instance of an *existing* bug with nsChangeHint_UpdateSubtreeOverflow, and of course Jesse already found & filed it: bug 1032613.  My patch here just gives us a new way to hit that existing bug (using text-shadow), and it turns out some Mac browser-chrome tests do end up tripping over that bug it via this new route.

So: we need to fix bug 1032613 before this can land.
Depends on: 1032613
Blocks: 1206469
Attached patch fix v5Splinter Review
I think this is good to go. This latest patch-revision is just unbitrotted to apply to current inbound.  (The assertions noted above are gone, when this is combined with my fix for bug 1032613.)

Recapping the strategy of the patch here:
 - Simplify CalcShadowDifference to an equals function, to just detect changes.
 - When 'box-shadow' or 'text-shadow' change, only update overflow regions & repaint; don't ask for a reflow.
 - In some of the code that I'm changing, I'm replacing a few NS_CombineHint calls with an equivalent "|" (bitwise-or), since NS_CombineHint only accepts two values, and nesting NS_CombineHint calls to combine more than 2 hints ends up pretty messy.
Attachment #8653070 - Attachment is obsolete: true
Attachment #8664407 - Flags: review?(dbaron)
Tracking this to keep an eye on it for 43.
Comment on attachment 8664407 [details] [diff] [review]
fix v5

Shifting review to heycam (who recently reviewed related bug 1209952), in the interests of lightening dbaron's review-load & getting this fixed sooner.

(dbaron, feel free to steal back; and heycam, feel free to punt back to dbaron if you'd rather he review this.)
Attachment #8664407 - Flags: review?(dbaron) → review?(cam)
Comment on attachment 8664407 [details] [diff] [review]
fix v5

Review of attachment 8664407 [details] [diff] [review]:

Looks good.
Attachment #8664407 - Flags: review?(cam) → review+
Thanks for the review!

(This can't land quite yet; it's waiting on bug 1032613, since this causes an assertion-failure in several of our tests, without that bug's fix.)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Daniel, is this something you're looking to uplift to aurora along with the test fix in bug 1032613? They both seem to affect 43.  Has this issue been around for a while, or is it a recent regression? If we can let it ride the trains that is best, but we're not too late in aurora if you want to give it a try.
Flags: needinfo?(dholbert)
This bug is something that's been around since forever -- not a regression -- so I'd prefer to just let it ride the trains.

(This also depends on bug 1198894, which [like this bug here] *should* have no visible rendering effects, but could conceivably cause subtle breakage and/or expose an bug that was formerly hidden.  So, more baking time is good.)
Flags: needinfo?(dholbert)
That sounds good to me! Thanks.
Blocks: 1239619
Depends on: 1302571
Summary: Changes to "box-shadow" should just cause an update to overflow areas, instead of a full reflow → Changes to "box-shadow" & "text-shadow" should just cause an update to overflow areas, instead of a full reflow
You need to log in before you can comment on or make changes to this bug.