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

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: clement.lefevre, Assigned: yzen)

Tracking

(Blocks 1 bug, {nightly-community})

unspecified
Firefox 58
Unspecified
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57- verified, firefox58 verified)

Details

(Whiteboard: [mozfr-community] [reserve-photon-visual])

Attachments

(8 attachments)

Reporter

Description

2 years ago
[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.
Reporter

Updated

2 years ago
OS: Unspecified → Mac OS X
Whiteboard: [mozfr-community][photon-visual] → [mozfr-community] [photon-visual] [triage]
(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.
Reporter

Comment 2

2 years ago
(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.
Reporter

Updated

2 years ago
Summary: On macOS, fullscreen button on private browsing windows is not vertically centered and the badge take the full height → On macOS, fullscreen button is not vertically centered nor anchored to the top while it should, as it was
Flags: qe-verify+
Priority: -- → P3
QA Contact: ovidiu.boca
Whiteboard: [mozfr-community] [photon-visual] [triage] → [mozfr-community] [reserve-photon-visual]
Priority: P3 → P4
Assignee

Comment 4

2 years ago
Posted patch 1402309 patchSplinter Review
Attachment #8912329 - Flags: review?(dao+bmo)
Assignee

Updated

2 years ago
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
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.
Attachment #8912329 - Flags: review?(dao+bmo) → review?(jhofmann)
Priority: P4 → P1
Assignee

Updated

2 years ago
Summary: On macOS, fullscreen button is not vertically centered nor anchored to the top while it should, as it was → On macOS 10.9 (mavericks), fullscreen button is not vertically centered nor anchored to the top while it should, as it was
Reporter

Comment 6

2 years ago
:yzen, looking at the change of summary, this was not affecting other macOS versions?
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.
Attachment #8912329 - Flags: review?(jhofmann) → review+
Assignee

Comment 8

2 years ago
(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).
Assignee

Comment 9

2 years ago
Would you be able to try this version of Firefox and see if the issue is fixed in your environment? Thanks!
Flags: needinfo?(clement.lefevre)
Reporter

Comment 10

2 years ago
(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?
Flags: needinfo?(clement.lefevre)
Assignee

Comment 11

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

Comment 13

2 years ago
See comment 12
Flags: needinfo?(clement.lefevre)
Reporter

Comment 14

2 years ago
(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.
Flags: needinfo?(clement.lefevre)

Comment 15

2 years ago
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87438bd3387b
fix titlebar-fullscreen-button vertical alignment. r=johannh

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/87438bd3387b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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]: /
Attachment #8912329 - Flags: approval-mozilla-beta?
Comment on attachment 8912329 [details] [diff] [review]
1402309 patch

Photon polish, taking it.
Should be in 57b5
Attachment #8912329 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Flags: needinfo?(jhofmann)
(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?
Flags: needinfo?(jhofmann) → needinfo?(clement.lefevre)
Reporter

Comment 24

2 years ago
(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.
Flags: needinfo?(clement.lefevre)
Reporter

Comment 25

2 years ago
Reporter

Comment 26

2 years ago
Posted image firefox_light_theme
Reporter

Comment 27

2 years ago
Posted image thunderbird
Reporter

Comment 28

2 years ago
Posted image finder
Reporter

Comment 29

2 years ago
Looking at above screenshots, I'll let Johann decide whether this is the normal/wished style or not.
Flags: needinfo?(jhofmann)
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.
Flags: needinfo?(jhofmann)
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.
This is also verified on FF beta 57.0b9 with Mac OS X 10.9.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.