1364338 - Negative CSS outline not rendering correctly when changed with JavaScript

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Louis Lazaris]

Attachment: outlinebug-rtc.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce:

- Define a visible outline on an element
- Set the outline-offset to a negative value
- Change the value via JavaScript


Actual results:

- The outline does not change correctly (sometimes not at all).
- The outline eventually settles its position at the border edge (the default position), or just inside the border edge (even if it is set to a larger negative value)


Expected results:

The outline should adjust to match the value of the outline-offset property being dynamically changed by JavaScript.

Comment 1

[Louis Lazaris]

I should also point out that scrolling the element off the page, then back into view will sometimes cause the outline to be repositioned/repainted in a different place. This is why my test case has extra vertical scrolling included.

Comment 2

[Loic]

It worked in the past:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=09f4968d5f42&tochange=9696d1c4b3ba

Daniel Holbert — Bug 1131371: Only update overflow region (& trigger DLBI) when CSS "outline" changes, instead of triggering a reflow. r=heycam 

Daniel, any thoughts?

Comment 3

[Daniel Holbert [:dholbert]]

Looks like a legit regression.  We probably need to send nsChangeHint_RepaintFrame for "overflow" changes to handle this case, in addition to the change hints used in Bug 1131371.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1364338: Force a repaint when CSS 'outline-width' or 'outline-offset' change.

Previously, when these properties changed, we'd only send change hints to
recompute overflow areas & trigger DLBI. If the outline was always outside of
the element's border box, this old strategy was generally OK, because the
outline tweak would cause a change to the overflow areas' size, and that would
invalidate the changed area via DLBI & trigger a repaint.

However, for outlines that are *inside* of the element (via negative
'outline-offset'), these change hints were not sufficient, because tweaks to
the outline width & offset will NOT affect the size of the element's overflow
areas and will not trigger any DLBI invalidation.

So in order to correctly handle these changes, we really need to request a
repaint of the affected element, since some piece of the element may need to be
repainted even if it's not changing in size.

Review commit: https://reviewboard.mozilla.org/r/138968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138968/

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8867428 [details]
Bug 1364338: Force a repaint when CSS 'outline-width' or 'outline-offset' change.

https://reviewboard.mozilla.org/r/138966/#review142284

::: layout/style/nsStyleStruct.cpp:574
(Diff revision 1)
>      return nsChangeHint_UpdateOverflow |
> -           nsChangeHint_SchedulePaint;
> +           nsChangeHint_SchedulePaint |
> +           nsChangeHint_RepaintFrame;

Note that this combined change hint will now be consistent (in substance & ordering) with what we return for some "box-shadow" comparisons, e.g. here:

https://dxr.mozilla.org/mozilla-central/rev/1e2fe13035e13b7b4001ade3b48f226957cef5fc/layout/style/nsStyleStruct.cpp#4417-4424

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8867428 [details]
Bug 1364338: Force a repaint when CSS 'outline-width' or 'outline-offset' change.

https://reviewboard.mozilla.org/r/138968/#review142286

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9efa29ce0e3e
Force a repaint when CSS 'outline-width' or 'outline-offset' change. r=heycam

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9efa29ce0e3e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Can this ride the 55 train or should we consider it for backport?

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Can this ride the 55 train or should we consider it for backport?

This would be pretty safe to uplift, but I'd default to letting this ride the normal release trains, given that we've been shipping this bug since Firefox 38 without anyone apparently noticing, and because this bug is only visible if a site combines negative "outline-offset" AND dynamic changes to that outline-offset [or to outline-width] -- and I'd guess that each of those two things are uncommon on their own, and their combination might be vanishingly uncommon.

Louis: you discovered & filed this bug -- do you know of any real-world sites that are impacted by this? That'd help inform the decision about whether it'd be worth taking a small risk to get this uplifted to our current beta so that it ships faster. (And thank you for filing, BTW!)

Comment 11

[Julien Cristau [:jcristau]]

Sounds like an edge case, wontfix for esr52.  We can reconsider if this affects web compat much.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Daniel Holbert [:dholbert] (reduced availability - travel & post-PTO backlog) from comment #10)
> This would be pretty safe to uplift, but I'd default to letting this ride
> the normal release trains, given that we've been shipping this bug since
> Firefox 38 without anyone apparently noticing, and because this bug is only
> visible if a site combines negative "outline-offset" AND dynamic changes to
> that outline-offset [or to outline-width] -- and I'd guess that each of
> those two things are uncommon on their own, and their combination might be
> vanishingly uncommon.

outline-offset is used by ~13% of page loads, according to Chrome's popularity counter, outline-width by ~10%, and outline by ~39%:

https://www.chromestatus.com/metrics/css/popularity

Dynamic changes to those properties are...much less common, maybe 0.1% between outline-offset and outline-width together:

https://www.chromestatus.com/metrics/css/animated

That's not that much, but still feels like it would be worth uplifting--we certainly keep web apis around if they're used that much.  WDYT?

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd [:froydnj] from comment #12)
> outline-offset is used by ~13% of page loads, according to Chrome's
> popularity counter

Note that in comment 10, I was talking about *negative* outline-offset values being rare (which I don't have any data to back me up on, admittedly).

Anyway, this seems like an extremely safe fix, so we might as well try for an uplift.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8867428 [details]
Bug 1364338: Force a repaint when CSS 'outline-width' or 'outline-offset' change.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1131371
[User impact if declined]: Rendering glitches (failure to repaint the outline), if a site uses a negative "outline-offset" and then makes dynamic changes to that property or to "outline-width".
[Is this code covered by automated tests?]: Yes. Regression test included for this bug as well.
[Has the fix been verified in Nightly?]: Yes. (I just verified it myself using the attached testcase & latest Nightly vs. Beta.)
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a very small targeted change, which simply makes us invalidate slightly more when these CSS properties change.
[String changes made/needed]: None.

Comment 15

[Daniel Holbert [:dholbert]]

(Marking VERIFIED for Nightly 55 -- just tested original testcase on Nightly 55.0a1 (2017-05-23) (64-bit), and dragged the slider around, and observed that the outline repainted correctly.)

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8867428 [details]
Bug 1364338: Force a repaint when CSS 'outline-width' or 'outline-offset' change.

Fix a regression and include tests. Beta54+. Should be in 54 beta 11.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a2261759caf

Comment 18

[Daniel Holbert [:dholbert]]

Thanks Gerry & Ryan!

I'll cancel the needinfo=reporter from comment 10, since it's not needed for uplift-decision-purposes anymore.

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Daniel Holbert [:dholbert] (reduced availability - travel & post-PTO backlog) from comment #14)
> [Is this code covered by automated tests?]: Yes. Regression test included
> for this bug as well.
> [Has the fix been verified in Nightly?]: Yes. (I just verified it myself
> using the attached testcase & latest Nightly vs. Beta.)
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Daniel's assessment on manual testing needs.