1645008 - Dark line between toolbar and page content on non-100% DPI for either normal or maximized windows

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[[:philipp]]

Attachment: 2020-06-11 12_50_55-Start.png

I started seeing a dark 1px line under the firefox toolbar once the browser window gets maximised. this is happening for me under windows 10 on a hidpi monitor (2560x1440px, with windows scaling set at recommended 125% - when i set scaling to 100% or 150% the issue doesn't occur).

mozregression is pointing to this changeset as the regressor: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5c88e66a73e6d8353ea8d20881b603b598d47dfe&tochange=5df17ecbcaa13a0f5bb09d4b197d58804eb10112

the problem is very reminiscent of bug 1543152 which went away on its own at one point.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Dao, do you know if we have any workaround for bug 477157 in the tree that may be causing something like this?

Comment 2

[Dão Gottwald [:dao]]

We likely have a few workarounds but not for this particular border afaik: https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/browser/themes/shared/browser.inc.css#47-49

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Thanks, I'll poke a bit then.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

I can't repro in any of the relevant scalings, though this is on Linux + WR... I'll try to repro on Windows tomorrow I suppose.

Are you using WebRender or layers out of curiosity? This is weird because the border has the right size, it's just the wrong color, somehow.

Comment 5

[[:philipp]]

Attachment: about_support.txt

yes, i have webrender on - i'll attach my about:support dump

Comment 6

[:Gijs (he/him)]

Funny, I see this on win10 with a local build and with nightly, but on 150% dpi and only for "normal" windows, not maximized ones...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to :Gijs (he/him) from comment #6)

Funny, I see this on win10 with a local build and with nightly, but on 150% dpi and only for "normal" windows, not maximized ones...

Also webrender?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case.

So this is not a magic line, it's the background of the toolbox.

I see the same background bleeding through in this test-case on my windows machine, and also on Chrome Canary.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

And the reason this happens is because there's a similar border here, which makes everything fractional.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1645008 - Only floor borders to device pixels if there are borders in the opposite side.

This doesn't match other browsers, but I think this is better behavior.

This needs a couple reftest adjustments if we decide to go for this.

Otherwise I'll look into tweaking the front-end.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9156275 [details]
Bug 1645008 - Only floor borders to device pixels if there are borders in the opposite side.

David, any thoughts on this? I think something like this is sensible, but I'd rather check with you before spending time fixing reftests (there aren't too many in any case).

Comment 12

[Itiel]

I can see a similar issue on the urlbar and searchbar, fwiw.

Comment 13

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

(In reply to :Gijs (he/him) from comment #6)

Funny, I see this on win10 with a local build and with nightly, but on 150% dpi and only for "normal" windows, not maximized ones...

Also webrender?

If "Compositing: WebRender" in about:support indicates this, then yes.

Comment 14

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

Comment on attachment 9156275 [details]
Bug 1645008 - Only floor borders to device pixels if there are borders in the opposite side.

I don't think this makes sense. Consistency of border widths matters not just on multiple sides of the same box, but across a sequence of similar items (for example, if all of the items in a list have border-top: 1px). So I don't think this is a good idea.

Why do we have this bug and other browsers not?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Other browsers also have this bug.

Comment 16

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

(BTW, I'm also still a bit curious about the answers to my questions in my first paragraph in the initial patch's review.)

Comment 17

[Emilio Cobos Álvarez (:emilio)]

But yeah, I agree with you, comment 14 is a very good point I should've known about, d'oh. I'll fix the front-end then.

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 from comment #16)

(BTW, I'm also still a bit curious about the answers to my questions in my first paragraph in the initial patch's review.)

I apparently had not submitted them, so I'll do here instead:

I guess I'm not particularly convinced about this being a better behavior, but it seems worth doing because it's a more compatible behavior, so I'm ok with it -- at least assuming you agree that it's a more compatibile behavior because there are real compatibility issues that you know of that it will fix. (That's the case, right?)

Yes, multiple (see dupes of that bug).

If you go back to the initial comment in the bug, I think this change will increase reports of the complaint there, about slivers between things, rather than reducing it, since we'll now observe slivers on the inside edges of borders as well, against the background or border of a child.

Right, it's fundamentally a trade-off between showing slivers between borders of the background and the children, or borders behaving differently layout-wise from everything else plus showing slivers between borders and the background of the parent.

Comment 18

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

Right, it's fundamentally a trade-off between showing slivers between borders of the background and the children, or borders behaving differently layout-wise from everything else plus showing slivers between borders and the background of the parent.

I don't follow the later case about slivers between borders and the background of the parent. I would expect slivers against a background of an unrelated element that the developer has manually positioned to be the same size by doing themselves the math that CSS does. But I don't see why, if we do the rounding at style computation time, and do layout correctly, we should end up with slivers in the case of an element's border against its own background. (If we were having that, it sounds like a bug in what we were attempting to do, rather than a fundamental flaw in the idea.)

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Well, attachment 360825 [details] is I guess more the layout one. But the result is showing slivers with the parent because the way we round the border means that the elements don't fill all the space as the author expects.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

(Didn't mean to clear ni?)

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1645008 - Refactor content / chrome separator to avoid the background bleeding through it. r=dao,gijs

Use a border top + background on the #browser hbox instead of a border-bottom in the toolbox.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Itiel, can you post an screenshot of what you see in comment 12? I don't see anything like that here in all resolutions I've tried so I want to make sure I'm looking for the same thing.

Comment 23

[Itiel]

Attachment: urlbar bad

Comment 24

[Itiel]

Attachment: urlbar good

Comment 25

[Itiel]

I've confirmed with moz-regression that that bug is indeed the regressor:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7a6a822be3b4508223a476ea1ab3940a4541db1c&tochange=f8faae6114e00d12dcfbb3ed07b6fbd89c400944

I'm on Windows 10, 1 monitor, 1920x1080, DPI 150%, Nightly is maximized.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

I see, so I wasn't seeing it because it only seems to happen with the dark theme, but I can repro as well...

Here's a test-case for that:

<!doctype html>
<div style="height: 10px; border: 1px solid black; background-color: black; background-clip: padding-box"></div>

You shouldn't see white stripes between the border and the background, but both on Firefox Nightly and Chrome Canary, you do.

This is because of this rule, effectively.

This is a bit unfortunate, I think... But, I think that gave me an idea for how to easily fix both in the front-end, at least for these to where they're 1px borders...

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1645008 - Make some borders one device pixel, to ensure they're not fractional. r=dao,gijs

Note that this means that in HiDPI displays these borders will still be
1 device pixel, rather than one CSS pixel, so this technically is a
minor change for such displays.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Dao, Gijs: It's your call whether you want to take this, or we can otherwise back out my change instead and WONTFIX bug 477157.

I think my mind has changed a bit specially given the padding-box use-case, it's quite unfortunate that such a naive example shows slivers.

The behavior implemented in bug 477157 is more compatible with other browsers, but I want your opinion about which behavior is better as a developer, if the behavior that Gecko had, or the new behavior.

David, if you have thoughts on the matter on top of what you expressed above also let me know, I'm a bit half-and-half about the two approaches, if you think it's better to revert my change in bug 477157 let me know, I'm also willing to accept that :)

Comment 29

[Itiel]

I can see the same issue as in comment 23 and comment 24 in the protections popup and the site identity popup...

Comment 30

[Itiel]

Attachment: Site identity popup

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Yeah, same padding-box issue, and thus would have the same fix.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

So I did a bit more digging, and the Blink behavior is pre-fork, but WebKit changed to match us in https://bugs.webkit.org/show_bug.cgi?id=156121.

Given that and that David was right (unsurprisingly...), I think we should back out the regressing patch.

I filed https://github.com/w3c/csswg-drafts/issues/5210 about defining some of this at least.

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Fixed by backout.

Comment 34

[Phabricator Automation]

Comment on attachment 9156509 [details]
Bug 1645008 - Make some borders one device pixel, to ensure they're not fractional. r=dao,gijs

Revision D79602 was moved to bug 1647356. Setting attachment 9156509 [details] to obsolete.