1181554 - [HiDPI] box-shadow rendered unevenly at uneven scaling factors, e.g. 150%, 250%

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Dão Gottwald [:dao]]

STR:
- change your OS scaling factor to 150%
- log out or restart if your OS suggests doing so (logging out should be sufficient on Windows)
- log in again, start Firefox, load the reduced testcase that phlsa is going to attach

Actual result: the box-shadow is one device pixel thick on some sides, two devices pixels on others

Expected result: As with e.g. CSS borders, the box-shadow should have the same thickness on all four sides

We've seen this on Windows 10, need to confirm our assumption that this happens on older versions and other OSes too.

dbaron, can you or one of your peers confirm that this is a layout bug?

Comment 1

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Box Shadow Test Case

Here's a very simple test case that illustrates the issue.
I was also able to trigger it using Firefoxes built-in text scaling.

Comment 2

[Dão Gottwald [:dao]]

I can't seem to reproduce this on Ubuntu...

Comment 3

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #2)
> I can't seem to reproduce this on Ubuntu...

That is by changing DPI settings.

I *can* reproduce on Ubuntu by using full zoom in Firefox, i.e. Ctrl-+.

Comment 4

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

I see it on Linux after pressing Ctrl-+ twice.

I agree it would make sense to snap spread to device pixels.  It's not clear to me whether we'd need to do anything for blur to avoid the same problem there, but I suspect not.  It's also not clear that it would need to be done as early in the process as we do it for border, although I don't think it would be harmful.

Comment 5

[Dão Gottwald [:dao]]

Is there a chance that we can get this prioritized and fixed soon? Sounds as if it might be an easy fix? This is affecting Firefox theme changes which are riding trains and are even expected to be uplifted to beta.

Comment 6

[Markus Stange [:mstange]]

Attachment: patch for inset box shadows

Can you test this patch? I've only fixed up inset box shadows here and it seems to fix the testcase for me.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8631337 [details] [diff] [review]
patch for inset box shadows

This fixes it for me. Thanks! I've noticed that when zooming in repeatedly, the border and the box-shadow reach a thickness of 2 device pixels at the same zoom level, as it should be. This patch only takes care of inset box-shadows, outside ones should probably get the same treatment.

Comment 8

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6196e316359d

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8631337 [details] [diff] [review]
patch for inset box shadows

Jeff reviewed this in person and asked me to move all the snapping code closer together in the function. I'll do that and re-request review on the updated patch.

Comment 10

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

Is there a risk that when this is rounding to larger numbers, overflow areas or display item bounds or IsInvisibleInRect might be off, and we end up with glitches after dynamic changes?

Comment 11

[Justin Dolske [:Dolske]]

Any updates here? This is pretty noticeable in primary UI for Windows 10, and we'll want to either uplift this fix to Firefox 40 or find a workaround.

Comment 12

[Markus Stange [:mstange]]

I'm sorry, I completely forgot about this! I'll deal with it first thing tomorrow morning.

Comment 13

[Markus Stange [:mstange]]

Attachment: patch for inset box shadows

This moves a few variables so that most of the rounding code is in the same place. It also adds a comment and removes a misleading call to NS_lround.

Comment 14

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/250cf47bfa76

Comment 15

[Markus Stange [:mstange]]

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy until July 20) from comment #10)
> Is there a risk that when this is rounding to larger numbers, overflow areas
> or display item bounds or IsInvisibleInRect might be off, and we end up with
> glitches after dynamic changes?

I'm only changing inset box shadows here.
The overflow areas / display item bounds aren't affected by an increased spread radius because we're only growing towards the inside of the rect, not towards the outside. The outside is still determined by the padding rect which clips the shadow.
If nsDisplayBoxShadowInner had an implementation of IsInvisibleInRect, we'd have to tweak that, but it doesn't.

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/250cf47bfa76

Comment 17

[Justin Dolske [:Dolske]]

Woot, thanks! Can you do the aurora+beta uplift request too?

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8635458 [details] [diff] [review]
patch for inset box shadows

Sure!

Approval Request Comment
[Feature/regressing bug #]: needed for upcoming Firefox theme changes
[User impact if declined]: uneven borders in the Firefox UI
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: extremely low, small fix in isolated parts of the code
[String/UUID change made/needed]: none

Comment 19

[Cornel Ionce [:noni] [Hubs QA]]

Verified fixed on latest Nightly, build ID: 20150721030212.
Tested on a MS Pro 2 device running Windows 10 64-bit.

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8635458 [details] [diff] [review]
patch for inset box shadows

Verified fix required for theme changes. Beta+ Aurora+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b69bdd81e1cd

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cea8b82865cf

Comment 23

[Cornel Ionce [:noni] [Hubs QA]]

Also verified with:
- latest Aurora, build ID: 20150723004007;
- Firefox 40 beta 7, build ID: 20150720220238.