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.

Brian, should we special case windows 10 here ? Windows 10 now has a white titkebar by default.

(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?

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".

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.

(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.

ni? for Comment 7

Sure, here's the thing :)

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

Otherwise will have to keep the caption button cutout.

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.

The remaining white top border is because of bug 1163184.

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?

Rebased. diff -w coming right away.

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).

(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?

Gah, wrong needinfo.

(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.

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

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).

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?

(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.

(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

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).

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

(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.

(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

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.

(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

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.

(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.

(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

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.

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.

Thanks for the review Gijs. Honza

Needs rebasing.

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

(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

(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

@Brian: should we uplift this to Aurora? Honza

(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.

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

Honza, is this a regression?

Comment on attachment 8634214 [details] [diff] [review] bug1172716-5.patch Approving as we need to get win10 support in good shape on DevEdition.

(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

Needs rebasing for Aurora uplift.

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

(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

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

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

(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?

(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.

(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.

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

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+

(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.

(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.)

(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.