Closed Bug 1407185 Opened 2 years ago Closed 2 years ago

Window drag space next to tabs is too big [Mac OS]

Categories

(Firefox :: Theme, defect, P1)

Unspecified
macOS
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox58 --- verified

People

(Reporter: valentina.ona, Assigned: daleharvey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(3 files)

[Affected versions]:

- Nightly 58.0a1 
 
[Affected platforms]:

- Mac OS X 10.12
 
[Steps to reproduce]:

1. Open the browser and open many tabs until the tab strip is overflowed.  


Actual result:

The persistent drag space is bigger than in the specs. 

Expected result:

The persistent drag space should match the specs. https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/229786513

[Note]: See the attached print screen with the actual result.

This is a follow-up from:
https://bugzilla.mozilla.org/show_bug.cgi?id=1397265#c32
Summary: Window drag space next to tabs is too big → Window drag space next to tabs is too big [Mac OS]
No longer depends on: 1397265
Flags: qe-verify+
Priority: -- → P4
Assignee: nobody → dharvey
So my first patch just gave osx a 34px width with the rest keeping with 40, bit of duplication code wise and doubt it would work great with mavericks fullscreen button

This seems a little riskier but fixes the alignment whatever is the rightmost in tabstoolbar
Status: NEW → ASSIGNED
Priority: P4 → P1
Comment on attachment 8918690 [details]
Bug 1407185 - Remove extra toolbar drag space on osx.

https://reviewboard.mozilla.org/r/189502/#review194840

The space is caused by the fullscreen button placeholder (https://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/browser/themes/osx/browser.css#93) and 2px padding on the last button.

Is this really the only thing we can do to prevent the placeholder from taking up space in OSX > 10.9? I haven't tested this on 10.9 but I have the feeling it wouldn't be correct there (the drag space would be too small). We should try not to show the placeholder when the fullscreen button is not shown.

What do you think?
Attachment #8918690 - Flags: review?(jhofmann) → review-
Ah with the inspector I couldnt see the fullscreen placeholder taking up space but I can see its effect if I modify the placeholder margin, will do a patch to prevent the placeholder taking up space when it isnt used, cheers
Depends on: 1408736
No longer depends on: 1408736
Comment on attachment 8918690 [details]
Bug 1407185 - Remove extra toolbar drag space on osx.

https://reviewboard.mozilla.org/r/189502/#review195616

This looks good to me. Make sure to coordinate with QA that the expected distance is 42px instead of 40px. I think it's fine not dealing with the extra complexity from removing the button padding right now (though I'd take a patch if someone is willing to work out the edge cases).

::: browser/themes/osx/browser.css:92
(Diff revision 4)
>  .titlebar-placeholder[type="caption-buttons"],
>  #titlebar-buttonbox {
>    margin-left: 7px;
>  }
>  
> +/* The fullscreen button doesnt show on Yosemite(10.9) or above so dont give it a

Nit: Yosemite is 10.10
Attachment #8918690 - Flags: review?(jhofmann) → review+
> work out the edge cases

budum tsh

But yeh the extra 2px from the new tab button isnt particularly clear from the spec if that should collapse or not and if thats what UX would like then will do it in a follow up, will fix the 10.10 then land, cheers
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04a6b79edba9
Remove extra toolbar drag space on osx. r=johannh
https://hg.mozilla.org/mozilla-central/rev/04a6b79edba9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
I verified this issue using Nightly 58.0a1 with Build ID 20171020100426 on Mac OS X 10.12.In the right side, drag space is 40px but on the left side is still 47 px which is not correct. 
Please see the attachment.
Flags: needinfo?(dharvey)
Hey Valentina, I opened a new bug so we can explicitly decide whether the 40px should include or excluse the margins of the buttons @ https://bugzilla.mozilla.org/show_bug.cgi?id=1411309, this bug now just removes the definitely wrong extra margin on an invisible button
Flags: needinfo?(dharvey)
Based on fact that another bug was filled I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.