Closed Bug 1397265 Opened 4 years ago Closed 4 years ago

Window drag space next to tabs is too small

Categories

(Firefox :: Theme, defect, P1)

Product:
Firefox
Component:
Theme
Version:
57 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: Ovidiu, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(4 files)

Persistent drag space windows 7.png
12.43 KB, image/png
Details
Bug 1397265 - Add a titlebar-placeholder on the right-hand side of the tabs.
59 bytes, text/x-review-board-request
dao
: review+
ritu
: approval-mozilla-beta+
Details
Screen Shot 2017-10-10 at 10.33.03 (2).png
1.29 MB, image/png
Details
Screen Shot 2017-10-10 at 10.33.03 (2).png
141.47 KB, image/png
Details 
[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.
Whiteboard: [photon-visual]
Whiteboard: [photon-visual] → [photon-visual][triage]
Flags: qe-verify+
Priority: -- → P3
QA Contact: ovidiu.boca
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Blocks: 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.
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 → ---
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 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"].
(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 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)
(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.
Good point.
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)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/b50e085efc2f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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
3.22 +.titlebar-placeholder[type="pre-tabs"],

I think this line is buggy
Depends on: 1404337
(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)
(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)
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)
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)
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-firefox58: fixed → verified
Status: RESOLVED → VERIFIED
I verified this in FF 57.0b9 with Windows 7 x64 and I can confirm the fix.
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.