1402309 - On macOS 10.9 (mavericks), fullscreen button is not vertically centered nor anchored to the top while it should, as it was

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Clément Lefèvre]

Attachment: Private browing sign and fullscreen button on OS X 10.9

[Tracking Requested - why for this release]:

After bug 1396240 got fixed and sign went back, I could see that the fullscreen button appear to be a lot lower than it should (it's actually lower than in normal windows), hence not vertically centered on the bar.
See the joined screenshot to have an overview.

In the meantime, private browsing sign and fullscreen button now looks very very close, is this normal? Same question for the private browing sign that takes the full height on compact density while it wasn't before if I remember correctly, is this intended?

Screenshot was taken on OS X 10.9.

Comment 1

[Dão Gottwald [::dao]]

(In reply to Clément Lefèvre from comment #0)
> Created attachment 8911152 [details]
> Private browing sign and fullscreen button on OS X 10.9

It looks like this screenshot isn't from the latest nightly build.

Comment 2

[Clément Lefèvre]

Attachment: Top-right corner on latest nightly

(In reply to Dão Gottwald [::dao] from comment #1)
> (In reply to Clément Lefèvre from comment #0)
> > Created attachment 8911152 [details]
> > Private browing sign and fullscreen button on OS X 10.9
> 
> It looks like this screenshot isn't from the latest nightly build.

No it isn't, I reused the one I posted on https://bugzilla.mozilla.org/show_bug.cgi?id=1396240#c7
It's then nine days old. It was still the same at the time I posted that bug bug it's right I didn't restarted my Nightly since one day or two for whatever reason. That's now done and I attached a new screenshot.
Private browsing badge indeed changed to a circular version so I guess that part isn't important anymore.

About the fullscreen button, at the time of that earlier comment it was at a different position between normal window and private browsing window.
It is now at the same position, but at the very bottom of the bar.

In my humble opinion it should be either vertically centered or anchored to the top of the bar rather than the bottom, closer to where it was before.

I'll edit the title to adjust with those changes.

Comment 3

[Julien Cristau [:jcristau]]

P4, not tracking.

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1402309 patch

Comment 5

[Dão Gottwald [::dao]]

Comment on attachment 8912329 [details] [diff] [review]
1402309 patch

Nice code simplification, thanks! Looks good to me if it works. I'm redirecting the review request to Johann since I can't currently test on Mac.

Comment 6

[Clément Lefèvre]

:yzen, looking at the change of summary, this was not affecting other macOS versions?

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8912329 [details] [diff] [review]
1402309 patch

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

I only tested on 10.12, but that works fine, thank you. The window controls have a very very slightly different vertical position than before, but that's ok and probably to be expected.

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Clément Lefèvre from comment #6)
> :yzen, looking at the change of summary, this was not affecting other macOS
> versions?

This is the only version I could reproduce it with. Newer OSX builds (like the current 10.12) do not have a full screen button (unless it's added through customization, and it is not the same one).

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Would you be able to try this version of Firefox and see if the issue is fixed in your environment? Thanks!

Comment 10

[Clément Lefèvre]

(In reply to Yura Zenevich [:yzen] from comment #9)
> Would you be able to try this version of Firefox and see if the issue is
> fixed in your environment? Thanks!

Which version are you referring to?

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

OS X 10.9, since I could not display full screen button on newer osx.

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

urgh sorry I realized I did not include the image: https://queue.taskcluster.net/v1/task/PkyJqFtrTja1Omw2EmAS2g/runs/0/artifacts/public/build/target.dmg

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

See comment 12

Comment 14

[Clément Lefèvre]

(In reply to Yura Zenevich [:yzen] from comment #13)
> See comment 12

No worries, I was just checking everything and trying to find older screenshots to compare with :)

I can't find old enough ones to compare with. It might have been a little bit higher, but I'm not even sure and it looks pretty natural on your test build to be honest.
As :johannh told, it moved window controls from one or two pixels vertically higher: I don't know if this is a problem, but it doesn't look particularly shocking (I guess it should be vertically centered but I don't know how to "measure" it).
This is nitpick, but I'll let you decide if that's worth checking it.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87438bd3387b
fix titlebar-fullscreen-button vertical alignment. r=johannh

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/87438bd3387b

Comment 17

[Dão Gottwald [::dao]]

Comment on attachment 8912329 [details] [diff] [review]
1402309 patch

Approval Request Comment
[Feature/Bug causing the regression]: photon tab strip and title bar changes
[User impact if declined]: fullscreen button is misaligned on OS X 10.9
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: hardly
[Why is the change risky/not risky?]: pretty straightforward CSS fix (actually a code simplification)
[String changes made/needed]: /

Comment 18

[Johann Hofmann [:johannh]]

Screenshots with the slightly moving window controls:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=64c8107a2a13895e9e4295806bf65b99639cb651&newProject=mozilla-central&newRev=57f68296c350469d73d788eb3695a898947b4acb

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912329 [details] [diff] [review]
1402309 patch

Photon polish, taking it.
Should be in 57b5

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2ad2dfbbb93a

Comment 21

[ovidiu boca[:Ovidiu]]

I tested this on Mac OS X 10.9 with FF Nightly 58.0a1(2017-10-02) and here are my results: I measured the distance of the buttons(the 3 buttons, close, maximize and minimize) and I have 8px from the button and 10px from the top, please see the attachment. Johann this is the expected output? I also so that the buttons have a kind of a border in the bottom part, I mark it with a red arrow. 
Thanks

Comment 22

[ovidiu boca[:Ovidiu]]

Attachment: Screen Shot 2017-10-03 at 12.30.27.png

Here is the attachment.

Comment 23

[Johann Hofmann [:johannh]]

(In reply to ovidiu boca[:Ovidiu] from comment #21)
> I tested this on Mac OS X 10.9 with FF Nightly 58.0a1(2017-10-02) and here
> are my results: I measured the distance of the buttons(the 3 buttons, close,
> maximize and minimize) and I have 8px from the button and 10px from the top,
> please see the attachment. Johann this is the expected output? I also so
> that the buttons have a kind of a border in the bottom part, I mark it with
> a red arrow. 
> Thanks

I noted the slight height difference in comment 7 but it's OK in my book. I'll redirect the window buttons question to Clément, since I don't know what it was like before (I'm not running 10.9 and we don't have screenshot tests there). I was under the impression that this is just the 10.9 style, is that correct?

Comment 24

[Clément Lefèvre]

(In reply to Johann Hofmann [:johannh] from comment #23)
> (In reply to ovidiu boca[:Ovidiu] from comment #21)
> > I tested this on Mac OS X 10.9 with FF Nightly 58.0a1(2017-10-02) and here
> > are my results: I measured the distance of the buttons(the 3 buttons, close,
> > maximize and minimize) and I have 8px from the button and 10px from the top,
> > please see the attachment. Johann this is the expected output? I also so
> > that the buttons have a kind of a border in the bottom part, I mark it with
> > a red arrow. 
> > Thanks
> 
> I noted the slight height difference in comment 7 but it's OK in my book.
> I'll redirect the window buttons question to Clément, since I don't know
> what it was like before (I'm not running 10.9 and we don't have screenshot
> tests there). I was under the impression that this is just the 10.9 style,
> is that correct?

Well, honnestly I was not shocked by that at all, and I guess that's way more visible due to the black bar of the default/dark theme. I'm not very good at distinguishing this kind of details, so I'll just add screenshots of how it looks on Firefox with default and light theme, and then with two other applications to compare and let you decide whether it's considered normal and should be like that or not.

Comment 25

[Clément Lefèvre]

Attachment: firefox_default_theme

Comment 26

[Clément Lefèvre]

Attachment: firefox_light_theme

Comment 27

[Clément Lefèvre]

Attachment: thunderbird

Comment 28

[Clément Lefèvre]

Attachment: finder

Comment 29

[Clément Lefèvre]

Looking at above screenshots, I'll let Johann decide whether this is the normal/wished style or not.

Comment 30

[Johann Hofmann [:johannh]]

I think we're good. It looks like this is just a little artifact from the dark background and it doesn't seem to annoy people too much.

Comment 31

[ovidiu boca[:Ovidiu]]

Based on comment 23 and comment 30 I will mark this as verified fixed. Testing was made on Nightly 58.0a1(2017-10-07) with Mac OS X 10.9.

Comment 32

[ovidiu boca[:Ovidiu]]

This is also verified on FF beta 57.0b9 with Mac OS X 10.9.