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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

(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.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1 (wip)

Here's a WIP fix, basically cribbing from bug 1131371's patch.

Comment 2

[Daniel Holbert [:dholbert]]

(Note to self: we can test this by extending this mochitest:
http://mxr.mozilla.org/mozilla-central/source/layout/style/test/test_dynamic_change_causing_reflow.html?force=1
...and we should make sure this doesn't break bug 723669's testcase (which mats already checked in as a reftest).

Comment 3

[baxxabit]

Hi! Daniel, what info do you need?

Comment 4

[Daniel Holbert [: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.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v2

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.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v3

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.

Comment 7

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95d29bfec4c7

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8652728 [details] [diff] [review]
fix v3

Please ask a Style System peer for review instead.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 10

[Daniel Holbert [: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.)

Comment 11

[Daniel Holbert [:dholbert]]

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).

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: fix v4

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:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8d882b2828e8/try-macosx64-debug/try_snowleopard-debug_test-mochitest-browser-chrome-2-bm107-tests1-macosx-build2407.txt.gz

...from this Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d882b2828e8

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...)

Comment 13

[Daniel Holbert [:dholbert]]

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.)

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: testcase that triggers fatal assertion about overflow & non-display SVG

With this testcase, I can reproduce the Try assertion-failure discussed in comment 12, using a linux debug build with the current patch applied.

Comment 15

[Daniel Holbert [: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.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: fix v5

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.

Comment 17

[Daniel Holbert [:dholbert]]

Try run with patches for this bug & bug 1032613:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=45efc9bb0324

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this to keep an eye on it for 43.

Comment 19

[Daniel Holbert [:dholbert]]

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.)

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8664407 [details] [diff] [review]
fix v5

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

Looks good.

Comment 21

[Daniel Holbert [:dholbert]]

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.)

Comment 22

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ab03d382684

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/009f9286101b

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/009f9286101b

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 26

[Daniel Holbert [: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.)

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

That sounds good to me! Thanks.