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

()

P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: clement.lefevre, Assigned: yzen)

Tracking

(Blocks: 1 bug, {nightly-community})

unspecified
Firefox 58
Unspecified
macOS
nightly-community
Points:
---

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

a year ago
Created attachment 8911152 [details]
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.
(Reporter)

Updated

a year 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

a year ago
Created attachment 8911165 [details]
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.
(Reporter)

Updated

a year 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]
Blocks: 1325171
Priority: P3 → P4
P4, not tracking.
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox57: --- → fix-optional
status-firefox58: --- → fix-optional
status-firefox-esr52: --- → unaffected
tracking-firefox57: ? → -
(Assignee)

Comment 4

a year ago
Created attachment 8912329 [details] [diff] [review]
1402309 patch
Attachment #8912329 - Flags: review?(dao+bmo)
(Assignee)

Updated

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year ago
OS X 10.9, since I could not display full screen button on newer osx.
(Assignee)

Comment 13

a year ago
See comment 12
Flags: needinfo?(clement.lefevre)
(Reporter)

Comment 14

a year 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

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/87438bd3387b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: fix-optional → fixed
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+

Comment 20

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2ad2dfbbb93a
status-firefox57: fix-optional → fixed
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)
Created attachment 8914771 [details]
Screen Shot 2017-10-03 at 12.30.27.png

Here is the attachment.
(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

a year 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

a year ago
Created attachment 8916213 [details]
firefox_default_theme
(Reporter)

Comment 26

a year ago
Created attachment 8916214 [details]
firefox_light_theme
(Reporter)

Comment 27

a year ago
Created attachment 8916215 [details]
thunderbird
(Reporter)

Comment 28

a year ago
Created attachment 8916216 [details]
finder
(Reporter)

Comment 29

a year 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.
status-firefox58: fixed → verified
This is also verified on FF beta 57.0b9 with Mac OS X 10.9.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.