Comment on attachment 8702884 [details] [diff] [review] patch v1 Review of attachment 8702884 [details] [diff] [review]: ----------------------------------------------------------------- Just a couple of questions ::: devtools/client/themes/floating-scrollbars.css @@ -1,1 @@ > -@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); Why give the namespace a name here? Were you unable to do the :root selector without doing so? If so, could we do `[platform="mac"] scrollbar`, etc (removing :root from the selector) @@ +42,5 @@ > background-color: rgba(0,0,0,0.2); > border-radius: 3px; > } > + > +:root[platform="win"] xul|thumb, Why change `scrollbar thumb` to just `thumb`?

(In reply to Brian Grinstead [:bgrins] from comment #2) > Comment on attachment 8702884 [details] [diff] [review] > patch v1 > > Review of attachment 8702884 [details] [diff] [review]: > ----------------------------------------------------------------- > > Just a couple of questions > > ::: devtools/client/themes/floating-scrollbars.css > @@ -1,1 @@ > > -@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); > > Why give the namespace a name here? Were you unable to do the :root > selector without doing so? If so, could we do `[platform="mac"] scrollbar`, > etc (removing :root from the selector) Yes, that's to workaround the root with html documents. I got this trick suggested by Vivien, a old XUL guru and it allows to magically work. This is common to use explicit xul selector everywhere when we mixup namespaces like this. But I just realized this doesn't work on Responsive mode. We inject this stylesheet in a content document, which doesn't have the platform attribute set. I'll split this stylesheet per platform and inject the good one based. We already inject it dynamically (light-theme and responsive mode) > > @@ +42,5 @@ > > background-color: rgba(0,0,0,0.2); > > border-radius: 3px; > > } > > + > > +:root[platform="win"] xul|thumb, > > Why change `scrollbar thumb` to just `thumb`? To simplify, as sometime we were already using just `thumb` few lines before for Mac. I thought <xul:thumb> would only be used for scrollbars, but it is also used in <xul:scale> used in canvas debugger. So I'll readd the additional xul|scrollbar everywhere and stay consitant to one specific selector.

This patch complexify the floating scrollbars a bit, but reflects how complex our rules are. Here is the CSS hierarchy (@import hierarchy): - floating-scrollbars-theme-mac.css (additional overload just for mac and only for toolbox, not for responsive) > floating-scrollbars-mac.css (mac specifics, shared by responsive and toolbox) > floating-scollbars.css (common overrides, shared by responsive and toolbox and all OSes) - floating-scrollbars-non-mac.css (windows/linux specifics, shared by responsive and toolbox) > floating-scrollbars.css Note that I also removed unecessary !important in `scrollbar thumb` rules. Verified on Mac and Linux.

Why is floating-scrollbars-theme-mac.css needed? Seems that we were already loading all of these rules inside of responsive design via floating-scrollbars.css. If we didn't have that file then the hierarchy would be easier to follow.

(In reply to Brian Grinstead [:bgrins] from comment #5) > Why is floating-scrollbars-theme-mac.css needed? Seems that we were already > loading all of these rules inside of responsive design via > floating-scrollbars.css. If we didn't have that file then the hierarchy > would be easier to follow. Because we want floating-scrollbars-mac.css, with, for example, its `border:none` on `scrollbar`. But then this files also overloads the background of `scrollbar thumb` to `background-color: rgba(0,0,0,0.2)`. I think we can only simplify this by changing the behavior/final look of our scrollbars. Like, using the exact same styles between responsive design and toolboxes. Then, we could get rid of floating-scrollbars-theme-mac.css. I imagine we ended up with this complexity to match the precise look of system floating scrollbars on Mac and also help seeing them on white backgrounds. Note that scrollbars are light color on Windows and are very hard to see on light background.

(In reply to Alexandre Poirot [:ochameau] from comment #6) > (In reply to Brian Grinstead [:bgrins] from comment #5) > > Why is floating-scrollbars-theme-mac.css needed? Seems that we were already > > loading all of these rules inside of responsive design via > > floating-scrollbars.css. If we didn't have that file then the hierarchy > > would be easier to follow. > > Because we want floating-scrollbars-mac.css, with, for example, its > `border:none` on `scrollbar`. > But then this files also overloads the background of `scrollbar thumb` to > `background-color: rgba(0,0,0,0.2)`. > > I think we can only simplify this by changing the behavior/final look of our > scrollbars. > Like, using the exact same styles between responsive design and toolboxes. > Then, we could get rid of floating-scrollbars-theme-mac.css. Aren't we sharing exact styles between the responsive design and toolboxes right now?

ni for Comment 7. It seems like this is adding a new distinction that didn't exist before, between toolbox floating scrollbars and responsive design floating scrollbars. Maybe I'm missing something but I don't think we should change any UI with this work.

This patch is complex for keeping the exact same behavior than before. It would have been much simplier if we would have changed the UI. That what I tried to highlight in comment 6. Responsive mode loads floating-scrollbars-mac.css or floating-scrollbars-nonmac.css While toolboxes load floating-scrollbars-theme-mac.css or floating-scrollbars-nonmac.css if dark theme. The scrollbars look is only different between responsive mode and toolboxes on mac, before and after this patch.

(In reply to Alexandre Poirot [:ochameau] from comment #9) > This patch is complex for keeping the exact same behavior than before. > It would have been much simplier if we would have changed the UI. > That what I tried to highlight in comment 6. > > Responsive mode loads floating-scrollbars-mac.css or > floating-scrollbars-nonmac.css > While toolboxes load floating-scrollbars-theme-mac.css or > floating-scrollbars-nonmac.css if dark theme. > The scrollbars look is only different between responsive mode and toolboxes > on mac, before and after this patch. OK so if it's only different between those two modes on Mac, then I agree we should do a UI review of what it looks like now and see about consolidating into a single look on OSX since that will greatly simplify implementation. Or, what about completely removing floating scrollbars in responsive design mode? Ryan, maybe you have some context as to why they are there right now? The scrollbars are in the page's viewport - I don't see why we are overriding them.

Removing floating scrollbars in RDM would also simplify this patch a lot because we could go back to using [platform] selectors in a single CSS file.

(In reply to Brian Grinstead [:bgrins] from comment #10) > OK so if it's only different between those two modes on Mac, then I agree we > should do a UI review of what it looks like now and see about consolidating > into a single look on OSX since that will greatly simplify implementation. I would like to proceed with preprocessing removal rather than waiting for UI reviews. That's why I invested time in providing a patch that shouldn't change anything. From what I've seen, paul originaly just implemented some design from shortlander. But I couldn't find any image/pdf/document from him. So may be paul just wanted to match the style of native MacOS scrollbars, that are dark and not light. floating-scrollbars is only for dark theme and are light. Note in RDM it is often helpfull to have them dark as the content is often with a white background and it is hard to see the light scrollbars on windows and linux. But again, please help me get rid of preprocessing sooner than later.

(In reply to Alexandre Poirot [:ochameau] from comment #12) > (In reply to Brian Grinstead [:bgrins] from comment #10) > > OK so if it's only different between those two modes on Mac, then I agree we > > should do a UI review of what it looks like now and see about consolidating > > into a single look on OSX since that will greatly simplify implementation. > > I would like to proceed with preprocessing removal rather than waiting for > UI reviews. > That's why I invested time in providing a patch that shouldn't change > anything. > > From what I've seen, paul originaly just implemented some design from > shortlander. > But I couldn't find any image/pdf/document from him. > So may be paul just wanted to match the style of native MacOS scrollbars, > that are dark and not light. > floating-scrollbars is only for dark theme and are light. > Note in RDM it is often helpfull to have them dark as the content is often > with a white background and it is hard to see the light scrollbars on > windows and linux. > But again, please help me get rid of preprocessing sooner than later. I understand that, but we should at least consider removing the floating scrollbars in RDM since it would simplify the implementation greatly and should also improve the UX of RDM against light backgrounds. This patch is adding a lot of complexity that could be removed in that case - we could go back to using [platform] and not having all the extra scrollbars files.

It is important for RDM to use floating scrollbars of *some* kind or another. It's meant to simulate the look of a device screen, which (nearly?) always have floating scrollbars. On Mac, the existing styles for RDM actually on target a very specific case. For the average Mac user, they will see OS generated floating scrollbars inside RDM, and these styles have no effect on them. You can tell you have the OS generated ones because they are white on dark backgrounds and dark on white backgrounds. Clearly this is not possible from the single background color in CSS. The Mac floating scrollbar styles we are discussing here only apply for users who have changed System Preferences -> General -> Show scroll bars to Always. This means they see fixed scrollbars in all OS applications that take up actual space. These users are the only ones for which the Mac floating scrollbar styles actually apply: they create something similar to the floating OS scrollbars, except they are always black of course. So this means RDM gets special floating scrollbars for this case, which differs from the scrollbars these users see anywhere else. (If you wish to test the difference in Mac OS scrollbar settings, you *must* close and open a fresh tab after changing the OS settings. Reloading is not enough.) For Windows and Linux, the styles are much more important since (AFAIK) they always apply to create the floating scrollbars. So, does it simplify things to use the same scrollbar colors for all OSes in RDM? For Mac, it won't be a change for the default OS settings anyway, so most users will see no difference. Also, the RDM scrollbar color should probably be black on all OSes, since like Alex said most pages are probably light. (But changing that color can be done separately from this bug, just an idea.) Overall, we do need to keep some kind of floating scrollbar for all OSes in RDM. But yes, let's try to simplify down from the current patch here. It seems likely that we will want to a separate scrollbar color in RDM vs. toolbox, since in RDM it covers page content vs. the toolbox which has a known theme color we can control.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14) > So, does it simplify things to use the same scrollbar colors for all OSes in > RDM? For Mac, it won't be a change for the default OS settings anyway, so > most users will see no difference. Also, the RDM scrollbar color should > probably be black on all OSes, since like Alex said most pages are probably > light. (But changing that color can be done separately from this bug, just > an idea.) > > Overall, we do need to keep some kind of floating scrollbar for all OSes in > RDM. But yes, let's try to simplify down from the current patch here. It > seems likely that we will want to a separate scrollbar color in RDM vs. > toolbox, since in RDM it covers page content vs. the toolbox which has a > known theme color we can control. Somehow merging floating-scrollbars-theme-mac.css and floating-scrollbars-mac.css sounds good to me. If I'm undestanding correctly, here is the plan: Toolbox (dark theme only) and Responsive Design Mode will both use the same light colored floating scrollbars. There will be a separate file for mac and non-mac. There will be a plan to later convert these to be dark scrollbars for RDM - probably using `root:not(.theme-dark)` or a domain specific selector to distinguish between the two modes. I'm still not sure how this all interacts with floating-scrollbars-light.css.. Does that file need to be deleted in this patch?

(In reply to Brian Grinstead [:bgrins] from comment #15) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14) > > So, does it simplify things to use the same scrollbar colors for all OSes in > > RDM? For Mac, it won't be a change for the default OS settings anyway, so > > most users will see no difference. Also, the RDM scrollbar color should > > probably be black on all OSes, since like Alex said most pages are probably > > light. (But changing that color can be done separately from this bug, just > > an idea.) > > > > Overall, we do need to keep some kind of floating scrollbar for all OSes in > > RDM. But yes, let's try to simplify down from the current patch here. It > > seems likely that we will want to a separate scrollbar color in RDM vs. > > toolbox, since in RDM it covers page content vs. the toolbox which has a > > known theme color we can control. > > Somehow merging floating-scrollbars-theme-mac.css and > floating-scrollbars-mac.css sounds good to me. The current set of files in this patch is really confusing... I get lost each time I try to reason about what is used where. The file "floating-scrollbars-mac.css" is really "floating-scrollbars-responsive-mac.css" I guess (for RDM only)? I think we do want at least: * floating-scrollbars-responsive.css * floating-scrollbars-toolbox.css ...or something. They will want to be different colors. Maybe we should talk about this on IRC / Vidyo. > If I'm undestanding correctly, here is the plan: > > Toolbox (dark theme only) and Responsive Design Mode will both use the same > light colored floating scrollbars. There will be a separate file for mac > and non-mac. If we had a separate file for floating-scrollbars-toolbox.css, then it could use the platform selectors, and stay as one file. That might be easier. So, we may duplicate some things between -responsive.css and -toolbox.css, but it may also be easier to follow too. > There will be a plan to later convert these to be dark > scrollbars for RDM - probably using `root:not(.theme-dark)` or a domain > specific selector to distinguish between the two modes. I don't think RDM scrollbars care much about the DevTools theme...? They want to always be there. The color only matters based on the page content under them. Currently they are a mix of colors across platforms, but probably should switch to black everywhere.

Based on comment 16, with just two distinct files. Note that if don't split per platform, with a mac AND a non-mac file for RDM, we have to flatten the per-platform rules. We don't have just the background color. http://mxr.mozilla.org/mozilla-central/source/devtools/client/themes/floating-scrollbars.css#10 http://mxr.mozilla.org/mozilla-central/source/devtools/client/themes/floating-scrollbars.css#29 But it looks like, we can just flatten/merge everything and have the same end result, at least on Linux, I don't have Mac next to me. I still kept two distinct files, we could have merge these two with using root:not(.theme-dark) just for the dark background color in RDM, but it feels weak? These two files are basically the same thing, except: - the `scrollbar thumb { background-color: }` which is light on toolbox, and dark on RDM - explicit namespace in toolbox css, that to make :root work against html docs. (we can get rid of that, if we remove any usage of :root in the CSS, by merging all per-platform rules, like what I did for RDM css)

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16) > > > There will be a plan to later convert these to be dark > > scrollbars for RDM - probably using `root:not(.theme-dark)` or a domain > > specific selector to distinguish between the two modes. > > I don't think RDM scrollbars care much about the DevTools theme...? They > want to always be there. The color only matters based on the page content > under them. Currently they are a mix of colors across platforms, but > probably should switch to black everywhere. I intended for :not(.theme-dark) to be a way to select 'is in RDM' since the only time floating scrollbars are on in devtools toolbox is when the dark theme is applied. As Alex points out in Comment 17 it's a little flaky because a web page could presumably add that class to the root element and we'd incorrectly think we were in the toolbox. Maybe we could use the @document selector in addition to it and look for chrome / devtools paths somehow: https://developer.mozilla.org/en-US/docs/Web/CSS/@document.

Comment on attachment 8707438 [details] [diff] [review] patch v3 Review of attachment 8707438 [details] [diff] [review]: ----------------------------------------------------------------- Great, this version is much easier to think about! Personally I am fine with two separate files, I feel like they are for fairly independent problems, even if most of the contents are the same. But, feel free to try @document or other ideas if you like! I'll check this across platforms once the selectors get fixed. ::: devtools/client/themes/floating-scrollbars-dark-theme.css @@ +32,5 @@ > +} > + > +:root[platform="mac"] xul|thumb { > + -moz-appearance: none !important; > + background-color: rgba(0,0,0,0.2); The toolbox dark theme case (used to be "floating-scrollbars-light.css") would set background-color: rgba(170,170,170,0.2) for all platforms, so this line should be removed. ::: devtools/client/themes/floating-scrollbars-responsive-design.css @@ +8,5 @@ > z-index: 2147483647; > padding: 2px; > +} > + > +scrollbar { Same selector, collapse with above @@ +39,5 @@ > border-radius: 3px !important; > } > + > +scrollbar scrollbarbutton, > +scrollbar scrollbarbutton { scrollbar gripper

I tried experimenting with @-moz-document, but I have some issues with codemirror iframe which are data:text/html :x

I can do: @-moz-document url-prefix(chrome://devtools/content/) { ... } But then it doesn't match codemirror iframe. Or at the opposite, do: @-moz-document url-prefix(https://), url-prefix(http://) { ... } But that will misbehaves if loading anything else in responsive mode like file:// about:// chrome:// ...

(In reply to Alexandre Poirot [:ochameau] from comment #20) > I tried experimenting with @-moz-document, but I have some issues with > codemirror iframe which are data:text/html :x In that case I'd be fine with 2 separate, mostly identical files as suggested in Comment 19. The files aren't big and rarely change so that seems like the simplest solution.

Just addressed the comments. I must say the single css using -moz-document was much much better, especially when based on floating-scrollbars-responsive-design.css, without :root, not explicit namespace. But it also feels weak, I haven't found nice and solid url selectors.

Comment on attachment 8710505 [details] [diff] [review] patch v4 Review of attachment 8710505 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/floating-scrollbars-dark-theme.css @@ +1,3 @@ > +@namespace xul url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); > + > +xul|scrollbar { Why is the explicit `xul|` namespace needed again?

To make the :root selections magically work for html and xul documents. If we don't set a default namespace, it looks like document one is automatically set.

Comment on attachment 8710505 [details] [diff] [review] patch v4 Review of attachment 8710505 [details] [diff] [review]: ----------------------------------------------------------------- While testing this, I realized that our current behavior in the toolbox is strange. We only insert floating scrollbars for the dark theme, but leave the light theme up to the OS, which may or may not float. I would assume the toolbox would want floating all the time, but just with different colors per theme. Anyway, this change appears to recreate the existing behavior correctly. I only had time to check Mac, hopefully you've checked Win / Linux?

Just verified on windows. And I already verified this on linux while working on it.

https://hg.mozilla.org/integration/fx-team/rev/1bcd7705d2dc1610850ae01c221f55a6249a14cd Bug 1235780 - Remove preprocessing from floating scrollbar css. r=jryans