Closed Bug 1162441 Opened 9 years ago Closed 8 years ago

[HiDPI] Separator between toolbars and content is sometimes too thick at 125% scale

Categories

(Firefox :: Theme, defect, P3)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox39 --- unaffected
firefox40 --- affected
firefox51 --- fixed

People

(Reporter: aryx, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

screenshot
13.71 KB, image/png
Details
patch
6.54 KB, patch
jaws
: review+
Details | Diff | Splinter Review
Attached image screenshot — 
Firefox Nightly 20150506 on Windows 8.1

The line which separates the navbox from the content has twice the size it has on Aurora. Inspecting the ::after pseudo element with browser toolbox says the height is 1 px, but measuring it in the screenshot shows 2px.
Are you using a hidpi screen and/or windows setup?
Flags: needinfo?(archaeopteryx)
The Windows 8.1 is set to scale fonts to 125% by default. Toggling hidpi with https://addons.mozilla.org/editors/review/fxdpi doesn't let the thick line shrink.
Flags: needinfo?(archaeopteryx)
(In reply to Archaeopteryx [:aryx] from comment #2)
> The Windows 8.1 is set to scale fonts to 125% by default.

This depends on your screen/computer, I think. Mine defaulted to 100%, I think? That's certainly what it's currently set at...


Do you have time to find a regression window? :-)
Flags: needinfo?(archaeopteryx)
Keywords: regression, regressionwindow-wanted
This is a regression from http://hg.mozilla.org/mozilla-central/rev/87f15ce8266d / bug 1147702 and only visible if the menubar is hidden.
Blocks: 1147702
status-firefox39: --- → unaffected
Flags: needinfo?(archaeopteryx)
Keywords: regressionwindow-wanted
The nav-bar has different heights:

Without bug: 39.6px
With bug: 40px

Dao, do you have any ideas what has changed?
Flags: needinfo?(dao)
This does only occur if there are buttons without the class "badged-button" in the toolbar. E.g. remove all toolbar buttons except Loop and this issue will go away.
(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #5)
> The nav-bar has different heights:
> 
> Without bug: 39.6px
> With bug: 40px
> 
> Dao, do you have any ideas what has changed?
For me it's 40.8px on HiDPI and 41px on normal DPI (100%)   [Win7_64, Nightly 45 (2015-10-31), 32bit]
I think the answer is bug 477157 which breaks borders on zoom / transform:scale() .
Note that even users with DPI = 100% can _test_ this bug by setting layout.css.devPixelsPerPx -> 1.25
See Also: → 477157
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] from comment #5)
> The nav-bar has different heights:
> 
> Without bug: 39.6px
> With bug: 40px
> 
> Dao, do you have any ideas what has changed?

no idea
Flags: needinfo?(dao+bmo)
(In reply to arni2033 from comment #7)
> I think the answer is bug 477157 which breaks borders on zoom /
> transform:scale() .

Sounds about right. So we could fix this bug by setting a border on #navigator-toolbox::after instead of using background. Would you like to write that patch?
Flags: needinfo?(arni2033)
Priority: -- → P3
(In reply to Dão Gottwald [:dao] from comment #9)
> Sounds about right. So we could fix this bug by setting a border on #navigator-toolbox::after
> instead of using background. Would you like to write that patch?
Sorry for long response. No, because I'm too busy saving bugs locally.

Your solution wasn't immediately obvious to me, so I'll leave this note anyway:
Comment 7 only explains why toolbox has fractional height in this particular case, but it's not the
root of the problem (see [1]). So I didn't immediately understand that bug477157 is a way to _fix_
this, and solution will work as long as bug477157 is present. (Does it mean that 477157 is WONTFIX?..)

Testcases: bug 1224732 comment 5, url [1], url [2]
> [1] data:text/html,<!DOCTYPE html><body id="B" onload="S='';for(i=0;i<10;i++)S+='<div style=\'margin:0px 2px '+i+'px 0px;width:15px;height:1px;background:black;display:inline-block;\'></div>';for(i=0;i<10;i++)S+='<div style=\'margin:0px 0px 2px '+i+'px;height:15px;width:1px;background:black;\'></div>';B.innerHTML=S+'<title>Even elements w/ integer position have different width on zoom'">
> [2] data:text/html,<!DOCTYPE html><body id="B" onload="S='';for(i=0;i<10;i+=(i<1)?0.1:1)S+='<div style=\'margin:0px 2px '+i+'px 0px;width:15px;border-bottom:1px solid black;display:inline-block;\'></div>';for(i=0;i<10;i+=(i<1)?0.1:1)S+='<div style=\'margin:0px 0px 2px '+i+'px;height:15px;border-left:1px solid black;\'></div>';B.innerHTML=S+'<title>Elements have equal width on any zoom level'">
Flags: needinfo?(arni2033)
(In reply to arni2033 from comment #10)
> Your solution wasn't immediately obvious to me, so I'll leave this note
> anyway:
> Comment 7 only explains why toolbox has fractional height in this particular
> case, but it's not the
> root of the problem (see [1]). So I didn't immediately understand that
> bug477157 is a way to _fix_
> this, and solution will work as long as bug477157 is present. (Does it mean
> that 477157 is WONTFIX?..)

Hm, I expected bug 477157 would (if fixed) change how backgrounds are rendered, not borders. Maybe it's the other way around.
Summary: separator line (pseude ::after element) between nav-box (tabbox with tabs, bookmarks toolbar) and content has increased height (2px instead of 1) → separator line (pseudo ::after element) between nav-box (tabbox with tabs, bookmarks toolbar) and content has increased height (2px instead of 1)
Oh wait... does anyone actually still see this bug? I can't seem to reproduce it with latest Nightly on Windows 10 @ 125%.
Flags: needinfo?(aryx.bugmail)
Flags: needinfo?(arni2033)
Cant reproduce anymore with latest Nightly on the same machine.
Flags: needinfo?(aryx.bugmail)
>>>   My Info:   Win7_64, Nightly 51, 32bit, ID 20160902030222 (2016-09-02)
Can reproduce on a clear profile (launched via Mozregression GUI).
In some configurations of toolbar buttons and toolbars this bug manifests, in others it does not.

BTW, sorry for generating all the spam, but I actually think solution in comment 9 is good in long term, because (I believe) bug 477157 stays for a long time as is.
Flags: needinfo?(arni2033)
I can't reproduce this in a new profile either :/

But if you can reproduce this, maybe you could also check if my patch helps: https://archive.mozilla.org/pub/firefox/try-builds/dgottwald@mozilla.com-b6022e130526b4f929bec09a7507eb455516dd91/try-win32/
So I eventually can reproduce with a totally new profile, no idea what makes the difference to my testing stub. Your patch fixes the issue for me.
Attached patch patch — — Splinter Review 
AFAIK there's no reason to prefer background over border here... so I think this is a reasonable change even if bug 477157 may at some point make background scale like border (making this fix unnecessary as far as this bug is concerned) or vice versa (making this fix ineffective).
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #8787769 - Flags: review?(jaws)
Summary: separator line (pseudo ::after element) between nav-box (tabbox with tabs, bookmarks toolbar) and content has increased height (2px instead of 1) → [HiDPI] Separator between toolbars and content is sometimes too thick at 125% scale
Blocks: win-hidpi
Comment on attachment 8787769 [details] [diff] [review]
patch

Review of attachment 8787769 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/osx/browser.css
@@ +67,5 @@
>  @media (-moz-mac-yosemite-theme) {
>    #navigator-toolbox:-moz-window-inactive::after {
> +    border-top-style: none;
> +    border-bottom-color: hsla(0,0%,0%,.1);
> +    margin-top: -1px;

Can you explain why the margin-top should change here? And why the border-top isn't `1px solid hsla(0,0%,100%,0)`? Previously only the background-image changed and the stops of the linear-gradient were the same distance.
Attachment #8787769 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> Comment on attachment 8787769 [details] [diff] [review]
> patch
> 
> Review of attachment 8787769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/osx/browser.css
> @@ +67,5 @@
> >  @media (-moz-mac-yosemite-theme) {
> >    #navigator-toolbox:-moz-window-inactive::after {
> > +    border-top-style: none;
> > +    border-bottom-color: hsla(0,0%,0%,.1);
> > +    margin-top: -1px;
> 
> Can you explain why the margin-top should change here? And why the
> border-top isn't `1px solid hsla(0,0%,100%,0)`? Previously only the
> background-image changed and the stops of the linear-gradient were the same
> distance.

Because the negative margin-top matches the combined border width and hsla(0,0%,100%,0) is completely transparent so there's no point in having that border.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd747d43e7c7
Use CSS border instead of background for the separator between toolbars and content for more predictable high-DPI behavior. r=jaws
https://hg.mozilla.org/mozilla-central/rev/fd747d43e7c7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
See Also: → 1327324
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: