Verdi [:verdi] Best to slack me Reporter
	Description • 9 years ago

In windowed mode in Windows 10, The Dev Edition title bar and tab bar are white. In maximized mode the window control buttons have a white background.

	Tim Nguyen :ntim
	Comment 2 • 9 years ago

Brian, should we special case windows 10 here ?

Windows 10 now has a white titkebar by default.

	Brian Grinstead [:bgrins]
	Comment 3 • 9 years ago

(In reply to Tim Nguyen [:ntim] from comment #2)
> Brian, should we special case windows 10 here ?
> 
> Windows 10 now has a white titkebar by default.

Forwarding ni? to Stephen, who has done various Windows mockups.  What should this look like in Windows 10?

	:Gijs (he/him)
	Comment 4 • 9 years ago

P2 because the people most likely to upgrade to win10 soon overlap with people who'd be running devedition, so we should probably do something "soon".

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 6 • 9 years ago

Here's what Stephen and I agreed on:
• Remove the top padding of the tab strip (as on Mac DevEdition)
• Use #1c2125 (dark) and #e3e4e6 (light) as the background color for the entire tab strip
• Keep a little white »window« for the window controls, since we probably won't be able to replace them entirely for Fx40. If we can replace them, let's invert them.

Happy to supply mockups on request.

	Brian Grinstead [:bgrins]
	Comment 7 • 9 years ago

(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #6)
> Here's what Stephen and I agreed on:
> • Remove the top padding of the tab strip (as on Mac DevEdition)
> • Use #1c2125 (dark) and #e3e4e6 (light) as the background color for the
> entire tab strip
> • Keep a little white »window« for the window controls, since we probably
> won't be able to replace them entirely for Fx40. If we can replace them,
> let's invert them.
> 
> Happy to supply mockups on request.

Mockups would be very helpful for implementation and reviews, if it's not too much trouble.

	Brian Grinstead [:bgrins]
	Comment 8 • 9 years ago

ni? for Comment 7

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 9 • 9 years ago

Sure, here's the thing :)

	Stephen Horlander [:shorlander]
	Comment 10 • 9 years ago

If we can make the caption button inverted and mesh better in that layout it would be nice.

	Stephen Horlander [:shorlander]
	Comment 11 • 9 years ago

Otherwise will have to keep the caption button cutout.

	Tim Nguyen :ntim
	Comment 12 • 9 years ago

We won't be able to achieve (01) without bug 1173725.

Let's do (02) for now and do (01) once bug 1173725 is fixed.

	Tim Nguyen :ntim
	Comment 13 • 9 years ago

The remaining white top border is because of bug 1163184.

	:Gijs (he/him)
	Comment 14 • 9 years ago

Comment on attachment 8626911 [details] [diff] [review]
Patch

Review of attachment 8626911 [details] [diff] [review]:
-----------------------------------------------------------------

Why are you moving rules like this? Can you upload a diff -w that doesn't move it to ease reviewing?

	Tim Nguyen :ntim
	Comment 15 • 9 years ago

Rebased. diff -w coming right away.

	Tim Nguyen :ntim
	Comment 16 • 9 years ago

Pretty much all the changes in windows/devedition.css consist in moving rules into the @media query. The remaining changes in that files are just simple reordering (moving more global rules to the top).

	:Gijs (he/him)
	Comment 17 • 9 years ago

(In reply to Tim Nguyen [:ntim] from comment #16)
> Created attachment 8627251 [details] [diff] [review]
> diff -w
> 
> Pretty much all the changes in windows/devedition.css consist in moving
> rules into the @media query. The remaining changes in that files are just
> simple reordering (moving more global rules to the top).

I tried asking before *why* we are moving things. I don't understand what "more global rules" means, and I don't understand why the patch is reordering rules at all. I wouldn't expect the patch to change any rule orders unless there was a need for it because of rule specificity being equal (and the order therefore being important).

Can you clarify?

	:Gijs (he/him)
	Comment 18 • 9 years ago

Gah, wrong needinfo.

	Tim Nguyen :ntim
	Comment 19 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to Tim Nguyen [:ntim] from comment #16)
> > Created attachment 8627251 [details] [diff] [review]
> > diff -w
> > 
> > Pretty much all the changes in windows/devedition.css consist in moving
> > rules into the @media query. The remaining changes in that files are just
> > simple reordering (moving more global rules to the top).
> 
> I tried asking before *why* we are moving things. I don't understand what
> "more global rules" means, and I don't understand why the patch is
> reordering rules at all. I wouldn't expect the patch to change any rule
> orders unless there was a need for it because of rule specificity being
> equal (and the order therefore being important).
> 
> Can you clarify?

"More global rules" means rules that apply to all Windows versions. I think it makes sense to separate those rules from rules that apply to Windows 8 or lower.

It also feels ugly (at least to me) to have these kinds of rules split up before and after the @media query.

	:Gijs (he/him)
	Comment 20 • 9 years ago

Comment on attachment 8627247 [details] [diff] [review]
Patch v1.1

Review of attachment 8627247 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/devedition.css
@@ +213,5 @@
> +  #main-window[sizemode=normal]:not([customizing]) #TabsToolbar {
> +    padding-left: 0;
> +    padding-right: 0;
> +  }
> +

Nit: no need for whitespace here

	:Gijs (he/him)
	Comment 21 • 9 years ago

Comment on attachment 8627251 [details] [diff] [review]
diff -w

Review of attachment 8627251 [details] [diff] [review]:
-----------------------------------------------------------------

There are two issues:
1) there's no space to drag the window in either maximized or normal mode
2) the new size of the location and search bars makes the back/fwd button look crappy (I'm OK if you want to defer this to a different bug, but I figured I'd bring it up).

	Andrei Vaida [:avaida]
	Comment 22 • 9 years ago

Just noticed this on 42.0a1 (2015-07-07) and Aurora 41.0a2 (2015-07-07) using Microsoft Surface Pro 2 with Windows 10 Pro x64 (Insider Preview Build 10162): http://i.imgur.com/TT6pC6F.png. 

I assume this is more of a HiDPI-related issue since I couldn't replicate in on desktop, but I thought I should mention it here as well, just to be safe. Do we have a generic bug already on file for these kind of issues or should I file a new one?

	Tim Nguyen :ntim
	Comment 23 • 9 years ago

(In reply to Andrei Vaida, QA [:avaida] from comment #22)
> Just noticed this on 42.0a1 (2015-07-07) and Aurora 41.0a2 (2015-07-07)
> using Microsoft Surface Pro 2 with Windows 10 Pro x64 (Insider Preview Build
> 10162): http://i.imgur.com/TT6pC6F.png. 
> 
> I assume this is more of a HiDPI-related issue since I couldn't replicate in
> on desktop, but I thought I should mention it here as well, just to be safe.
> Do we have a generic bug already on file for these kind of issues or should
> I file a new one?

That's bug 1179283, which will be fixed by bug 1173725.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 24 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #21)
> Comment on attachment 8627251 [details] [diff] [review]
> diff -w
> 
> Review of attachment 8627251 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are two issues:
> 1) there's no space to drag the window in either maximized or normal mode
> 2) the new size of the location and search bars makes the back/fwd button
> look crappy (I'm OK if you want to defer this to a different bug, but I
> figured I'd bring it up).
Gijs, both is fixed and new patch attached, based on (Patch v1.1)

(this bug is important for DevEdition, so I am helping out here)

Honza

	:Gijs (he/him)
	Comment 25 • 9 years ago

Comment on attachment 8632786 [details] [diff] [review]
bug1172716-1.patch

Review of attachment 8632786 [details] [diff] [review]:
-----------------------------------------------------------------

This still doesn't have a drag area for non-maximized windows, where only on win10 you aligned the tabs with the top of the window (unlike on winxp-win8).

	:Gijs (he/him)
	Comment 26 • 9 years ago

Philipp, can you clarify what to do about the tabs and dragging the window here?

	Tim Nguyen :ntim
	Comment 27 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #26)
> Philipp, can you clarify what to do about the tabs and dragging the window
> here?

Seems like we can do the same thing as OSX : add more padding on the left.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 28 • 9 years ago

(In reply to Tim Nguyen [:ntim] (away 11-14 July) from comment #27)
> (In reply to :Gijs Kruitbosch from comment #26)
> > Philipp, can you clarify what to do about the tabs and dragging the window
> > here?
> 
> Seems like we can do the same thing as OSX : add more padding on the left.
Done, patch updated.

I tested also on OSX and noticed that the padding is on both left and right, so I did the same for Win10. Let me know if the right padding shouldn't be there.

Gijs: The change is just 6 lines you can diff with the previous patch.

Honza

	:Gijs (he/him)
	Comment 29 • 9 years ago

Comment on attachment 8633330 [details] [diff] [review]
bug1172716-2.patch

Review of attachment 8633330 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/devedition.css
@@ +118,5 @@
>  
> +/* Restore draggable space on the sides of tabs when maximized */
> +#main-window[sizemode="maximized"] .tabbrowser-arrowscrollbox > .arrowscrollbox-scrollbox {
> +  padding-left: 15px;
> +  padding-right: 15px;

Put this block back in the @media query, in the same order it was before, please.

@@ +219,5 @@
> +@media (-moz-os-version: windows-win10) {
> +  #back-button > .toolbarbutton-icon,
> +  #forward-button > .toolbarbutton-icon {
> +    padding-top: 4px !important;
> +    padding-bottom: 4px !important;

Why !important?

Also, this will only work with the default Windows font sizes. It'd be nice if we got this right (which I expect means calc(calc(1.2em + (whatever-the-urlbar-padding-is) - (whatever-the-basic-size-of-the-back/fwd-buttons-is)) / 2)), but that's my offhand attempt.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 30 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #29)
> > +/* Restore draggable space on the sides of tabs when maximized */
> > +#main-window[sizemode="maximized"] .tabbrowser-arrowscrollbox > .arrowscrollbox-scrollbox {
> > +  padding-left: 15px;
> > +  padding-right: 15px;
> 
> Put this block back in the @media query, in the same order it was before,
> please.
Done

> 
> @@ +219,5 @@
> > +@media (-moz-os-version: windows-win10) {
> > +  #back-button > .toolbarbutton-icon,
> > +  #forward-button > .toolbarbutton-icon {
> > +    padding-top: 4px !important;
> > +    padding-bottom: 4px !important;
> 
> Why !important?

Since devedition.css is already using !important for #back-button > .toolbarbutton-icon, #forward-button > .toolbarbutton-icon selector (line 38) to force padding. 

> Also, this will only work with the default Windows font sizes. It'd be nice
> if we got this right (which I expect means calc(calc(1.2em +
> (whatever-the-urlbar-padding-is) -
> (whatever-the-basic-size-of-the-back/fwd-buttons-is)) / 2)), but that's my
> offhand attempt.

I went through the patch for Bug 1173736 where the URL and search bar height has been increased (to 28px) and the patch among other things defines min-height: 28px for the .searchbar-textbox. Since, this is the min height that icons for back and forward buttons needs to follow I've also set it. I have tested with various font sizes in Win10 and works nicely for me (just like the search bar).

Honza

	:Gijs (he/him)
	Comment 31 • 9 years ago

Comment on attachment 8633431 [details] [diff] [review]
bug1172716-3.patch

Review of attachment 8633431 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jan Honza Odvarko [:Honza] from comment #30)
> I
> have tested with various font sizes in Win10 and works nicely for me (just
> like the search bar).

Really? How? Because I just tested it and it doesn't work for me.

STR:

1) apply patch, build, start Firefox
2) open the settings app
3) search for "font"
4) click the "Make text and other items larger or smaller" item
5) in the "Change the text size only" section, change the messagebox size to 14
6) wait for Windows to do whatever it's doing
7) sadness as your buttons no longer match up to your urlbar.

I can live with making this a followup, but I'd prefer to fix it right the first time.

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 32 • 9 years ago

(In reply to Tim Nguyen [:ntim] (away 11-14 July) from comment #27)
> (In reply to :Gijs Kruitbosch from comment #26)
> > Philipp, can you clarify what to do about the tabs and dragging the window
> > here?
> 
> Seems like we can do the same thing as OSX : add more padding on the left.

Yes, that seems reasonable.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 33 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #31)
> Comment on attachment 8633431 [details] [diff] [review]
> bug1172716-3.patch
> 
> Review of attachment 8633431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Jan Honza Odvarko [:Honza] from comment #30)
> > I
> > have tested with various font sizes in Win10 and works nicely for me (just
> > like the search bar).
> 
> Really? How? Because I just tested it and it doesn't work for me.
> 
> STR:
> 
> 1) apply patch, build, start Firefox
> 2) open the settings app
> 3) search for "font"
> 4) click the "Make text and other items larger or smaller" item
> 5) in the "Change the text size only" section, change the messagebox size to
> 14
> 6) wait for Windows to do whatever it's doing
> 7) sadness as your buttons no longer match up to your urlbar.
Alright, I can see the problem now.

I have been testing with "Change the size of text, apps and other items: 100%"


> I can live with making this a followup, but I'd prefer to fix it right the
> first time.
OK, let's solve this as a separate report (bug 1184097), so we can land patch for the original report. Patch updated.

Honza

	Tim Nguyen :ntim
	Comment 34 • 9 years ago

Comment on attachment 8634050 [details] [diff] [review]
bug1172716-4.patch

Review of attachment 8634050 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/devedition.css
@@ +215,5 @@
> +    padding-right: 0;
> +  }
> +}
> +
> +@media (-moz-os-version: windows-win10) {

Adding this media query will make this code not apply to future Win versions (after Win 10). Not sure if this is wanted.

	:Gijs (he/him)
	Comment 35 • 9 years ago

Comment on attachment 8634050 [details] [diff] [review]
bug1172716-4.patch

Review of attachment 8634050 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not too worried about the media query because we use it for the urlbar on win10 as well.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 36 • 9 years ago

Thanks for the review Gijs.

Honza

	Ryan VanderMeulen [:RyanVM]
	Comment 37 • 9 years ago

Needs rebasing.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 38 • 9 years ago

Patch rebased

Brian: I had to rebase on top of your changes:
https://github.com/mozilla/gecko-dev/commit/eb3ca5522ae9a5b303ff52bf5a9962bb2388d94e

Can you please test it to make sure it still works after the merge?

Honza

	Brian Grinstead [:bgrins]
	Comment 39 • 9 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #38)
> Created attachment 8634214 [details] [diff] [review]
> bug1172716-5.patch
> 
> Patch rebased
> 
> Brian: I had to rebase on top of your changes:
> https://github.com/mozilla/gecko-dev/commit/
> eb3ca5522ae9a5b303ff52bf5a9962bb2388d94e
> 
> Can you please test it to make sure it still works after the merge?
> 
> Honza

Tested and that fix is still in place for Win7 classic theme.  Looks like this should be good to go

	Brian Grinstead [:bgrins]
	Comment 40 • 9 years ago

(In reply to Tim Nguyen [:ntim] from comment #12)
> We won't be able to achieve (01) without bug 1173725.
> 
> Let's do (02) for now and do (01) once bug 1173725 is fixed.

Filed Bug 1184325 to invert the titlebar controls

	Pulsebot
	Comment 41 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/343b87d18876

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 42 • 9 years ago

@Brian: should we uplift this to Aurora?

Honza

	Brian Grinstead [:bgrins]
	Comment 43 • 9 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #42)
> @Brian: should we uplift this to Aurora?
> 
> Honza

Our next release is on Aug 11 and I see a release date for Windows 10 as July 29, so I guess we probably should.  We should also probably look into uplifting Bug 1165718  (which you had to rebase on top of and is a general fix that would be nice to uplift anyway) to simplify things.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 44 • 9 years ago

Comment on attachment 8634214 [details] [diff] [review]
bug1172716-5.patch

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: space above tab bar and colors in DevEdition Win 10 are wrong (breaking the theme)
[Describe test coverage new/current, TreeHerder]: manual testing
[Risks and why]: Space above tab bar and colors could be wrong in DevEdition theme.
[String/UUID change made/needed]: n/a

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 45 • 9 years ago

Honza, is this a regression?

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 46 • 9 years ago

Comment on attachment 8634214 [details] [diff] [review]
bug1172716-5.patch

Approving as we need to get win10 support in good shape on DevEdition.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 47 • 9 years ago

(In reply to Ritu Kothari (:ritu) from comment #45)
> Honza, is this a regression?
No, I don't think so.

Note that this bug depends on bug 1165718 (also related to DevEdition theme),
where I also requested approval for Aurora uplift.
(so, it should be approved too now)

Honza

	Tim Taubert [:ttaubert] (inactive)
	Comment 48 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/343b87d18876

	Ryan VanderMeulen [:RyanVM]
	Comment 49 • 9 years ago

Needs rebasing for Aurora uplift.

	Tim Nguyen :ntim
	Comment 50 • 9 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #49)
> Needs rebasing for Aurora uplift.

Needs bug 1165718 to be uplifted first.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 51 • 9 years ago

(In reply to Tim Nguyen [:ntim] from comment #50)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #49)
> > Needs rebasing for Aurora uplift.
> 
> Needs bug 1165718 to be uplifted first.

Yes, precisely, bug 1165718 needs to be uplifted first, see also my comment #47

Honza

	Ryan VanderMeulen [:RyanVM]
	Comment 52 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/8ac1cdef68f1

	Cornel Ionce [:noni] [Hubs QA]
	Comment 53 • 9 years ago

Confirming this fix on Windows 10 64-bit using latest 42.0a1 Nightly (build ID: 20150722030205) and latest 41.0a2 Aurora (build ID: 20150722004007).

	Verdi [:verdi] Best to slack me Reporter
	Comment 54 • 9 years ago

I just installed today's dev edition and, for me, the window controls aren't inverted and the tab close button on the active tab isn't sized correctly (it flips to a correctly sized button when you hover over it).

Firefox Version 	41.0a2 Build ID 	20150723004007

	Brian Grinstead [:bgrins]
	Comment 55 • 9 years ago

(In reply to Verdi [:verdi] from comment #54)
> Created attachment 8637960 [details]
> dev-edition-july23.jpg
> 
> I just installed today's dev edition and, for me, the window controls aren't
> inverted and the tab close button on the active tab isn't sized correctly
> (it flips to a correctly sized button when you hover over it).
> 
> Firefox Version 	41.0a2 Build ID 	20150723004007

Thanks for checking - the inverted issue is fixed in bug 1184325 which hasn't been uplifted yet.  Regarding the tab close button, do you see that issue on nightly also?

	Verdi [:verdi] Best to slack me Reporter
	Comment 56 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #55)
> Regarding the tab close button, do you see that
> issue on nightly also?

Nope. Nightly looks good.

	Tim Nguyen :ntim
	Comment 57 • 9 years ago

(In reply to Verdi [:verdi] from comment #54)
> Created attachment 8637960 [details]
> dev-edition-july23.jpg
> 
> I just installed today's dev edition and, for me, the window controls aren't
> inverted and the tab close button on the active tab isn't sized correctly
> (it flips to a correctly sized button when you hover over it).
> 
> Firefox Version 	41.0a2 Build ID 	20150723004007

That is bug 1185889.

	Tim Nguyen :ntim
	Comment 58 • 9 years ago

Comment on attachment 8634214 [details] [diff] [review]
bug1172716-5.patch

Approval Request Comment
[Feature/regressing bug #]: windows 10
[User impact if declined]: bug 1184934 can't be uplifted
[Describe test coverage new/current, TreeHerder]: on m-c and aurora, tested by QA
[Risks and why]: low, see test coverage.
[String/UUID change made/needed]: none

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 59 • 9 years ago

Comment on attachment 8634214 [details] [diff] [review]
bug1172716-5.patch

This is a bigger change than I'd normally like to see for beta9 but it is both verified and a prereq of bug 1184934 (support for Windows 10). Beta+

	Brian Grinstead [:bgrins]
	Comment 60 • 9 years ago

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #59)
> Comment on attachment 8634214 [details] [diff] [review]
> bug1172716-5.patch
> 
> This is a bigger change than I'd normally like to see for beta9 but it is
> both verified and a prereq of bug 1184934 (support for Windows 10). Beta+

FWIW the Dev Edition theme can't be applied at all (for now) on Beta/Release so the biggest changes in this patch won't affect it.

	Justin Dolske [:Dolske]
	Comment 61 • 9 years ago

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #59)

> This is a bigger change than I'd normally like to see for beta9 but it is
> both verified and a prereq of bug 1184934 (support for Windows 10). Beta+

I concur. This is bigger than we'd prefer to take in the last beta, hence our beta-specific tweak to bug 1184934 to avoid the trivial dependency on this.


(In reply to Brian Grinstead [:bgrins] from comment #60)

> FWIW the Dev Edition theme can't be applied at all (for now) on Beta/Release
> so the biggest changes in this patch won't affect it.

I tested this, and indeed the old force-enable pref no longer works. (And in any case, between it being quasi-unsupported and not something we've been testing as part of the Windows 10 work, I'd have been ok with it being broken anyway.)

	Brian Grinstead [:bgrins]
	Comment 62 • 9 years ago

(In reply to Justin Dolske [:Dolske] from comment #61)
> (In reply to Brian Grinstead [:bgrins] from comment #60)
> 
> > FWIW the Dev Edition theme can't be applied at all (for now) on Beta/Release
> > so the biggest changes in this patch won't affect it.
> 
> I tested this, and indeed the old force-enable pref no longer works. (And in
> any case, between it being quasi-unsupported and not something we've been
> testing as part of the Windows 10 work, I'd have been ok with it being
> broken anyway.)

Bug 1181721 is opened to possibly re-enable it on release channels.  But yeah, the pref was never publicized as working in non-DevEdition contexts, it just happened to work before we switched it to the lightweight theme implementation.