1016419 - Implement showing lightweight themes in customization mode

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Implementation bug for the design in bug 1007325

Comment 1

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

Marco, can you add this to the current iteration?

Comment 2

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3

Comment 3

[Marco Mucci [:MarcoM]]

Hi Gijs, any recommendation if this bug should be set to [qa+] or [qa-] for verification.

Comment 4

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

QA+ because it's an appearance thing which will need testing on different platforms.

Comment 5

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

Attachment: WIP patch

This works on OS X, at least. Still need to implement the groove per spec, and look at Windows and Linux. Hopeful that those are easier than OSX with its overlapping titlebar and toolbox... Also need to doublecheck that altering the lwt during customize mode doesn't mess this up (but I /believe/ I've catered for that case).

Comment 6

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

Attachment: implement showing lwt in customization mode,

As far as I can tell this implements the spec as described on OS X.

Comment 7

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

Attachment: implement showing lwt in customization mode,

This works on win8, but I need to check that this still works on OS X, I've not looked at Linux yet, and I am worried about the top window borders on XP and Windows 7 (non-glass).

Comment 8

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

Attachment: implement showing lwt in customization mode,

Ugh. This is getting uglier and uglier. try push: https://tbpl.mozilla.org/?tree=Try&rev=2e6c56f798a9

Comment 9

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

Attachment: implement showing lwt in customization mode,

This takes care of Linux. Now need to double/triple-test on Windows.

Comment 10

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

Attachment: implement showing lwt in customization mode,

Noticed that we still weren't reacting properly to theme updates, fixed in this iteration.

Comment 11

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

Attachment: implement showing lwt in customization mode,

So... I believe this is done, code-wise. I'm flagging both of you for review - Mike because of OS X titlebar craziness, and Jared for the Windows styling, and both of you for the tweaks to lwts and CustomizeMode.jsm in general, I suppose. I hope the patch is mostly self-explanatory, but I'll add some more details in a bit.

Comment 12

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

Comment on attachment 8433642 [details] [diff] [review]
implement showing lwt in customization mode,

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

::: browser/base/content/browser.js
@@ +4546,5 @@
>      }
>  
>      ToolbarIconColor.inferFromText();
> +    if (CustomizationHandler.isCustomizing()) {
> +      gCustomizeMode.updateLWTStyling();

Basically, if something toggles tabsintitlebar within customize mode, we need to update so we continue matching the size of the tab view deck - but it needs to happen after this code runs...

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +537,5 @@
> +    if (!aData) {
> +      let lwt = docElement._lightweightTheme;
> +      aData = lwt.getData();
> +    }
> +    let headerURL = aData && aData.headerURL;

aData can be an object with lots of ""-value properties, so I'm checking headerURL as well.

@@ +545,5 @@
> +    }
> +
> +    let deck = this.document.getElementById("tab-view-deck");
> +    let headerImageRef = this._getHeaderImageRef(aData);
> +    docElement.setAttribute("customization-lwtheme", "true");

As much as I didn't like adding another attribute, I wanted something that was added well before and removed after the transition, and didn't want to interfere with the existing attributes.

@@ +583,5 @@
> +                                 ", rgba(0,0,0,0.25) " + ridgeCenter +
> +                                 ", rgba(255,255,255,0.5) " + ridgeCenter +
> +                                 ", rgba(255,255,255,0.5) " + ridgeEnd + ", " +
> +                                 "transparent " + ridgeEnd + ")";
> +    deck.style.backgroundImage = ridge + ", " + limitedBG;

So generally what happens is this: we set the lwt background and the ridge/groove/thing on #tab-view-deck, with the grid and any gradient stuff being set on #main-window . Because of the order of this backgroundImage, the ridge is overlaid on top of the bg, and the bg + ridge on top of the grid.

One more thing: on Windows, we manually add borders for the window because the lwt means we don't draw the titlebar. I moved those from the #titlebar-content to the #tab-view-deck's first and only child, the #browser-panel, which means they then go on top of this bg set still.

@@ +610,5 @@
> +    }
> +  },
> +
> +  _getHeaderImageRef: function(aData) {
> +    return "url(\"" + aData.headerURL.replace(/"/g, '\\"') + "\")";

This is cargo-culted from lwtconsumer, not sure if it's worth abstracting out.

@@ -1207,5 @@
>  #endif
>      }
>    },
>  
> -#ifdef CAN_DRAW_IN_TITLEBAR

Basically, I needed the observer bit of the object to work even on Linux.

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +84,5 @@
>  
>  #customization-container {
>    background-color: rgb(247,247,247);
> +  color: black;
> +  text-shadow: none;

Otherwise, at least on OS X, the header of customize mode gets an ugly shadow, and the text is white, for dark lwts - all inherited from #main-window.

Comment 13

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #12)
> One more thing: on Windows, we manually add borders for the window because
> the lwt means we don't draw the titlebar. I moved those from the
> #titlebar-content to the #tab-view-deck's first and only child, the
> #browser-panel, which means they then go on top of this bg set still.

#browser-panel is #tab-view-deck's only /static/ child. Panorama obviously adds another one and other consumers could theoretically do the same. That's the whole point of having a deck there.

Comment 14

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

(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #12)
> > One more thing: on Windows, we manually add borders for the window because
> > the lwt means we don't draw the titlebar. I moved those from the
> > #titlebar-content to the #tab-view-deck's first and only child, the
> > #browser-panel, which means they then go on top of this bg set still.
> 
> #browser-panel is #tab-view-deck's only /static/ child. Panorama obviously
> adds another one and other consumers could theoretically do the same. That's
> the whole point of having a deck there.

So obvious there isn't even a comment to that effect, clearly!

I'm not sure this matters for what's going on here, I'd need to look what happens in panorama mode before and after the patch. Did you check already, and/or do you have suggestions on what to do instead?

Comment 15

[Dão Gottwald [:dao]]

I haven't checked, but I expect that your changes will cause the borders to go missing when entering Panorama, as this hides #browser-panel; this is how <deck> works.

Comment 16

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

(In reply to Dão Gottwald [:dao] from comment #15)
> I haven't checked, but I expect that your changes will cause the borders to
> go missing when entering Panorama, as this hides #browser-panel; this is how
> <deck> works.

I understand that that is how deck works.

However, it doesn't seem to be an issue, because in restored mode, switching to panorama shows the titlebar again, on both Windows and OS X, so the borders not being there is actually a feature rather than a bug (which I presume worked before because #titlebar-content probably also isn't shown in Panorama).

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8433642 [details] [diff] [review]
implement showing lwt in customization mode,

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +545,5 @@
> +    }
> +
> +    let deck = this.document.getElementById("tab-view-deck");
> +    let headerImageRef = this._getHeaderImageRef(aData);
> +    docElement.setAttribute("customization-lwtheme", "true");

Can you add a note at http://hg.mozilla.org/mozilla-central/annotate/7146e89a7b83/browser/components/customizableui/src/CustomizeMode.jsm#l474 about the timing of this attribute?

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8433642 [details] [diff] [review]
implement showing lwt in customization mode,

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

Ok, I think I get this. I've tested it on Windows, and it does the job nicely. The ridgeStart hack needs, imo, a little more explanation in the code, but other than that, I think this is pretty solid.

Thanks Gijs!

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +565,5 @@
> +                    height + ", 0)";
> +
> +    // To make this align correctly with the toolbox, and to make antialiasing
> +    // not turn the ridge into an ugly blur on non-hidpi displays, we resort
> +    // to gross hacks:

Bodacious! :D

@@ +572,5 @@
> +#else
> +    let ridgeStart = height - 0.5;
> +#endif
> +
> +    let ridgeCenter = (ridgeStart + 1) + "px";

I could be wrong, but this looks fragile / mighty sensitive to any future style changes in the toolbars. Can you include some more documentation on what these gross hacks do in the code itself?

::: browser/themes/windows/browser.css
@@ +255,5 @@
>          rgb(164,178,127) 2px, rgb(164,178,127) 3px,
>          transparent 3px);
>    }
>  
> +  #main-window[tabsintitlebar][sizemode="normal"] > #tab-view-deck > #browser-panel:-moz-lwtheme:-moz-window-inactive {

Yay! Death to descendant selectors!

@@ +2834,5 @@
>    background-image: url("chrome://browser/skin/customizableui/customizeMode-gridTexture.png");
>    background-attachment: fixed;
>  }
>  
> +#main-window[customization-lwtheme] > #tab-view-deck:-moz-lwtheme {

Seems like this rule might be something we could put into a shared place at some point...

::: toolkit/modules/LightweightThemeConsumer.jsm
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  this.EXPORTED_SYMBOLS = ["LightweightThemeConsumer"];
>  
> +const {utils: Cu} = Components;

Nice to see some modernization in here. :)

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Oh, and I think we might want to keep an eye on CART when this lands.

Comment 20

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

The gross hack is the different offset for the border, just that. It's essentially a 2px high background image that is two lines (darker and lighter line) that create the impression of a groove/ridge/thing. The 'hack' is that I'm using a different positioning (by 0.5px) for the background on OS X and Windows. I basically just lined things up visually and this worked. I can look into it again and see if I can figure out why this is necessary - I've poked a little with DOMI and not noticed anything obvious in terms of differences, but maybe I missed something.

Try push of the latest patch on fx-team tip:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=c373df7200c9


In theory, I expect zero CART impact because this is 90% changes for LWTs in customize mode, which I'm pretty sure CART doesn't test... In practice, well, let's find out:

baseline for the above: remote:   https://tbpl.mozilla.org/?tree=Try&rev=642a2022cc66

Comment 21

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

http://compare-talos.mattn.ca/?oldRevs=642a2022cc66&newRev=c373df7200c9&server=graphs.mozilla.org&submit=true looks like a wash, leaning slightly to 'positive', to me.

I checked and AFAICT I can remove the ifdef hack without consequence, so I'm landing it without that.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> Can you add a note at
> http://hg.mozilla.org/mozilla-central/annotate/7146e89a7b83/browser/
> components/customizableui/src/CustomizeMode.jsm#l474 about the timing of
> this attribute?

Done.



remote:   https://hg.mozilla.org/integration/fx-team/rev/261ac0a51324

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/261ac0a51324

Comment 23

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned to this bug for verification.

Comment 24

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

Mozilla/5.0 (Windows NT 6.3; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (X11; Linux i686; rv:33.0) Gecko/20100101 Firefox/33.0

The LW themes are now displayed in the customization mode using latest Nightly (build ID: 20140610030202) on Windows 8.1 32bit, Windows 7 64bit, Mac OS X 10.9 and Ubuntu 14.04.