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.

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.

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?

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 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 on attachment 8867428 [details] Bug 1364338: Force a repaint when CSS 'outline-width' or 'outline-offset' change. https://reviewboard.mozilla.org/r/138968/#review142286

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

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

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

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

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

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

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

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

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