1173725 - Title bar and tab strip should have a darker background color on Windows 10

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Dão Gottwald [:dao]]

According to <http://people.mozilla.org/~shorlander/mockups/Windows-10/Windows-10.html>, we should use hsl(0,0%,78%).

Comment 1

[Tim Nguyen :ntim]

I wonder if this will need platform work for the titlebar. Especially since we may see a white background under the window controls.

Comment 2

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] from comment #1)
> I wonder if this will need platform work for the titlebar. Especially since
> we may see a white background under the window controls.

It will, unless we opt to show the XUL buttons and just use images to simulate the "real" buttons.

Comment 3

[Tim Nguyen :ntim]

Here is an interesting article about the Windows 10 titlebar APIs : http://blog.thomasnigro.fr/2015/02/11/exploring-w10-apis-windows-and-titlebar/
I guess we could explore that.

Comment 4

[Tim Nguyen :ntim]

That API seems official : https://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.viewmanagement.applicationviewtitlebar.aspx

Comment 5

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen [:ntim] (unavailable 17-25 June) from comment #4)
> That API seems official :
> https://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.
> viewmanagement.applicationviewtitlebar.aspx

Jim, can we somehow make it so that we set these things with CSS such that our widget code forwards them to Windows?

Comment 6

[:Gijs (out for now; he/him)]

I wonder if this even works from a classic app. All the examples on that page are modern apps. The titlebar Windows gives us doesn't actually look like the example, and doesn't get "modern" treatment when used in tablet mode.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Tim Nguyen [:ntim] (ltd. availability 17-25 June) from comment #4)
> That API seems official :
> https://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.
> viewmanagement.applicationviewtitlebar.aspx

"The universal device family is special. It is not, directly, the foundation of any OS. Instead, the set of APIs in the universal device family is inherited by child device families. The universal device family APIs are thus guaranteed to be present in every OS and consequently on every device."

https://msdn.microsoft.com/en-us/library/dn894631.aspx

Should be available to us according to the docs.

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to Tim Nguyen [:ntim] (unavailable 17-25 June) from comment #4)
> > That API seems official :
> > https://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.
> > viewmanagement.applicationviewtitlebar.aspx
> 
> Jim, can we somehow make it so that we set these things with CSS such that
> our widget code forwards them to Windows?

We just added a new idl down in widget for win10 related stuff. You could hand it through there. You could also hand it through via a theme related api. I guess it depends on when you want this set and how often.

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > (In reply to Tim Nguyen [:ntim] (unavailable 17-25 June) from comment #4)
> > > That API seems official :
> > > https://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.
> > > viewmanagement.applicationviewtitlebar.aspx
> > 
> > Jim, can we somehow make it so that we set these things with CSS such that
> > our widget code forwards them to Windows?
> 
> We just added a new idl down in widget for win10 related stuff. You could
> hand it through there. You could also hand it through via a theme related
> api. I guess it depends on when you want this set and how often.

I expect ideally we would like to be able to also change it for lwthemes.

I'm still a little confused how this maps to the OS-wide accent color.

Comment 9

[:Gijs (out for now; he/him)]

I'll poke at this now it looks like bug 1170522 will land soon.

Comment 10

[:Gijs (out for now; he/him)]

So the weird thing is that GetForCurrentView is a static thing on ApplicationView, and there doesn't seem to be an Interop class like there was for the stuff in bug 1170522 where you pass in an HWND. So I am struggling to understand if/how the rv from GetForCurrentView will reflect which of the many windows your app has...

Comment 11

[Tim Nguyen :ntim]

It may be worth upgrading to build 100158 before working on this bug for these 2 reasons :
- Win 32 titlebar controls and Universal apps titlebar controls are now consistent
They now have the same thickness and the same opacity for inactive windows. Since titlebar controls are now consistent across different types of apps, the UWA APIs for the titlebars could possibly work on Win32 apps as well.

- MS is pushing the Windows 10 preview SDK for this build

Source : http://blogs.windows.com/bloggingwindows/2015/06/29/announcing-windows-10-insider-preview-build-10158-for-pcs/

Comment 12

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] from comment #11)
> It may be worth upgrading to build 100158 before working on this bug for
> these 2 reasons :
> - Win 32 titlebar controls and Universal apps titlebar controls are now
> consistent

They also interfere with the minimize button in Firefox, I presume because the dwm doesn't have the updated sizes...

> They now have the same thickness and the same opacity for inactive windows.
> Since titlebar controls are now consistent across different types of apps,
> the UWA APIs for the titlebars could possibly work on Win32 apps as well.
> 
> - MS is pushing the Windows 10 preview SDK for this build

The latest available is still 10.0.10075 which does not seem to correspond to this. :-\
I think there's meant to be a blogpost on http://blogs.windows.com/buildingapps/ today when they upload the new one, but my EOB for "today" is already gone. I'm a little surprised, but maybe it'll be there tomorrow.

Comment 13

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] from comment #11)
> It may be worth upgrading to build 100158 before working on this bug for
> these 2 reasons :
> - Win 32 titlebar controls and Universal apps titlebar controls are now
> consistent

They still don't work the same way in tablet mode (the minimize and maximize button don't disappear). We also gained bug 1179283). Not a net improvement, tbh. :-\

> They now have the same thickness and the same opacity for inactive windows.
> Since titlebar controls are now consistent across different types of apps,
> the UWA APIs for the titlebars could possibly work on Win32 apps as well.

So, I have found no evidence of changes in this respect.


In the meantime, I have an attempt at a patch to just test this functionality, and I find that trying to use "IApplicationView" from a win32 app doesn't seem to work. It returns an HRESULT of 0x80070490, AKA "ELEMENT_NOT_FOUND". From looking around, it seems on WinRT this generally means "you're not running on the UI thread for your (winrt) application('s view/corewindow)".

I expect that for win32 this really means "you're not a winrt app so there isn't a winrt application view for you to access" (there's not an awful lot of precedent here to access winrt from win32 *generally*, as far as I can tell).

Jim, do you think I am being overly pessimistic? Because if not, it seems like this is essentially "cantfix" unless we switch to non-native buttons or decide we're OK with a cutout for the titlebar buttons...

Comment 14

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: Bug 1173725 - part 1: force top border to be visible on windows 10 and don't cut out caption buttons, r?jimm

Bug 1173725 - proof of concept trying to access the IApplicationViewTitlebar API from win32

Comment 15

[:Gijs (out for now; he/him)]

So this is a poc patch. It fails doing:

    hr = appViewStatics->GetForCurrentView(&appView);

Comment 16

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #15)
> So this is a poc patch. It fails doing:
> 
>     hr = appViewStatics->GetForCurrentView(&appView);

Oh rats. So this can only be accessed from a win32 app running the winrt environment. Looks like we can't use this api.

Comment 17

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

So, since making the caption buttons transparent seems to be capital H Hard, how about hiding them altogether and drawing our own buttons. It seems that all we'd need for this is the following SVG icons:
- minimize
- maximize
- window
- close (maybe a separate white version of that)

Modern apps don't even use any animation on these items, so we can get away with ditching that as well.
This would also allow us to freely position the buttons (vertical centering, yay!)

NI Stephen for the icons.
I think ntim had a version of those in his user style (though they had some glitches).

Comment 18

[Tim Nguyen :ntim]

Attachment: caption-buttons.svg

Comment 19

[Tim Nguyen :ntim]

(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #17)
> So, since making the caption buttons transparent seems to be capital H Hard,
> how about hiding them altogether and drawing our own buttons.
This has a few cons (as seen in the userstyle) :
- This requires the whole window to be redrawn (-moz-appearance: none;) unless we do some platform work that would allow us to hide the caption buttons without redrawing the whole window
- Redrawing the whole window is fairly buggy, here are 2 known bugs I've seen so far :
=> The right side the window sometimes gets cuts off
=> The content area can be cut-off after launching Firefox
=> ... and possibly more bugs, since this is not well tested

So if we go in that direction, we should probably do some platform to hide the default caption buttons first.

> It seems that all we'd need for this is the following SVG icons:
> - minimize
> - maximize
> - window
> - close (maybe a separate white version of that)
I've just attached a new version of those SVGs (The ones in the userstyle were made for an older build which had bigger icons). The sprite image contains close, minimize, maximize and restore icons. Feel free to tweak them to your liking :)

Comment 20

[Tim Nguyen :ntim]

Attachment: caption-buttons.svg

Tweaked the svg a bit

Comment 21

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] from comment #19)
> (In reply to Philipp Sackl [:phlsa] please use needinfo from comment #17)
> > So, since making the caption buttons transparent seems to be capital H Hard,
> > how about hiding them altogether and drawing our own buttons.
> This has a few cons (as seen in the userstyle) :
> - This requires the whole window to be redrawn (-moz-appearance: none;)
> unless we do some platform work that would allow us to hide the caption
> buttons without redrawing the whole window
> - Redrawing the whole window is fairly buggy, here are 2 known bugs I've
> seen so far :
> => The right side the window sometimes gets cuts off
> => The content area can be cut-off after launching Firefox
> => ... and possibly more bugs, since this is not well tested

I don't really understand any of these. We don't need to "redraw" the window, we just don't set its moz-appearance to glass, which means it gets moz-appearance "window", which works.

> So if we go in that direction, we should probably do some platform to hide
> the default caption buttons first.
> 
> > It seems that all we'd need for this is the following SVG icons:
> > - minimize
> > - maximize
> > - window
> > - close (maybe a separate white version of that)
> I've just attached a new version of those SVGs (The ones in the userstyle
> were made for an older build which had bigger icons). The sprite image
> contains close, minimize, maximize and restore icons. Feel free to tweak
> them to your liking :)


The problem with these is that they get pixel-snapped wrongly in all kinds of ways. AFAICT the "real" mininimize/restore icons always get drawn as lines/rects which are pixel-snapped to 1 device pixel (at least up to the 150% dpi that my surface supports). The close icon is harder to tell apart because the diagonal lines mean they get antialiased.

Either way, this effect is hard to achieve in CSS/SVG because we don't have a "device pixel" unit. :-\

I have a patch that has this approach working apart from:
- aforementioned pixel alignment issues with the images/SVGs
- sizing the buttons and their spacing correctly on both maximized/restored windows
- I've not checked what happens on fullscreen windows yet
- I'm seeing issues where, if the buttons are reasonably-high, showing the menubar somehow keeps the excess space above the tabs; I assume tabsintitlebar is getting confused there somehow, but I've not checked out exactly why.

I'll continue to tweak this approach this week. Philipp/Stephen, if you can get me exact measurements etc. for the buttons that would be helpful. Same for colours. We might also need to think about lwthemes - right now I'm using the accentcolor for the background of the buttons in that case on hover, but I'm not sure that there will be enough contrast in the non-hover case.

Comment 22

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #21)
> I'll continue to tweak this approach this week. Philipp/Stephen, if you can
> get me exact measurements etc. for the buttons that would be helpful. Same
> for colours. We might also need to think about lwthemes - right now I'm
> using the accentcolor for the background of the buttons in that case on
> hover, but I'm not sure that there will be enough contrast in the non-hover
> case.

Is this approach Windows 10 specific or will this apply everywhere?

Do we need PNGs instead of SVG for various scaling factors?

Would it be possible to get a try build of your WIP patch?

Comment 23

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #21)
> (In reply to Tim Nguyen [:ntim] from comment #19)
> > (In reply to Philipp Sackl [:phlsa] please use needinfo from comment #17)
> > > So, since making the caption buttons transparent seems to be capital H Hard,
> > > how about hiding them altogether and drawing our own buttons.
> > This has a few cons (as seen in the userstyle) :
> > - This requires the whole window to be redrawn (-moz-appearance: none;)
> > unless we do some platform work that would allow us to hide the caption
> > buttons without redrawing the whole window
> > - Redrawing the whole window is fairly buggy, here are 2 known bugs I've
> > seen so far :
> > => The right side the window sometimes gets cuts off
> > => The content area can be cut-off after launching Firefox
> > => ... and possibly more bugs, since this is not well tested
> 
> I don't really understand any of these. We don't need to "redraw" the
> window, we just don't set its moz-appearance to glass, which means it gets
> moz-appearance "window", which works.
Alright, didn't know that. One thing I've noticed is that the top border disappears with -moz-appearance: window. (-moz-appearance: none; also has this side effect).

> > So if we go in that direction, we should probably do some platform to hide
> > the default caption buttons first.
> > 
> > > It seems that all we'd need for this is the following SVG icons:
> > > - minimize
> > > - maximize
> > > - window
> > > - close (maybe a separate white version of that)
> > I've just attached a new version of those SVGs (The ones in the userstyle
> > were made for an older build which had bigger icons). The sprite image
> > contains close, minimize, maximize and restore icons. Feel free to tweak
> > them to your liking :)
> 
> 
> The problem with these is that they get pixel-snapped wrongly in all kinds
> of ways. AFAICT the "real" mininimize/restore icons always get drawn as
> lines/rects which are pixel-snapped to 1 device pixel (at least up to the
> 150% dpi that my surface supports). The close icon is harder to tell apart
> because the diagonal lines mean they get antialiased.
> 
> Either way, this effect is hard to achieve in CSS/SVG because we don't have
> a "device pixel" unit. :-\
You could try changing the stroke-width to 0.75px/0.5px so the stroke-width won't excess 1.5dp/1dp on higher DPIs. 0.5px would probably end up too thin on 100% DPI though. Maybe we can have 2 separate SVGs ?

Comment 24

[:Gijs (out for now; he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #22)
> (In reply to :Gijs Kruitbosch from comment #21)
> > I'll continue to tweak this approach this week. Philipp/Stephen, if you can
> > get me exact measurements etc. for the buttons that would be helpful. Same
> > for colours. We might also need to think about lwthemes - right now I'm
> > using the accentcolor for the background of the buttons in that case on
> > hover, but I'm not sure that there will be enough contrast in the non-hover
> > case.
> 
> Is this approach Windows 10 specific or will this apply everywhere?

Just Windows 10, though I could see it helping us on Win8, that's not really in-scope for this bug.

> Do we need PNGs instead of SVG for various scaling factors?

I don't see how that would help. If anything, SVG should be easier because we can adjust some of it using just CSS (as Tim has since mentioned).

> Would it be possible to get a try build of your WIP patch?

Yes. I've not actually tested the lwt stuff on this, I vaguely recall it needing more work. Trying to sort out the sizing/pixels first (today was a meeting-VPs-in-London day so I didn't get as much done as I would have liked). YMMV. Builds should be at https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-34371dca2975 . ( treeherder: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=34371dca2975 )



(In reply to Tim Nguyen [:ntim] from comment #23)
> One thing I've noticed is that the top border
> disappears with -moz-appearance: window. (-moz-appearance: none; also has
> this side effect).

Hm. I expect this has to do with chromemargin and/or widget. I'll look into it. Originally I ran into the caption button cutout, but I talked with jimm a few weeks back about the glass appearance and sidebar issues anyway, so I have a fix for that (it's a simple change, it's in the trypush) already. This might need similar changes. Thanks for flagging it, because I hadn't noticed it.

> You could try changing the stroke-w   1173732idth to 0.75px/0.5px so the stroke-width
> won't excess 1.5dp/1dp on higher DPIs. 0.5px would probably end up too thin
> on 100% DPI though.

Yes, but this is not enough, IME, though maybe I should try the mathematically correct (for my 150% screen) 0.6666(...)px before drawing conclusions. 0.5px still gets antialiasing artifacts on my machine (most noticeably on the restore/maximize buttons, but it happens with minimize as well).

> Maybe we can have 2 separate SVGs ?

Windows 10 allows scaling factors at 25% increments from 75% up to at least 250% (on phlsa's machine; mine stops at 225), so I don't think 2 would necessarily cut it. If it's just the stroke-width, we can see if the DPI media queries work in SVG's CSS, or just make lots of versions... but at that point I'd start being interested in more finnicky solutions, like data-uri'ing the SVG so that we can generate it in JS on first load and just do the math there, or something.

Comment 25

[Stephen Horlander [:shorlander]]

@media (min-resolution: *dppx) does work in SVG based on my (limited) testing.

Comment 26

[:Gijs (out for now; he/him)]

So the trypush didn't work, and now try is closed, so I just uploaded my own build: http://gijsk.com/temp/firefox-win10-xul-captions.zip .

Stephen, I'd really appreciate feedback re: what to do about the images here. On 150% dpi, they definitely get blurry, and just the stroke-width isn't enough to fix this, so it sounds like we basically need a bunch of separate images / parts to the SVG in order to get this right.

This patch fixes the titlebar sizing problem with the menubar, but I haven't had a chance to poke at the window border issue yet. I've also changed my approach for the lwtheme stuff because it turns out the accentcolors are largely disconnected from the image background, so I just went with something half-transparent and hopefully-vaguely-sensible. YMMV (but let me know!).

Comment 27

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #26)
> So the trypush didn't work, and now try is closed, so I just uploaded my own
> build: http://gijsk.com/temp/firefox-win10-xul-captions.zip .
> 
> Stephen, I'd really appreciate feedback re: what to do about the images
> here. On 150% dpi, they definitely get blurry, and just the stroke-width
> isn't enough to fix this, so it sounds like we basically need a bunch of
> separate images / parts to the SVG in order to get this right.
Maybe shape-rendering: crispEdges will fix the blurriness ?

Comment 28

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] from comment #27)
> (In reply to :Gijs Kruitbosch from comment #26)
> > So the trypush didn't work, and now try is closed, so I just uploaded my own
> > build: http://gijsk.com/temp/firefox-win10-xul-captions.zip .
> > 
> > Stephen, I'd really appreciate feedback re: what to do about the images
> > here. On 150% dpi, they definitely get blurry, and just the stroke-width
> > isn't enough to fix this, so it sounds like we basically need a bunch of
> > separate images / parts to the SVG in order to get this right.
> Maybe shape-rendering: crispEdges will fix the blurriness ?

This looks like it works, thanks! I will look at this and the top-of-the-window stuff tomorrow.

Leaving ni on Stephen for a general checking of whether this looks OK / like what you were expecting.

Comment 29

[:Gijs (out for now; he/him)]

FWIW, Stephen, it seems like your design spec update here requires different images for the caption buttons when using lwthemes. It would be great if you could attach those.

Comment 30

[Stephen Horlander [:shorlander]]

Attachment: Caption Button Specs - i01

Specs for caption buttons.

Note: Hover and pressed states would show up individually per button not all at once :)

(In reply to :Gijs Kruitbosch from comment #29)
> FWIW, Stephen, it seems like your design spec update here requires different
> images for the caption buttons when using lwthemes. It would be great if you
> could attach those.

Will post those tomorrow.

Comment 31

[Stephen Horlander [:shorlander]]

Attachment: Caption Button Specs - i02

Updated specs to remove button spacing and add maximized dimensions.

Comment 32

[:Gijs (out for now; he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #25)
> @media (min-resolution: *dppx) does work in SVG based on my (limited)
> testing.

Not when the SVG is used as a background-image/list-style-image inside a XUL element, AFAICT... sigh.

Comment 33

[:Gijs (out for now; he/him)]

Comment on attachment 8629390 [details]
MozReview Request: Bug 1173725 - part 1: force top border to be visible on windows 10 and don't cut out caption buttons, r?jimm

Bug 1173725 - fix caption button cutout to only apply when using moz-appearance: *glass*, r?jimm

Comment 34

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: Bug 1173725 - part 2: make TabsInTitlebar logic remove margin bottom on titlebar after menubar goes away, r?dao

Bug 1173725 - make TabsInTitlebar logic remove margin bottom on titlebar after menubar goes away, r?dao

Comment 35

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

Bug 1173725 - use XUL for titlebar buttons, r?dao,ui-r=shorlander

Comment 36

[:Gijs (out for now; he/him)]

Known issues:
1) man, that CSS/SVG stroke width stuff is ugly. :-(
2) the missing top border. I don't currently have a solution for this and I don't think it's worth postponing submitting the current state for review because of it.

Comment 37

[:Gijs (out for now; he/him)]

Comment on attachment 8632090 [details]
MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

Stephen, I'd love to know if you think the current top-border-less state is shippable or not.

Comment 38

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #37)
> Comment on attachment 8632090 [details]
> MozReview Request: Bug 1173725 - use XUL for titlebar buttons,
> r?dao,ui-r=shorlander
> 
> Stephen, I'd love to know if you think the current top-border-less state is
> shippable or not.

Do you mean the way we are covering up the top highlighted native window border? No I don't think we should ship with that.

Comment 39

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #36)
> Known issues:
> 1) man, that CSS/SVG stroke width stuff is ugly. :-(
> 2) the missing top border. I don't currently have a solution for this and I
> don't think it's worth postponing submitting the current state for review
> because of it.

For some reason, the top border is properly shown with lw-themes.

Comment 40

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #32)
> (In reply to Stephen Horlander [:shorlander] from comment #25)
> > @media (min-resolution: *dppx) does work in SVG based on my (limited)
> > testing.
> 
> Not when the SVG is used as a background-image/list-style-image inside a XUL
> element, AFAICT... sigh.

Are you trying to specify the rules in the SVG? Would using the @media rules in the browser CSS and picking a different url target work?

Like we are doing with InContent Prefs icons: http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/incontentprefs/preferences.inc.css#45

e.g.

@media (min-resolution: 2dppx) {
  #category-general > .category-icon {
    list-style-image: url("chrome://browser/skin/preferences/in-content/icons.svg#general@2x");
  }
}

Comment 41

[:Gijs (out for now; he/him)]

(In reply to Stephen Horlander [:shorlander] (Away until 7/20) from comment #38)
> (In reply to :Gijs Kruitbosch from comment #37)
> > Comment on attachment 8632090 [details]
> > MozReview Request: Bug 1173725 - use XUL for titlebar buttons,
> > r?dao,ui-r=shorlander
> > 
> > Stephen, I'd love to know if you think the current top-border-less state is
> > shippable or not.
> 
> Do you mean the way we are covering up the top highlighted native window
> border? No I don't think we should ship with that.

It's not so much "covering up", that implies there is something there to cover up...

Comment 42

[:Gijs (out for now; he/him)]

(In reply to Stephen Horlander [:shorlander] (Away until 7/20) from comment #40)
> (In reply to :Gijs Kruitbosch from comment #32)
> > (In reply to Stephen Horlander [:shorlander] from comment #25)
> > > @media (min-resolution: *dppx) does work in SVG based on my (limited)
> > > testing.
> > 
> > Not when the SVG is used as a background-image/list-style-image inside a XUL
> > element, AFAICT... sigh.
> 
> Are you trying to specify the rules in the SVG? Would using the @media rules
> in the browser CSS and picking a different url target work?

Sure, this is what I'm doing right now, but it's ugly because it means I have to create 3 different <use> targets for every (5) shapes, plus the corresponding CSS rules.


(In reply to Tim Nguyen [:ntim] (away 11-14 July) from comment #39)
> (In reply to :Gijs Kruitbosch from comment #36)
> > Known issues:
> > 1) man, that CSS/SVG stroke width stuff is ugly. :-(
> > 2) the missing top border. I don't currently have a solution for this and I
> > don't think it's worth postponing submitting the current state for review
> > because of it.
> 
> For some reason, the top border is properly shown with lw-themes.

On Windows 10, with these patches? Or without? Without, the lwthemes will get the native buttons, which causes problems. The problem is not wanting Windows to paint the buttons but still wanting it to render the top border. There seems to be no way to do that (not just "we don't expose the magical widget stuff", but like, even if I try to change the non-client area so as to still render 1px of something, Windows decides to just give us a full titlebar). (Jim? Am I missing something?)

Comment 43

[Richard Marti (:Paenglab)]

(In reply to :Gijs Kruitbosch from comment #42)
> 
> Sure, this is what I'm doing right now, but it's ugly because it means I
> have to create 3 different <use> targets for every (5) shapes, plus the
> corresponding CSS rules.

In bug 1153615 comment 7, 9 and following Robert Longson wrote <uses> shouldn't be used because it uses a lot of memory: "At the moment use clones each item so your memory cost is going to be high, that's why you want to avoid it."

Maybe you can use a similar way I used in my bug and bug 1150627, which should be, referring to Robert, the right way.

Comment 44

[:Gijs (out for now; he/him)]

(In reply to Richard Marti (:Paenglab) from comment #43)
> (In reply to :Gijs Kruitbosch from comment #42)
> > 
> > Sure, this is what I'm doing right now, but it's ugly because it means I
> > have to create 3 different <use> targets for every (5) shapes, plus the
> > corresponding CSS rules.
> 
> In bug 1153615 comment 7, 9 and following Robert Longson wrote <uses>
> shouldn't be used because it uses a lot of memory: "At the moment use clones
> each item so your memory cost is going to be high, that's why you want to
> avoid it."
> 
> Maybe you can use a similar way I used in my bug and bug 1150627, which
> should be, referring to Robert, the right way.

This would mean duplicating all the paths, which seems like it'd be even uglier, nevermind memory usage probably not improving if I actually duplicate all the paths... Robert, what am I missing here?

Comment 45

[Robert Longson [:longsonr]]

use clones everything and then makes the clones track the original content. If you don't need that then duplicating the content is the way to go as it will perform better.

Comment 46

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Gijs, I see that you have Stephen up for a ui-r on a patch here and he's out this week.
I'd be happy to do the review instead of him, but I'd need a build for download (can't do Windows builds at the moment...)

Comment 47

[:Gijs (out for now; he/him)]

(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #46)
> Gijs, I see that you have Stephen up for a ui-r on a patch here and he's out
> this week.
> I'd be happy to do the review instead of him, but I'd need a build for
> download (can't do Windows builds at the moment...)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a4071a3b1e5

Comment 48

[:Gijs (out for now; he/him)]

Comment on attachment 8632090 [details]
MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

Note that I'm out for 2 weeks after this evening, so it'd be helpful if we can iterate quickly here.

One thing I forgot to mention is that I've not implemented the lwtheme difference because I have no icons to do that with. :-)

Comment 49

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #48)
> Comment on attachment 8632090 [details]
> MozReview Request: Bug 1173725 - use XUL for titlebar buttons,
> r?dao,ui-r=shorlander
> 
> Note that I'm out for 2 weeks after this evening, so it'd be helpful if we
> can iterate quickly here.

Ugh, fail. Let me try again. I'm out for 2 weeks after this *week*, ie Friday is my last day until August.

Comment 50

[Jim Mathies [:jimm]]

Comment on attachment 8629390 [details]
MozReview Request: Bug 1173725 - part 1: force top border to be visible on windows 10 and don't cut out caption buttons, r?jimm

https://reviewboard.mozilla.org/r/12601/#review11719

::: widget/windows/nsWindow.cpp:3577
(Diff revision 2)
> +  if (HasGlass()) {

Why don't we just early return here, there's no need to set the clip.

if (!HasGlass()) {
  return
}

Testing on Win7 I see no side effects from this, looks like a good optimization.

Comment 51

[Jim Mathies [:jimm]]

Comment on attachment 8629390 [details]
MozReview Request: Bug 1173725 - part 1: force top border to be visible on windows 10 and don't cut out caption buttons, r?jimm

bleh, mozreview never does what you want it too.

Comment 52

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] (away 11-14 July) from comment #39)
> (In reply to :Gijs Kruitbosch from comment #36)
> > Known issues:
> > 1) man, that CSS/SVG stroke width stuff is ugly. :-(
> > 2) the missing top border. I don't currently have a solution for this and I
> > don't think it's worth postponing submitting the current state for review
> > because of it.
> 
> For some reason, the top border is properly shown with lw-themes.

I just checked this, and I don't see this at all (even on builds without the patch). I just see a border on the cutout for the titlebar buttons, and our own fake border rendering (still the glass one from vista+ that we kept on win8 and now win10). I guess one option for this might therefore be forcing a cutout of the 1px top border on Windows 10 instead... I'd have to experiment with that though.

Comment 53

[Richard Marti (:Paenglab)]

Attachment: noAeroTitlebar.png

On Win7 I'm seeing no Glass titlebar. Without this patch the Glass is back.
Tested with my self built build and the one from Try.

Comment 54

[:Gijs (out for now; he/him)]

(In reply to Richard Marti (:Paenglab) from comment #53)
> Created attachment 8632857 [details]
> noAeroTitlebar.png
> 
> On Win7 I'm seeing no Glass titlebar. Without this patch the Glass is back.
> Tested with my self built build and the one from Try.

Which patch? Presumably this is all 3 patches? Or not?

Comment 55

[Richard Marti (:Paenglab)]

Yes, all patches. As they are also in Try build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a4071a3b1e5

Comment 56

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #54)
> (In reply to Richard Marti (:Paenglab) from comment #53)
> > Created attachment 8632857 [details]
> > noAeroTitlebar.png
> > 
> > On Win7 I'm seeing no Glass titlebar. Without this patch the Glass is back.
> > Tested with my self built build and the one from Try.
> 
> Which patch? Presumably this is all 3 patches? Or not?

I didn't see that with just the HasGlass patch I reviewed.

Comment 57

[Richard Marti (:Paenglab)]

Jim, could you try the try build? Only to be sure it's not only on my machine.

Comment 58

[Jim Mathies [:jimm]]

> On Windows 10, with these patches? Or without? Without, the lwthemes will
> get the native buttons, which causes problems. The problem is not wanting
> Windows to paint the buttons but still wanting it to render the top border.
> There seems to be no way to do that (not just "we don't expose the magical
> widget stuff", but like, even if I try to change the non-client area so as
> to still render 1px of something, Windows decides to just give us a full
> titlebar). (Jim? Am I missing something?)

Yes the top border numbers other than -1 and 0 don't work as you might expect. Really it's an unsupported config since it's hard to predict how the os will react. Windows 7 paints the full titlebar when we set the top value to anything other than 0. We might be able to address this with some heavy hacking down in win32 widget, I wouldn't suggest relying on that here though.

Comment 59

[Jim Mathies [:jimm]]

(In reply to Richard Marti (:Paenglab) from comment #57)
> Jim, could you try the try build? Only to be sure it's not only on my
> machine.

I can reproduce this with the try build - I see the fog running all the way up, and there's no top border on the window.

Comment 60

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #59)
> (In reply to Richard Marti (:Paenglab) from comment #57)
> > Jim, could you try the try build? Only to be sure it's not only on my
> > machine.
> 
> I can reproduce this with the try build - I see the fog running all the way
> up, and there's no top border on the window.

Right. I just checked, and this is just because I forgot to put back background-color: transparent for glass.

I'm going to try to see if I can get a cut-out of just the top of the window to work for getting a window border back though. Just not entirely sure how to get the coordinates for the rect (what units is that clearing rect in anyway?) and logging has so far proved ineffective in terms of finding out. Might be a job for tomorrow, but hints appreciated if you have them before then, Jim.

Comment 61

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #60)
> I'm going to try to see if I can get a cut-out of just the top of the window
> to work for getting a window border back though. Just not entirely sure how
> to get the coordinates for the rect (what units is that clearing rect in
> anyway?) and logging has so far proved ineffective in terms of finding out.
> Might be a job for tomorrow, but hints appreciated if you have them before
> then, Jim.

The units are device pixels. Clipping the top should be easy just add a new clear rect that's

(bounds.X(), bounds.Y(), bounds.Width(), bounds.Height() - 1.0)

which is the top pixel of the window.

Comment 62

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #61)
> (In reply to :Gijs Kruitbosch from comment #60)
> > I'm going to try to see if I can get a cut-out of just the top of the window
> > to work for getting a window border back though. Just not entirely sure how
> > to get the coordinates for the rect (what units is that clearing rect in
> > anyway?) and logging has so far proved ineffective in terms of finding out.
> > Might be a job for tomorrow, but hints appreciated if you have them before
> > then, Jim.
> 
> The units are device pixels. Clipping the top should be easy just add a new
> clear rect that's
> 
> (bounds.X(), bounds.Y(), bounds.Width(), bounds.Height() - 1.0)
> 
> which is the top pixel of the window.

Hmm I guess this is a bit trickier, I was looking at the current rect in the debugger, it matches the dimensions of the cutout for the command buttons. So the above bounds values wont work.

Comment 63

[Jim Mathies [:jimm]]

For the command buttons we rely on a certain widget that's tagged to handle the clipping. I think you can get away with adding your clipping area at the same time we do the command buttons.

Comment 64

[Jim Mathies [:jimm]]

RECT rect;
::GetWindowRect(mWnd, &rect);
clearRegion.Or(clearRegion, nsIntRect(0, 0, rect.right - rect.left, 5.0));

This little experiment worked for clipping the top of the fog on windows 7.

Comment 65

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8632090 [details]
MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

Thanks for the build Gijs!
I do see two blocking issues and one non-blocking:

Blocking:
1) Only the close button shows up to me (no icons visible for the minimize and maximize buttons). It seems that just the icons are missing, since the hover effects and click targets for all buttons seem fine.
2) The top border of the window is missing, but it sounds like you're well aware of that. I don't think we can go without this. If I understand this correctly, jmathies has a proposal on how to fix that. Let me know if it doesn't work - I'll try some last-resort design solutions...

Non-blocking:
3) The icons seem a bit thinner than on other applications when the window is focused. Looks like Windows is altering the stroke weight depending on whether a window has focus or not.

Comment 66

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #64)
> RECT rect;
> ::GetWindowRect(mWnd, &rect);
> clearRegion.Or(clearRegion, nsIntRect(0, 0, rect.right - rect.left, 5.0));
> 
> This little experiment worked for clipping the top of the fog on windows 7.

This works, but only when the window is using a glass -moz-appearance. On non-glass, the area gets painted black.

When I use a glass -moz-appearance, and Windows paints the titlebar, it also eats the hittest/mouse events for the caption buttons, which means we don't get hover/clicks on those parts of the buttons that overlap the Windows caption buttons.

The documentation here says we really ought to be passing these messages to windows first so it can handle hittesting the caption buttons... but seeing as we're not actually showing those, it seems like it might be OK to not do that? Testing that now.

Comment 67

[Dão Gottwald [:dao]]

Lots of recent activity. Are those patches from last week actually ready for review?

Comment 68

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #67)
> Lots of recent activity. Are those patches from last week actually ready for
> review?

The browser JS (part 2, as it were), definitely.

I'd appreciate feedback on part 3, but it will change a little bit because of the change in approach wrt glass or non-glass because of the top border.

Comment 69

[:Gijs (out for now; he/him)]

(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #65)
> Comment on attachment 8632090 [details]
> MozReview Request: Bug 1173725 - use XUL for titlebar buttons,
> r?dao,ui-r=shorlander
> 
> Thanks for the build Gijs!
> I do see two blocking issues and one non-blocking:
> 
> Blocking:
> 1) Only the close button shows up to me (no icons visible for the minimize
> and maximize buttons). It seems that just the icons are missing, since the
> hover effects and click targets for all buttons seem fine.

No, the icons are not missing. I expect that the stroke-width values are breaking (ie causing the strokes to be so narrow so as to not display at all on your DPI setting). Two questions:

1) What DPI setting are you using?

2) What happens (on a build with the patch) when you open:

chrome://browser/skin/caption-buttons.svg#minimize

chrome://browser/skin/caption-buttons.svg#minimize-half-stroke

chrome://browser/skin/caption-buttons.svg#minimize-third-stroke

in a tab? Which of them can you see, and which look right?

Comment 70

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #66)
> (In reply to Jim Mathies [:jimm] from comment #64)
> > RECT rect;
> > ::GetWindowRect(mWnd, &rect);
> > clearRegion.Or(clearRegion, nsIntRect(0, 0, rect.right - rect.left, 5.0));
> > 
> > This little experiment worked for clipping the top of the fog on windows 7.
> 
> This works, but only when the window is using a glass -moz-appearance. On
> non-glass, the area gets painted black.

Well that makes sense right? Without transparency you have to paint pixels in those areas otherwise you end up with whatever the default background is.

> 
> When I use a glass -moz-appearance, and Windows paints the titlebar, it also
> eats the hittest/mouse events for the caption buttons, which means we don't
> get hover/clicks on those parts of the buttons that overlap the Windows
> caption buttons.

With my experiment, the titlebar wasn't painted. There may have been some problems with the glass command buttons painted and maintained by Windows compositor, but I didn't notice. But it sounds like you are trying to turn the glass area in the titlebar on and use xul based command buttons?

> The documentation here says we really ought to be passing these messages to
> windows first so it can handle hittesting the caption buttons... but seeing
> as we're not actually showing those, it seems like it might be OK to not do
> that? Testing that now.

This is built in when glass is enabled -

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#4698

I remember this behavior as being very touchy code in Windows.

Comment 71

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #70)
> (In reply to :Gijs Kruitbosch from comment #66)
> > When I use a glass -moz-appearance, and Windows paints the titlebar, it also
> > eats the hittest/mouse events for the caption buttons, which means we don't
> > get hover/clicks on those parts of the buttons that overlap the Windows
> > caption buttons.
> 
> With my experiment, the titlebar wasn't painted.

I mean, the fog was cut off, but presumably on win7 aero you could see glass and the caption buttons, right? :-)

> There may have been some
> problems with the glass command buttons painted and maintained by Windows
> compositor, but I didn't notice. But it sounds like you are trying to turn
> the glass area in the titlebar on and use xul based command buttons?

Yes.

> > The documentation here says we really ought to be passing these messages to
> > windows first so it can handle hittesting the caption buttons... but seeing
> > as we're not actually showing those, it seems like it might be OK to not do
> > that? Testing that now.
> 
> This is built in when glass is enabled -
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.
> cpp#4698

Right. If I make an exception here, requiring that !(IsWin10OrLater() && HasGlass()), things behave as expected.

> I remember this behavior as being very touchy code in Windows.

I did not see any problems, but if you have any recollection as to what issues you remember seeing that would be helpful.

Comment 72

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #71)
> (In reply to Jim Mathies [:jimm] from comment #70)
> > (In reply to :Gijs Kruitbosch from comment #66)
> > > When I use a glass -moz-appearance, and Windows paints the titlebar, it also
> > > eats the hittest/mouse events for the caption buttons, which means we don't
> > > get hover/clicks on those parts of the buttons that overlap the Windows
> > > caption buttons.
> > 
> > With my experiment, the titlebar wasn't painted.
> 
> I mean, the fog was cut off, but presumably on win7 aero you could see glass
> and the caption buttons, right? :-)

Yes, since the command buttons are drawn by the Windows compositor they show up provided the window style requires them and the upper area of the window has transparency filling the entire area the buttons paint to.
 
> > There may have been some
> > problems with the glass command buttons painted and maintained by Windows
> > compositor, but I didn't notice. But it sounds like you are trying to turn
> > the glass area in the titlebar on and use xul based command buttons?
> 
> Yes.

Windows will try to paint the command buttons here unless we paint something opaque all the way up to the top of the window. It's an unfortunately side effect of the way they implemented this.

https://bug513162.bugzilla.mozilla.org/attachment.cgi?id=444597

If needed changes could be made to turn the compositor drawn command buttons off for the main browser window. We would have to remove WS_MINIMIZEBOX, WS_MAXIMIZEBOX, and the WS_SYSMENU styles but it can be done.

Comment 73

[Jim Mathies [:jimm]]

Just out of curiosity, why are you keeping glass mode here? It looks like window chrome is opaque on win10.

Comment 74

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #73)
> Just out of curiosity, why are you keeping glass mode here? It looks like
> window chrome is opaque on win10.

Because otherwise Windows doesn't paint the titlebar, which means there is no top border to the window.

(In reply to Jim Mathies [:jimm] from comment #72)
> (In reply to :Gijs Kruitbosch from comment #71)
> > (In reply to Jim Mathies [:jimm] from comment #70)
> > > (In reply to :Gijs Kruitbosch from comment #66)
> > > > When I use a glass -moz-appearance, and Windows paints the titlebar, it also
> > > > eats the hittest/mouse events for the caption buttons, which means we don't
> > > > get hover/clicks on those parts of the buttons that overlap the Windows
> > > > caption buttons.
> > > 
> > > With my experiment, the titlebar wasn't painted.
> > 
> > I mean, the fog was cut off, but presumably on win7 aero you could see glass
> > and the caption buttons, right? :-)
> 
> Yes, since the command buttons are drawn by the Windows compositor they show
> up provided the window style requires them and the upper area of the window
> has transparency filling the entire area the buttons paint to.
>  
> > > There may have been some
> > > problems with the glass command buttons painted and maintained by Windows
> > > compositor, but I didn't notice. But it sounds like you are trying to turn
> > > the glass area in the titlebar on and use xul based command buttons?
> > 
> > Yes.
> 
> Windows will try to paint the command buttons here unless we paint something
> opaque all the way up to the top of the window. It's an unfortunately side
> effect of the way they implemented this.
> 
> https://bug513162.bugzilla.mozilla.org/attachment.cgi?id=444597
> 
> If needed changes could be made to turn the compositor drawn command buttons
> off for the main browser window. We would have to remove WS_MINIMIZEBOX,
> WS_MAXIMIZEBOX, and the WS_SYSMENU styles but it can be done.

So with the patch I'm about to put up, we just paint over the top of these, and this works. Removing the system menu seemed too high-risk to me.

Comment 75

[:Gijs (out for now; he/him)]

Comment on attachment 8629390 [details]
MozReview Request: Bug 1173725 - part 1: force top border to be visible on windows 10 and don't cut out caption buttons, r?jimm

Bug 1173725 - part 1: force top border to be visible on windows 10 and don't cut out caption buttons, r?jimm

Comment 76

[:Gijs (out for now; he/him)]

Comment on attachment 8632089 [details]
MozReview Request: Bug 1173725 - part 2: make TabsInTitlebar logic remove margin bottom on titlebar after menubar goes away, r?dao

Bug 1173725 - part 2: make TabsInTitlebar logic remove margin bottom on titlebar after menubar goes away, r?dao

Comment 77

[:Gijs (out for now; he/him)]

Comment on attachment 8632090 [details]
MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=shorlander

Comment 78

[:Gijs (out for now; he/him)]

Trypush for testing / ui-review: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbdc1f443018

Comment 79

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #69)
> (In reply to Philipp Sackl [:phlsa] please use needinfo from comment #65)
> 1) What DPI setting are you using?
The issue occurs at 200% scaling on my machine. It doesn't occur at 250% (the recommended factor for this machine). Interestingly enough, it takes a signout in order for scale factor changes to take effect here: the icons were still gone at 250% before signing out and they were visible at 200% before signing out.

> 2) What happens (on a build with the patch) when you open:
> 
> chrome://browser/skin/caption-buttons.svg#minimize
Visible at 200%. Looks right.

> chrome://browser/skin/caption-buttons.svg#minimize-half-stroke
Invisible at 200%

> chrome://browser/skin/caption-buttons.svg#minimize-third-stroke
Invisible at 200%

Comment 80

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8632090 [details]
MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

This fixes the border issue, which is great!

The icon issue still persists though (see comment #79 for details).
So here we are...

Blockers:
- Icons disappearing

Non-blockers / follow ups:
- Icon thickness
- I think the top border has a similar issue where it is thinner than the other borders in some scale factors.


One additional note: I can't test at 100% (which is obviously important) until tomorrow, but I'll let you know how that goes.

Comment 81

[Justin Dolske [:Dolske]]

(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #79)

> > 2) What happens (on a build with the patch) when you open:
> > 
> > chrome://browser/skin/caption-buttons.svg#minimize
> Visible at 200%. Looks right.
> 
> > chrome://browser/skin/caption-buttons.svg#minimize-half-stroke
> Invisible at 200%
> 
> > chrome://browser/skin/caption-buttons.svg#minimize-third-stroke
> Invisible at 200%

On my desktop (caveat: Win10 build 10130, but shouldn't matter here), I also see invisible minimize/maximize controls at 200%. Although #minimize and #minimize-half-stroke are both visible for me -- the former is two device pixels thick, the latter 1 device pixel. #minimize-third-stroke is invisible.

Comment 82

[Justin Dolske [:Dolske]]

Attachment: Screenshot of line at top with LWT

Also, using the same try build from comment 78, with a LWT I see 2 extra rows of physical pixels, making the top window border look too thick. The normal window border is there, it's the two rows under it. Known?

Comment 83

[:Gijs (out for now; he/him)]

(In reply to Justin Dolske [:Dolske] from comment #82)
> Created attachment 8633783 [details]
> Screenshot of line at top with LWT
> 
> Also, using the same try build from comment 78, with a LWT I see 2 extra
> rows of physical pixels, making the top window border look too thick. The
> normal window border is there, it's the two rows under it. Known?

No, but this is just our artificial border drawing for lwthemes that we're using on windows 10 and that needs to be put back in its box, err, stuck in a media query. I have a (simple) fix for this locally, but I need to sort out the hidpi situation with the images as well.

So after I've done some more testing:

1) Windows uses a 1-device-pixel stroke width on all of 100, 125, 150 and 175% size/dpi levels
2) Windows uses a 2-device-pixel stroke width on 200 and 225%, and I expect the same on 250 and 275%, and 3 device pixels on 300%, and so on.

but also:

1) the caption button images are 10px wide on *both* 100 and 125%
2) 15px on 150 and 175%
3) 20px on 200 and 225% (and I expect 25 on 250 and 275%, 30 on 300%, and so on)

This makes getting this right everywhere a bit of a juggle. I'll work on this, but I'll also need to check if the outer sizes of the caption buttons do the same thing (I expect so, but I'd like to be sure). If so, that will make for "interesting" interactions with the tabstrip.

Comment 84

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #83)
> 1) the caption button images are 10px wide on *both* 100 and 125%

So I was wrong about this. They're 12 device pixels wide on 125.

> 2) 15px on 150 and 175%

but this is correct

> 3) 20px on 200 and 225% (and I expect 25 on 250 and 275%, 30 on 300%, and so
> on)

and so is this.

The size of the caption buttons *on modern apps* matches these sizes: 46 device pixels wide on 100, 57 on 125, 69 on 150, still 69 on 175, etc.

The size of the caption buttons on *classic apps* doesn't match this, they scale completely with the scaling factor (so at 175 dpi there is a visible discrepancy in the padding (and therefore in the "gap" when not hovering) between the icons). The same happens at 225% dpi.

> This makes getting this right everywhere a bit of a juggle. I'll work on
> this, but I'll also need to check if the outer sizes of the caption buttons
> do the same thing (I expect so, but I'd like to be sure). If so, that will
> make for "interesting" interactions with the tabstrip.

This times two, I guess. :-\

Comment 85

[Jim Mathies [:jimm]]

For the dpi differences you can try multiplying that top border value by nsIWidget's GetDefaultScale() and round up or down to get to a reliable value.

Comment 86

[Dão Gottwald [:dao]]

Comment on attachment 8632089 [details]
MozReview Request: Bug 1173725 - part 2: make TabsInTitlebar logic remove margin bottom on titlebar after menubar goes away, r?dao

Why the #ifndef XP_MACOSX change? Can you drop 'if (titlebarContent.style.marginBottom)'?

Comment 87

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #86)
> Comment on attachment 8632089 [details]
> MozReview Request: Bug 1173725 - part 2: make TabsInTitlebar logic remove
> margin bottom on titlebar after menubar goes away, r?dao
> 
> Why the #ifndef XP_MACOSX change?

Because we're trying to move away from ifdefs so we don't need to preprocess so much frontend code and can run automated tools on them more easily? This was in the line of fire, so I changed it. I can revert if you feel very strongly...

> Can you drop 'if
> (titlebarContent.style.marginBottom)'?

I mean, I could, but if there isn't a margin bottom then calling removeProperty won't do anything. Would you prefer I remove it? (Why?)

Comment 88

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Just tested the last build on a 100% display and it looks great!

Comment 89

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #87)
> (In reply to Dão Gottwald [:dao] from comment #86)
> > Comment on attachment 8632089 [details]
> > MozReview Request: Bug 1173725 - part 2: make TabsInTitlebar logic remove
> > margin bottom on titlebar after menubar goes away, r?dao
> > 
> > Why the #ifndef XP_MACOSX change?
> 
> Because we're trying to move away from ifdefs so we don't need to preprocess
> so much frontend code and can run automated tools on them more easily? This
> was in the line of fire, so I changed it. I can revert if you feel very
> strongly...

I don't see how this is supposed to work in browser.js which is intentionally composed of multiple files. It's also off-topic here, and since we'll want to uplift this let's please stay focused.

> > Can you drop 'if
> > (titlebarContent.style.marginBottom)'?
> 
> I mean, I could, but if there isn't a margin bottom then calling
> removeProperty won't do anything. Would you prefer I remove it? (Why?)

Exactly, it won't do anything, so why guard against it? Yes, I'd prefer getting rid of this check.

Comment 90

[:Gijs (out for now; he/him)]

Comment on attachment 8632089 [details]
MozReview Request: Bug 1173725 - part 2: make TabsInTitlebar logic remove margin bottom on titlebar after menubar goes away, r?dao

Bug 1173725 - part 2: make TabsInTitlebar logic remove margin bottom on titlebar after menubar goes away, r?dao

Comment 91

[:Gijs (out for now; he/him)]

Comment on attachment 8632090 [details]
MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

Comment 92

[:Gijs (out for now; he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee7a067c1635 will have updated builds.

I think this should work well up to and including 250% DPI. After that, YMMV, but I'd really like to push that out to a separate bug, if we do anything special at all.

Comment 93

[Dão Gottwald [:dao]]

Comment on attachment 8632090 [details]
MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

This is also using the gray background for high-contrast. Fixing this might be as simple as using the windows-default-theme media query in the right places.

Comment 94

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #93)
> Comment on attachment 8632090 [details]
> MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons,
> r?dao,ui-r=phlsa
> 
> This is also using the gray background for high-contrast. Fixing this might
> be as simple as using the windows-default-theme media query in the right
> places.

It isn't, though, because even if we removed the background, we'd then run the risk of the caption buttons being invisible on dark themes if the default window background is dark. Because the caption buttons aren't part of a toolbar, we don't do the same kind of "brighttext" detection as we do for toolbars, and I'd have to write that and add inverted variants to the SVG and use those. On top of that, it will complicate dealing with bug 1173728.

I (or someone, when I'm on PTO...) can do all that, but I'd like to not block the initial landing on it - the contrast of black-on-hsl(0,0%,78%) is very good (12:1, against the WCAG minimum of 7:1 for small text), and it remains easy to toggle the "normal" titlebar back on. On top of that, users of these themes are unlikely to correlate highly with jumping on Windows 10 the moment it is released (among other reasons: because a11y in Edge is apparently horrible). So, please can we deal with that in a followup?

Comment 95

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8632090 [details]
MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

This is great!
Tested on 100%, 150%, 200% and 250% and everything works everywhere.

There's still the issue that the top border is slightly thinner when running at 200%, but I'll file a follow-up for that.

Comment 96

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch (away Jul 18 - Aug 2) from comment #94)
> (In reply to Dão Gottwald [:dao] from comment #93)
> > Comment on attachment 8632090 [details]
> > MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons,
> > r?dao,ui-r=phlsa
> > 
> > This is also using the gray background for high-contrast. Fixing this might
> > be as simple as using the windows-default-theme media query in the right
> > places.
> 
> It isn't, though, because even if we removed the background, we'd then run
> the risk of the caption buttons being invisible on dark themes if the
> default window background is dark. Because the caption buttons aren't part
> of a toolbar, we don't do the same kind of "brighttext" detection as we do
> for toolbars, and I'd have to write that and add inverted variants to the
> SVG and use those. On top of that, it will complicate dealing with bug
> 1173728.

Can't you just use a native color like -moz-dialogText as the stroke color?

> I (or someone, when I'm on PTO...) can do all that, but I'd like to not
> block the initial landing on it - the contrast of black-on-hsl(0,0%,78%) is
> very good (12:1, against the WCAG minimum of 7:1 for small text), and it
> remains easy to toggle the "normal" titlebar back on. On top of that, users
> of these themes are unlikely to correlate highly with jumping on Windows 10
> the moment it is released (among other reasons: because a11y in Edge is
> apparently horrible). So, please can we deal with that in a followup?

While the contrast may be sufficient, it looks rather horrendous, such that I just assumed you completely missed this. I'd really feel more comfortable addressing this upfront before we land (and, more importantly, uplift) this.

Comment 97

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #96)
> (In reply to :Gijs Kruitbosch (away Jul 18 - Aug 2) from comment #94)
> > (In reply to Dão Gottwald [:dao] from comment #93)
> > > Comment on attachment 8632090 [details]
> > > MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons,
> > > r?dao,ui-r=phlsa
> > > 
> > > This is also using the gray background for high-contrast. Fixing this might
> > > be as simple as using the windows-default-theme media query in the right
> > > places.
> > 
> > It isn't, though, because even if we removed the background, we'd then run
> > the risk of the caption buttons being invisible on dark themes if the
> > default window background is dark. Because the caption buttons aren't part
> > of a toolbar, we don't do the same kind of "brighttext" detection as we do
> > for toolbars, and I'd have to write that and add inverted variants to the
> > SVG and use those. On top of that, it will complicate dealing with bug
> > 1173728.
> 
> Can't you just use a native color like -moz-dialogText as the stroke color?

Judging by how media queries don't work, I'm not sure this propagates environment stuff correctly. I can try, I guess.

Comment 98

[Dão Gottwald [:dao]]

We're already using platform colors in other SVGs, e.g. http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/icons/close.svg

Comment 99

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #98)
> We're already using platform colors in other SVGs, e.g.
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> icons/close.svg

So -moz-dialogtext is yellow on hc #1, but the caption buttons should be white.

I tried using -moz-fieldtext, which had the same problem, captiontext, which sometimes just doesn't match (black vs. white) and settled on buttontext, which works.

However, the highlight of these buttons uses the Highlight background on hc, and highlighttext obviously does not always correspond to buttontext (notably doesn't for hc black and hc white).

I tried setting the text color in CSS and using currentColor in SVG, but that didn't work.

So in the end I added another 3 "use" elements and used highlighttext for that and close-inverted (which I renamed), because on the default theme, highglighttext is also white, so that worked for close on the default theme. I'm confused as to why it does work, though, because highlighted menuitems in most types of context menus are still using black text (in the default theme), so I could also live with keeping a 5th "use" that hardcodes to white if you think that's more sensible (but maybe this just fits the pattern of these colors showing "classic" values when using glass?).

Finally, I'll note that -moz-dialog as a background for the window doesn't always fit the titlebar color used in other apps, which means it still doesn't look great (the stroke-width of the titlebar buttons and the position of the minimize icon and the size of the close icon is also off.). I would use Active/InactiveCaption for the titlebar element, but I'm worried that it won't mesh well with the colors for the toolbar/tabbar.

How far down this rabbithole do you think it's absolutely necessary to go within this bug? I'll put up the caption button + window background fixes in a second.

Comment 100

[:Gijs (out for now; he/him)]

Comment on attachment 8629390 [details]
MozReview Request: Bug 1173725 - part 1: force top border to be visible on windows 10 and don't cut out caption buttons, r?jimm

Bug 1173725 - part 1: force top border to be visible on windows 10 and don't cut out caption buttons, r?jimm

Comment 101

[:Gijs (out for now; he/him)]

Comment on attachment 8632089 [details]
MozReview Request: Bug 1173725 - part 2: make TabsInTitlebar logic remove margin bottom on titlebar after menubar goes away, r?dao

Bug 1173725 - part 2: make TabsInTitlebar logic remove margin bottom on titlebar after menubar goes away, r?dao

Comment 102

[:Gijs (out for now; he/him)]

Comment on attachment 8632090 [details]
MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

Comment 103

[:Gijs (out for now; he/him)]

Comment on attachment 8629390 [details]
MozReview Request: Bug 1173725 - part 1: force top border to be visible on windows 10 and don't cut out caption buttons, r?jimm

Carrying over review (turns out mozreview doesn't detect fast-forward rebases, I'll file a bug on that).

Comment 104

[Justin Dolske [:Dolske]]

(In reply to :Gijs Kruitbosch (away Jul 18 - Aug 2) from comment #99)

> How far down this rabbithole do you think it's absolutely necessary to go
> within this bug? I'll put up the caption button + window background fixes in
> a second.

I think we should not go down this rabbit hole at all here, and deal with any remaining issues in a followup.

My concern here is that is a substantial patch that changes fundamental UI, and I want to get it landed ASAP so we have time to discover and fix any _unknown_ issues quickly. Especially given the tight schedule before 40 ships, and that you're going to PTO at at the end of the week.

High-contrast mode already has lots of glitches, and we shouldn't be too concerned with adding one more, temporarily, to get earlier testing and feedback. Definitely for Nightly, and not a major issue for Beta either. Depending on how severe it is we could treat is as a blocker for shipping.

This has been through a few review cycles already, so if that's the last issue I expect this should now be ready to land?

Comment 105

[Jim Mathies [:jimm]]

(In reply to Justin Dolske [:Dolske] from comment #104)
> High-contrast mode already has lots of glitches, and we shouldn't be too
> concerned with adding one more, temporarily, to get earlier testing and
> feedback.

Sounds like it's time to run "High-contrast mode" through "Great or Dead".

Comment 106

[Dão Gottwald [:dao]]

Comment on attachment 8632090 [details]
MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons, r?dao,ui-r=phlsa

https://reviewboard.mozilla.org/r/12993/#review12083

::: browser/themes/windows/browser-aero.css:199
(Diff revision 4)
> +          }

this shouldn't be needed

::: browser/themes/windows/browser-aero.css:251
(Diff revision 4)
> +         (-moz-os-version: windows-win8) {

please merge this into the previous media query block

::: browser/themes/windows/browser-aero.css:223
(Diff revision 4)
> +            background-color: -moz-field;



btw, most of the time I spent on reviewing this was actually spent on dealing with reviewboard, and I'm not sure everything worked out... It feels kind of wrong that the patch auther can dictate how the review should be done. This goes both ways; if you really care about the nice things reviewboard gives you and review the vast majority of your patches there, and then somebody just attaches a diff to a bug, you're kind of lost.

Comment 107

[Dão Gottwald [:dao]]

(In reply to Jim Mathies [:jimm] from comment #105)
> (In reply to Justin Dolske [:Dolske] from comment #104)
> > High-contrast mode already has lots of glitches, and we shouldn't be too
> > concerned with adding one more, temporarily, to get earlier testing and
> > feedback.
> 
> Sounds like it's time to run "High-contrast mode" through "Great or Dead".

We digress, but please be reminded that accessibility is considered a core value, it's not a feature we can arbitrarily drop because apparently our support isn't great. So I think the question would be whether we want to maintain it as good enough or invest in making it great, unless there's data proving that nobody actually uses high contrast themes.

Comment 108

[Tim Nguyen :ntim]

https://reviewboard.mozilla.org/r/12993/#review12091

::: browser/themes/windows/caption-buttons.svg:1
(Diff revision 4)
> +<svg width="12" height="12" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

nit : Please don't forget to add a license header before landing.

::: browser/themes/windows/browser-aero.css:198
(Diff revision 4)
> +            background-color: hsla(0, 0%, 0%, 0);

As dao said, this shouldn't be needed,
and if it is, using transparent should just work.

Comment 109

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ec67632a6870
https://hg.mozilla.org/integration/fx-team/rev/1c6c590815ba
https://hg.mozilla.org/integration/fx-team/rev/47564df54abe

Comment 110

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #106)
> Comment on attachment 8632090 [details]
> MozReview Request: Bug 1173725 - part 3: use XUL for titlebar buttons,
> r?dao,ui-r=phlsa
> 
> https://reviewboard.mozilla.org/r/12993/#review12083
> 
> ::: browser/themes/windows/browser-aero.css:199
> (Diff revision 4)
> > +          }
> 
> this shouldn't be needed
> 
> ::: browser/themes/windows/browser-aero.css:251
> (Diff revision 4)
> > +         (-moz-os-version: windows-win8) {
> 
> please merge this into the previous media query block

This comment ended up at the first media query that had vista/win7/win8. I presume you meant to merge it with the next media query which had the same queries, rather than the previous one which was for win10+.
> 
> ::: browser/themes/windows/browser-aero.css:223
> (Diff revision 4)
> > +            background-color: -moz-field;
> 

I'm hoping you didn't actually mean to comment anything on this line. Pushed as-is, please ping me / file a bug if you did. I've already nuked review requests in bugzilla to prepare for PTO, but needinfo works.

> btw, most of the time I spent on reviewing this was actually spent on
> dealing with reviewboard, and I'm not sure everything worked out...

I would have liked to help, if I knew that before (or could have uploaded diffs). Feel free to ping me with questions, or when I've left tomorrow night, maybe mconley or smcleod could help.

> It feels
> kind of wrong that the patch auther can dictate how the review should be
> done.

Dictate feels like a strong word here, but I agree it's inconvenient.

> This goes both ways; if you really care about the nice things
> reviewboard gives you and review the vast majority of your patches there,
> and then somebody just attaches a diff to a bug, you're kind of lost.

I've used splinter long enough that I'm not lost, but e.g. the lack of context expansion annoys me, as do its inability to detect whitespace-only-changes, do inside-line-diffing, or to make explicit which lines you're commenting on (splinter blanket-quotes the last 4, or something? I've never really understood what it tries to do, if it's trying to be smarter it's not succeeded in being obvious enough for me to know...). Also, mozreview lets you easily push to try (though now that there's the hg push-to-try thing bundled with version-control-tools, I use that less often). All of these are nice features, but I agree that esp. the first few times, the UI is not the easiest to navigate.

To be fair, it is relatively easy to bzimport a patch, then push to reviewboard yourself and not publish the review, which allows you to look at it there, at least (which gets you back most of the features).

Likewise, if you have the revision for a reviewboard review request (listed on the page bugzilla links to from the 'attachment' for a review), you can use:

http://reviewboard-hg.mozilla.org/gecko/raw-rev/0f7115695281

to get a raw diff, or

http://reviewboard-hg.mozilla.org/gecko/rev/0f7115695281

to look at parents etc.

Hope that helps for next time.

Comment 111

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] from comment #108)
> https://reviewboard.mozilla.org/r/12993/#review12091
> 
> ::: browser/themes/windows/caption-buttons.svg:1
> (Diff revision 4)
> > +<svg width="12" height="12" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
> 
> nit : Please don't forget to add a license header before landing.

Meh. I disagree that SVGs need these, considering our other image assets also don't. It's not like the PNGs are "less work" or not covered under the license of the repo, or something, and we've never worried about adding e.g. comments in the PNG comment field with the license. Anyway, assuming this sticks, I'll land a DONTBUILD thing with a license header tomorrow.

Comment 112

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch (away Jul 18 - Aug 2) from comment #111)
> (In reply to Tim Nguyen [:ntim] from comment #108)
> > https://reviewboard.mozilla.org/r/12993/#review12091
> > 
> > ::: browser/themes/windows/caption-buttons.svg:1
> > (Diff revision 4)
> > > +<svg width="12" height="12" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
> > 
> > nit : Please don't forget to add a license header before landing.
> 
> Meh. I disagree that SVGs need these, considering our other image assets
> also don't. It's not like the PNGs are "less work" or not covered under the
> license of the repo, or something, and we've never worried about adding e.g.
> comments in the PNG comment field with the license. Anyway, assuming this
> sticks, I'll land a DONTBUILD thing with a license header tomorrow.

(err, to be clear, I did not see your comment until after I pushed, or I would have dealt with it then.)

Comment 113

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/ec67632a6870
https://hg.mozilla.org/mozilla-central/rev/1c6c590815ba
https://hg.mozilla.org/mozilla-central/rev/47564df54abe

Comment 114

[Pulsebot]

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

Comment 115

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec64584614d5

Comment 116

[:Gijs (out for now; he/him)]

Philipp, Stephen, when you're both back from PTO, it'd be great if you could doublecheck that the followups I filed cover what you expected.

Comment 117

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Looks good to me!

Comment 118

[Justin Dolske [:Dolske]]

Comment on attachment 8629390 [details]
MozReview Request: Bug 1173725 - part 1: force top border to be visible on windows 10 and don't cut out caption buttons, r?jimm

Approval Request Comment
[Feature/regressing bug #]: Windows 10 improvements
[User impact if declined]: Window minimize/maximize/close widgets look bad on Windows 10
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: Main risk is around getting details of our re-implementation of native controls looking and behaving right in various edge cases, although I'd expect the primary function of these buttons to be fine.
[String/UUID change made/needed]: n/a

I looked through the followup bugs Gijs filed, and I think that some don't need to ship in Firefox 40, and the others are not critical to have in the same beta (bug 1184934, bug 1184942, and bug 1184938 are the 3 I think we'd want to fix for release).

The earlier discussion about getting high contrast mode working seems to have mostly worked itself out, attachment 8636199 [details] indicates we look fine in a normal HC theme.

(Requesting uplift on this one patch, but really all 3 parts need to land.)

Comment 119

[Tim Nguyen :ntim]

(In reply to Justin Dolske [:Dolske] from comment #118)
> I looked through the followup bugs Gijs filed, and I think that some don't
> need to ship in Firefox 40, and the others are not critical to have in the
> same beta (bug 1184934, bug 1184942, and bug 1184938 are the 3 I think we'd
> want to fix for release).
We'll want bug 1184325 as well.

Comment 120

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Tim Nguyen [:ntim] from comment #119)
> We'll want bug 1184325 as well.

If this is strictly for dev edition we should only require the fix in Firefox 42 and later.

Comment 121

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8629390 [details]
MozReview Request: Bug 1173725 - part 1: force top border to be visible on windows 10 and don't cut out caption buttons, r?jimm

Straightforward patch in support of Windows 10. Let's get this into beta7. Beta+ Aurora+

Comment 122

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/001ba3f6d572
https://hg.mozilla.org/releases/mozilla-aurora/rev/8785b8c25228
https://hg.mozilla.org/releases/mozilla-aurora/rev/17b8af986d38

Comment 123

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/86ad14aebb17
https://hg.mozilla.org/releases/mozilla-beta/rev/dc26cb6f834e
https://hg.mozilla.org/releases/mozilla-beta/rev/f7c104dafd4b

Comment 124

[Tim Nguyen :ntim]

I'm seeing these 2 bugs (I've mentioned before) :
(In reply to Tim Nguyen [:ntim] from comment #19)
> => The right side the window sometimes gets cuts off
Filed bug 1187153
> => The content area can be cut-off after launching Firefox
Filed bug 1187125

I'm fairly sure this isn't profile specific since I've seen these issues in the past with different profiles (running on both Win 8 and Win 7) when I had used custom caption buttons (using an userstyle).

I may be wrong about the source of these bugs though, it may also be the chromemargin.

Comment 125

[Cornel Ionce [:noni] [Hubs QA]]

The title bar and the tab strip background is now indeed darker using:
- latest Nightly, build ID: 20150723030207;
- latest Aurora, build ID: 20150723004007;
- Firefox 40 beta 7, build ID: 20150723165742.

Closing this bug as the remaining dependencies will be handled as separate issues.

Comment 126

[Loic]

In bug 1195074, a user reported the same issue in 38 ESR on Win 10. Is there a possibility to backport this patch into ESR for users on Win 10?

Comment 127

[:Gijs (out for now; he/him)]

(In reply to Loic from comment #126)
> In bug 1195074, a user reported the same issue in 38 ESR on Win 10. Is there
> a possibility to backport this patch into ESR for users on Win 10?

I think it will be a significant amount of work to update ESR to include all our new styling plus the widget changes. Whether that's worth doing depends on the number of users on ESR that use Windows 10 and the risk of uplifting all this stuff. I would expect the number of users to be (very) low: Firefox 38 is our risk-averse, very-stable branch for enterprise, and I don't understand why people who would use a risk-averse, very-stable version of our browser would then update to a risky first release of an OS less than a month after it hit public release.

If they can update the OS, presumably they can also roll out a Firefox update to 40 or 41... I think unless there is solid data suggesting there's a large group of users in this group, it's not worth doing the work to backport all of these changes.

Comment 128

[jtechno555]

Thanks Loic for passing along the message/request. I do want to update you guys that the high DPI cosmetic issue in the ESR seems to have been fixed, even though I loaded up the exact version of Firefox under under the same OS.  My best guess is that Microsoft must have pushed an update somewhere between the time I reported the bug and today.