Window drag space next to tabs is too small

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Ovidiu, Assigned: johannh)

Tracking

(Blocks 1 bug)

57 Branch
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(4 attachments)

Reporter

Description

2 years ago
[Affected versions]:

- Nightly 57.0a1 
 
[Affected platforms]:

- Windows 7 x32
 
[Steps to reproduce]:

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


Actual result:

The persistent drag space is smaller 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.
Reporter

Updated

2 years ago
Whiteboard: [photon-visual]
Assignee

Updated

2 years ago
Whiteboard: [photon-visual] → [photon-visual][triage]
Flags: qe-verify+
Priority: -- → P3
QA Contact: ovidiu.boca
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Summary: Persistent drag space windows 7 → Persistent drag space is too small
I verify this issue with Nightly 57.0a1 and is also reproducible on Windows 8.1 and Windows 10.
The actual result in this case is 33 px.
Blocks: photon-tabs
No longer blocks: photon-visual
Summary: Persistent drag space is too small → Window drag space next to tabs is too small
Duplicate of this bug: 1397650
Tested this on Windows 10 with latest Nightly 57.0a1, the drag space next to tabs is 23px instead of 32px.
Assignee

Comment 4

2 years ago
I can take a look at this.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: P3 → P1
Iteration: --- → 57.3 - Sep 19
Iteration: 57.3 - Sep 19 → ---
Comment hidden (mozreview-request)
Assignee

Comment 6

2 years ago
Dão, I was hesitant to submit this to review earlier because I wanted to avoid conflicts with bug 1390025. I'll clear up the conflicts as soon as that patch lands. I'm not sure if this patch is the best way to do it (we could alternatively add another titlebar-placeholder element after tabs), but I don't think there's any advantage in doing it differently.

Let me know what you think.

Comment 7

2 years ago
mozreview-review
Comment on attachment 8912150 [details]
Bug 1397265 - Add a titlebar-placeholder on the right-hand side of the tabs.

https://reviewboard.mozilla.org/r/183532/#review188890

::: browser/themes/shared/browser.inc.css:31
(Diff revision 1)
> +:root[tabsintitlebar] #TabsToolbar
> +%endif
> +{
> +  padding-inline-end: 40px;
> +}
> +%endif

I removed the code you modeled this after in bug 1390025. I think I'd prefer this to be a margin for .titlebar-placeholder[type="caption-buttons"].
Assignee

Comment 8

2 years ago
(In reply to Dão Gottwald [::dao] from comment #7)
> Comment on attachment 8912150 [details]
> Bug 1397265 - Add 40px window drag space on the right-hand side of the tabs.
> 
> https://reviewboard.mozilla.org/r/183532/#review188890
> 
> ::: browser/themes/shared/browser.inc.css:31
> (Diff revision 1)
> > +:root[tabsintitlebar] #TabsToolbar
> > +%endif
> > +{
> > +  padding-inline-end: 40px;
> > +}
> > +%endif
> 
> I removed the code you modeled this after in bug 1390025. I think I'd prefer
> this to be a margin for .titlebar-placeholder[type="caption-buttons"].

Yes, I considered that, but caption buttons is on the left for Mac OS, how would that work?

Comment 9

2 years ago
mozreview-review
Comment on attachment 8912150 [details]
Bug 1397265 - Add a titlebar-placeholder on the right-hand side of the tabs.

https://reviewboard.mozilla.org/r/183532/#review189712

> > I removed the code you modeled this after in bug 1390025. I think I'd prefer
> > this to be a margin for .titlebar-placeholder[type="caption-buttons"].
> 
> Yes, I considered that, but caption buttons is on the left for Mac OS, how
> would that work?

Not sure, maybe we should have a "post-tabs" placeholder on Mac as opposed to a pre-tabs one? Either way we'll need an updated patch since this doesn't apply after bug 1390025.
Attachment #8912150 - Flags: review?(dao+bmo)
Assignee

Comment 10

2 years ago
(In reply to Dão Gottwald [::dao] from comment #9)
> Comment on attachment 8912150 [details]
> Bug 1397265 - Add 40px window drag space on the right-hand side of the tabs.
> 
> https://reviewboard.mozilla.org/r/183532/#review189712
> 
> > > I removed the code you modeled this after in bug 1390025. I think I'd prefer
> > > this to be a margin for .titlebar-placeholder[type="caption-buttons"].
> > 
> > Yes, I considered that, but caption buttons is on the left for Mac OS, how
> > would that work?
> 
> Not sure, maybe we should have a "post-tabs" placeholder on Mac as opposed
> to a pre-tabs one? Either way we'll need an updated patch since this doesn't
> apply after bug 1390025.

As I mentioned in comment 6, I don't think there's any advantage to that, do you?
I expect that it allows for simpler selectors.
Comment hidden (mozreview-request)
Assignee

Comment 13

2 years ago
Good point.
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8912150 [details]
Bug 1397265 - Add a titlebar-placeholder on the right-hand side of the tabs.

https://reviewboard.mozilla.org/r/183532/#review189744

::: browser/base/content/browser.xul:686
(Diff revision 2)
>          </menupopup>
>        </toolbarbutton>
>  
> +#ifdef CAN_DRAW_IN_TITLEBAR
> +      <hbox class="titlebar-placeholder" type="post-tabs"
> +              skipintoolbarset="true"/>

nit: indentation is off (my fault, affects the pre-tabs placeholder too

::: browser/base/content/browser.xul:686
(Diff revision 2)
>          </menupopup>
>        </toolbarbutton>
>  
> +#ifdef CAN_DRAW_IN_TITLEBAR
> +      <hbox class="titlebar-placeholder" type="post-tabs"
> +              skipintoolbarset="true"/>

Need to add ordinal="1000"

::: browser/themes/windows/browser.css
(Diff revision 2)
>      -moz-box-align: start;
>    }
>  }
>  
> -.titlebar-placeholder[type="caption-buttons"] {
> -  margin-left: 22px; /* space needed for Aero Snap */

Does this need to be updated or removed too? http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/browser/themes/osx/browser.css#80-87
Attachment #8912150 - Flags: review?(dao+bmo)
Assignee

Comment 16

2 years ago
mozreview-review-reply
Comment on attachment 8912150 [details]
Bug 1397265 - Add a titlebar-placeholder on the right-hand side of the tabs.

https://reviewboard.mozilla.org/r/183532/#review189744

> Does this need to be updated or removed too? http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/browser/themes/osx/browser.css#80-87

Nope, those add space between the window border and the caption/fullscreen buttons.
Comment hidden (mozreview-request)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8912150 [details]
Bug 1397265 - Add a titlebar-placeholder on the right-hand side of the tabs.

https://reviewboard.mozilla.org/r/183532/#review189754
Attachment #8912150 - Flags: review?(dao+bmo) → review+

Comment 19

2 years ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b50e085efc2f
Add a titlebar-placeholder on the right-hand side of the tabs. r=dao

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b50e085efc2f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Comment 21

2 years ago
The space on the right side, just before windows caption buttons is now bigger, but is it expected to have an extra space before the first tab also in maximized mode ? I haven't seen such space in the interactive mockup (Win 10) or specs from https://bugzilla.mozilla.org/show_bug.cgi?id=1397265#c0

Comment 22

2 years ago
3.22 +.titlebar-placeholder[type="pre-tabs"],

I think this line is buggy
Assignee

Updated

2 years ago
Depends on: 1404337
Assignee

Comment 23

2 years ago
(In reply to Eddward from comment #21)
> The space on the right side, just before windows caption buttons is now
> bigger, but is it expected to have an extra space before the first tab also
> in maximized mode ? I haven't seen such space in the interactive mockup (Win
> 10) or specs from https://bugzilla.mozilla.org/show_bug.cgi?id=1397265#c0

Thanks, filed bug 1404337 and will fix it in a second.
Do we want to uplift that to 57 or let it ride the train with 58?
Flags: needinfo?(jhofmann)
Assignee

Comment 25

2 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> Do we want to uplift that to 57 or let it ride the train with 58?

This got a little splintered into three bugs (also bug 1404337 and bug 1404497), but they're all very low-risk, so I'll just request uplift for all three.
Flags: needinfo?(jhofmann)
Assignee

Comment 26

2 years ago
Comment on attachment 8912150 [details]
Bug 1397265 - Add a titlebar-placeholder on the right-hand side of the tabs.

Approval Request Comment
[Feature/Bug causing the regression]: Oversight in Photon implementation
[User impact if declined]: Inconsistent window drag space
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Not really. 
[List of other uplifts needed for the feature/fix]: Bug 1404337 and bug 1404497.
[Is the change risky?]: No
[Why is the change risky/not risky?]: The change is quite simple and people were extensively testing this on Nightly already. It's unlikely that we find additional problems.
[String changes made/needed]: None
Attachment #8912150 - Flags: approval-mozilla-beta?
Comment on attachment 8912150 [details]
Bug 1397265 - Add a titlebar-placeholder on the right-hand side of the tabs.

Photon related, beta57+
Attachment #8912150 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1406063
I verified this issue using Latest Nightly 58.0a1 from 09-10-2017 on Windows 10 x64 and Windows 7 x64 where the issue is fixed but on Mac OS X 10.12  the problem still exists. Please see the attachment.
Flags: needinfo?(jhofmann)
Assignee

Comment 30

2 years ago
What's the issue exactly?
Flags: needinfo?(jhofmann) → needinfo?(valentina.ona)
The drag space is too big. 
Please see the attachment.
Flags: needinfo?(valentina.ona) → needinfo?(jhofmann)
Assignee

Comment 32

2 years ago
Hm, yeah, seems like it's a little too large on OSX. Please file a follow-up issue for that. Thanks!
Flags: needinfo?(jhofmann)
Blocks: 1407185
Based on the fact that has been opened a new bug 1407185 for the drag space I will mark this as fixed verified.
No longer blocks: 1407185
Status: RESOLVED → VERIFIED
Reporter

Comment 34

2 years ago
I verified this in FF 57.0b9 with Windows 7 x64 and I can confirm the fix.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.