Closed Bug 1181554 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect, P1)

Unspecified
All
defect

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox40 --- verified
firefox41 --- verified
firefox42 --- verified

People

(Reporter: dao, Assigned: mstange)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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?
Flags: needinfo?(philipp)
Flags: needinfo?(dbaron)
Attached file 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.
Flags: needinfo?(philipp)
I can't seem to reproduce this on Ubuntu...
Summary: [HiDPI] box-shadow rendered unevenly at uneven scaling factors e.g. (150%, 250%) → [HiDPI] box-shadow rendered unevenly at uneven scaling factors, e.g. 150%, 250%
(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-+.
OS: Windows 10 → All
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.
Flags: needinfo?(dbaron)
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.
Attached patch patch for inset box shadows (obsolete) — Splinter Review
Can you test this patch? I've only fixed up inset box shadows here and it seems to fix the testcase for me.
Attachment #8631337 - Flags: feedback?
Attachment #8631337 - Flags: feedback? → feedback?(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.
Attachment #8631337 - Flags: feedback?(dao) → feedback+
Attachment #8631337 - Flags: review?(jmuizelaar)
Assignee: nobody → mstange
Status: NEW → ASSIGNED
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.
Attachment #8631337 - Flags: review?(jmuizelaar)
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?
Priority: -- → P1
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.
Flags: needinfo?(mstange)
I'm sorry, I completely forgot about this! I'll deal with it first thing tomorrow morning.
Flags: needinfo?(mstange)
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.
Attachment #8631337 - Attachment is obsolete: true
Attachment #8635458 - Flags: review?(jmuizelaar)
Attachment #8635458 - Flags: review?(jmuizelaar) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/250cf47bfa76
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Woot, thanks! Can you do the aurora+beta uplift request too?
Flags: needinfo?(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
Flags: needinfo?(mstange)
Attachment #8635458 - Flags: approval-mozilla-beta?
Attachment #8635458 - Flags: approval-mozilla-aurora?
Verified fixed on latest Nightly, build ID: 20150721030212.
Tested on a MS Pro 2 device running Windows 10 64-bit.
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
Comment on attachment 8635458 [details] [diff] [review]
patch for inset box shadows

Verified fix required for theme changes. Beta+ Aurora+
Attachment #8635458 - Flags: approval-mozilla-beta?
Attachment #8635458 - Flags: approval-mozilla-beta+
Attachment #8635458 - Flags: approval-mozilla-aurora?
Attachment #8635458 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Also verified with:
- latest Aurora, build ID: 20150723004007;
- Firefox 40 beta 7, build ID: 20150720220238.
Depends on: 1434197
You need to log in before you can comment on or make changes to this bug.