Closed Bug 1645008 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

79 Branch
Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla79
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox77 --- unaffected
firefox78 --- unaffected
firefox79 --- fixed

People

(Reporter: philipp, Assigned: emilio)

References

(Regression)

Details

(Keywords: regression)

Attachments

(6 files, 3 obsolete files)

Attached image 2020-06-11 12_50_55-Start.png (deleted) —

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.

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

Flags: needinfo?(dao+bmo)
Flags: needinfo?(dao+bmo)

Thanks, I'll poke a bit then.

Flags: needinfo?(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.

Flags: needinfo?(emilio) → needinfo?(madperson)
Flags: needinfo?(emilio)
Attached file about_support.txt (deleted) —

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

Flags: needinfo?(madperson)

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

Summary: Dark line between toolbar and page content once window is maximized → Dark line between toolbar and page content on non-100% DPI for either normal or maximized windows

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

Attached file Reduced test-case. (deleted) —

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.

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

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

Attachment #9156275 - Flags: feedback?(dbaron)

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

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

Attachment #9156275 - Flags: feedback?(dbaron) → feedback-

Other browsers also have this bug.

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

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.

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

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.

Flags: needinfo?(emilio)

(Didn't mean to clear ni?)

Flags: needinfo?(emilio)

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

Assignee: nobody → emilio
Status: NEW → ASSIGNED

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.

Flags: needinfo?(emilio)
Flags: needinfo?(itiel_yn8)
Attached image urlbar bad (deleted) —
Flags: needinfo?(itiel_yn8)
Attached image urlbar good (deleted) —

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.

Flags: needinfo?(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...

Flags: needinfo?(emilio)

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.

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 :)

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dbaron)
Flags: needinfo?(dao+bmo)
Attachment #9156275 - Attachment is obsolete: true

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

Attached image Site identity popup (deleted) —

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

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.

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dbaron)
Flags: needinfo?(dao+bmo)

Fixed by backout.

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79

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.

Attachment #9156509 - Attachment is obsolete: true
Attachment #9156496 - Attachment is obsolete: true
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: