Tim Nguyen :ntim Reporter
	Description • 7 years ago

Switching between 2 themes feels jumpy and disrupting, especially when using a dynamic theming webextension.

For example, when switching tabs with the VivaldiFox webextension, there's no color transition, which feels jumpy. 

A fix for this would be adding a transition for main UI colors such as the frame background and the toolbar background, so the transition feels smooth.

	Justin Dolske [:Dolske]
	Comment 1 • 7 years ago

Yes! This would be pretty cool!

(But calling this a P5 because it's not actually hurting users.)

	Jared Wein [:jaws] (please needinfo? me)
	Comment 3 • 7 years ago

Hey Eric, when transitioning between themes, how long should the duration be for transition and what animation curve should we use?

	Eric Pang [:epang] UX
	Comment 4 • 7 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Hey Eric, when transitioning between themes, how long should the duration be
> for transition and what animation curve should we use?

Let's try a duration of 1.5sec with a easy ease curve and see how that looks. Thanks!

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 7 years ago

(In reply to Eric Pang [:epang] UX from comment #4)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> > Hey Eric, when transitioning between themes, how long should the duration be
> > for transition and what animation curve should we use?
> 
> Let's try a duration of 1.5sec with a easy ease curve and see how that
> looks. Thanks!

Can you explain what you mean by an "easy ease curve"? Would "ease-out" work? Can you make a curve with http://cubic-bezier.com/ ?

	Eric Pang [:epang] UX
	Comment 6 • 7 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> (In reply to Eric Pang [:epang] UX from comment #4)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> > > Hey Eric, when transitioning between themes, how long should the duration be
> > > for transition and what animation curve should we use?
> > 
> > Let's try a duration of 1.5sec with a easy ease curve and see how that
> > looks. Thanks!
> 
> Can you explain what you mean by an "easy ease curve"? Would "ease-out"
> work? Can you make a curve with http://cubic-bezier.com/ ?

sorry, seems that the terminology is different in after effects.  Just an ease like this:
cubic-bezier(.17,.67,.83,.67)

	Mike Conley (:mconley) (:⚙️)
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review225052

Thanks Vivek! Really excited for this patch - it really smooths out the experience.

I have suggestions to make your test a bit more robust - let me know if you have any questions about it.

::: browser/base/content/browser.css:24
(Diff revision 2)
>  :root:-moz-lwtheme {
>    background-color: var(--lwt-accent-color) !important;
>    background-image: var(--lwt-header-image), var(--lwt-additional-images) !important;
>    background-position: var(--lwt-background-alignment) !important;
>    background-repeat: var(--lwt-background-tiling) !important;
> +  transition: background-color 0.1s cubic-bezier(.17,.67,.83,.67);

This repeated pattern, `0.1s cubic-bezier(.17,.67,.83,.67);`, seems ripe for a CSS variable. Maybe something like... --theme-transition or something?

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:3
(Diff revision 2)
> +"use strict";
> +
> +Cu.importGlobalProperties(["InspectorUtils"]);

Is this used? I can't find a use in this file.

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:8
(Diff revision 2)
> +Cu.importGlobalProperties(["InspectorUtils"]);
> +
> +// This test checks whether the applied theme transition effects are applied
> +// correctly.
> +
> +add_task(async function test_button_background_properties() {

Should probably rename this function to be something like:

`async function test_theme_transition_effects()`.

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:38
(Diff revision 2)
> +
> +  });
> +
> +  await extension.startup();
> +
> +  let toolbars = document.querySelector("#navigator-toolbox").getElementsByTagName("toolbar");

Don't we also need to check the :root for the accent colour?

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:40
(Diff revision 2)
> +  for (let i = 0; i < toolbars.length; i++) {
> +    let toolbar = toolbars[i];

If you're iterating a collection like this in modern JS, you can do:

```js
for (let toolbar of toolbars) {
}
```

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:45
(Diff revision 2)
> +  for (let i = 0; i < toolbars.length; i++) {
> +    let toolbar = toolbars[i];
> +    let toolbarCS = window.getComputedStyle(toolbar);
> +    let transitionProperty = toolbarCS.getPropertyValue("transition-property");
> +
> +    if (transitionProperty == "background") {

This is going to skip toolbars where there are multiple transition properties - like the Bookmark toolbar, which transitions on min-height and max-height as well.

Supposing we have transitions on a node like this:

```css
somenode {
  transition: min-height 170ms ease-out, max-height 170ms ease-out, background-color 1s cubic-bezier(.17,.67,.83,.67);;
}
```

We can query for the styles, like:

```js
let styles = window.getComputedStyle(somenode);
let transitionProps = styles.getPropertyValue("transition-property");
```

And given the above rule, `transitionProps` will be equal to: `min-height, max-height, background-color`, which is not going to satisfy your conditional on line 45.

So what we might want to do is the following:

1. Have a statically defined list of nodes that you want to test, and what you expect their transitions to be
2. Loop through that list, get the node, get the transition-property, and split it by comma. Find the index of the expected property, and then get the `transition-duration`, `transition-timing-function` and `transition-delay` for the node. Split by comma, and use the same index that you already calculated to get the right value, and compare against your expectations.

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:49
(Diff revision 2)
> +
> +    if (transitionProperty == "background") {
> +      Assert.equal(
> +        toolbarCS.getPropertyValue("transition-duration"),
> +        TRANSITION_DURATION,
> +        "Toolbar transition duration set for - " + toolbar.getAttribute("id")

You can use `toolbar.id`

	Jared Wein [:jaws] (please needinfo? me)
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review225058

mconley's review here is sufficient.

	Tim Nguyen :ntim Reporter
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review225570

::: browser/base/content/browser.css:13
(Diff revision 4)
>  :root {
>    --panelui-subview-transition-duration: 150ms;
>    --lwt-additional-images: none;
>    --lwt-background-alignment: right top;
>    --lwt-background-tiling: no-repeat;
> +  --lwt-theme-transition: background-color 0.1s cubic-bezier(.17,.67,.83,.67);

use a preprocessor define since this is static.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review225876

Looks good, one more revision should be all that's needed.

::: browser/base/content/browser.css:143
(Diff revision 4)
> +#navigator-toolbox > toolbar:-moz-lwtheme {
> +  transition: var(--lwt-theme-transition);
> +}

This should be moved to /browser/themes/shared/browser.inc.css above the /* Bookmark toolbar */ section.

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:32
(Diff revision 4)
> +  let awaitTransitionEnd = BrowserTestUtils.waitForEvent(docEl, "transitionend");
> +  await awaitTransitionEnd;

waitForEvent returns a promise, and when you "await" on a promise the code will pause execution of this stack until the promise is resolved. Once the promise is resolved execution can continue.

Since you're storing the promise in the awaitTransitionEnd variable and then awaiting on it immediately, there is no need to store it in a local variable.

Instead you can just write `await BrowserTestUtils.waitForEvent(docEl, "transitionend");`

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:79
(Diff revision 4)
> +  let awaitTransitionEnd = BrowserTestUtils.waitForEvent(docEl, "transitionend");
> +  await awaitTransitionEnd;

Ditto.

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:88
(Diff revision 4)
>                 "Window background is set to the colors.frame property");
>  
>    // Now we'll open a new window to see if the inactive browser accent color changed
>    let window2 = await BrowserTestUtils.openNewBrowserWindow();
>  
> +  await awaitTransitionEnd;

Since this promise was already resolved on line 80 it will be guaranteed to be resolved at this point, and as such this "await" isn't doing anything. You could either remove it or wait add a new waitForEvent, depends on what you intended it to do.

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:124
(Diff revision 4)
> +  let awaitTransitionEnd = BrowserTestUtils.waitForEvent(docEl, "transitionend");
> +  await awaitTransitionEnd;

Same comment as above.

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:13
(Diff revision 4)
> +  const TOOLBAR = "#f27489";
> +  const TEXT_COLOR = "#000000";
> +  const TRANSITION_DURATION = "0.1s";
> +  const TRANSITION_DELAY = "0s";
> +  const TRANSTITION_TIMING_FUNCTION = "cubic-bezier(0.17, 0.67, 0.83, 0.67)";
> +  const BACKGROUND_COLOR = "background-color";

This variable should be renamed to TRANSITION_PROPERTY

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:40
(Diff revision 4)
> +  await extension.startup();
> +
> +  // check if transition effect is set for root, which affects transition for
> +  // accent color
> +  let docEl = window.document.documentElement;
> +  let style = window.getComputedStyle(docEl);

Please rename `style` to `rootCS`.

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:92
(Diff revision 4)
> +      Assert.equal(
> +        toolbarCS.getPropertyValue("transition-delay"),
> +        TRANSITION_DELAY,
> +        "Transition delay set for " + toolbar.id
> +      );
> +    } else if (transitionProperties.length > 1) {

This block of code here handles the case of 0, 1, or more transition properties, so there is no need for duplicating this with the above if-block. You can remove the above if-block and always run this code.

From the CSS that you have written we shouldn't have a case where a toolbar doesn't have a transition-property when a lwtheme is installed.

	Vivek Assignee
	Comment 17 • 7 years ago

Another revision should be up with all issues resolved. Please let me know if there is something else I can improve upon. Thank you for feedback!

	Mike Conley (:mconley) (:⚙️)
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review226098

Thanks Vivek! This is great stuff, and I'm really excited to see it land. I have some comments and suggestions below. Thakns for your work!

::: browser/themes/shared/browser.inc.css:9
(Diff revision 6)
> +%define themeTransitionProperty background-color
> +%define themeTransitionDuration 0.1s
> +%define themeTransitionDelay 0s
> +%define themeTransitionTimingFunction cubic-bezier(.17,.67,.83,.67)

Since these are always used together, I wonder if we might try to combine them like:

```
%define themeTransition background-color 0.1s 0s cubic-bezier(.17,.67,.83,.67)
```

and then use:

```css
navigator-toolbox > toolbar:-moz-lwtheme {
  transition: @themeTransition@
}
```

and

```css
#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar) {
  overflow: -moz-hidden-unscrollable;
  max-height: 4em;
  transition: min-height 170ms ease-out, max-height 170ms ease-out, 
              @themeTransition;
  padding: 0 6px 2px;
}
```

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:32
(Diff revision 6)
>    });
>  
>    await extension.startup();
>  
>    let docEl = window.document.documentElement;
> +  await BrowserTestUtils.waitForEvent(docEl, "transitionend");

Can you add a comment here saying that we're waiting for the theme transition to end?

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:78
(Diff revision 6)
>    await extension.startup();
>  
>    let docEl = window.document.documentElement;
>    let style = window.getComputedStyle(docEl);
>  
> +  await BrowserTestUtils.waitForEvent(docEl, "transitionend");

Similar to the above - we should mention that we're waiting for the theme transition to end.

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:120
(Diff revision 6)
>    await extension.startup();
>  
>    let docEl = window.document.documentElement;
>    let style = window.getComputedStyle(docEl);
>  
> +  await BrowserTestUtils.waitForEvent(docEl, "transitionend");

Same as above.

::: toolkit/components/extensions/test/browser/browser_ext_themes_dynamic_updates.js:18
(Diff revision 6)
>  function hexToRGB(hex) {
>    hex = parseInt((hex.indexOf("#") > -1 ? hex.substring(1) : hex), 16);
>    return "rgb(" + [hex >> 16, (hex & 0x00FF00) >> 8, (hex & 0x0000FF)].join(", ") + ")";
>  }
>  
> -function validateTheme(backgroundImage, accentColor, textColor, isLWT) {
> +async function validateTheme(backgroundImage, accentColor, textColor, isLWT) {

We should document this function, saying that it expects the themes to still be in transition when called.

::: toolkit/components/extensions/test/browser/browser_ext_themes_lwtsupport.js:32
(Diff revision 6)
>    await extension.startup();
>  
>    let docEl = window.document.documentElement;
>    let style = window.getComputedStyle(docEl);
>  
> +  await BrowserTestUtils.waitForEvent(docEl, "transitionend");

Same as earlier - we should mention that we're waiting for the theme transition to finish after the theme installs.

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:74
(Diff revision 6)
> +    for (let [index, property] of transitionProperties.entries()) {
> +      if (property == TRANSITION_PROPERTY) {
> +        Assert.equal(
> +          toolbarCS.getPropertyValue("transition-duration").split(",")[index],
> +          TRANSITION_DURATION,
> +          "Transition duration set for " + toolbar.id
> +        );
> +
> +        if (transitionProperties.length == 1) {
> +          Assert.equal(
> +            toolbarCS.getPropertyValue("transition-timing-function"),
> +            TRANSTITION_TIMING_FUNCTION,
> +            "Transition timing set for " + toolbar.id
> +          );
> +        } else {
> +          Assert.equal(
> +            toolbarCS.getPropertyValue("transition-timing-function").split(",")[index],
> +            TRANSTITION_TIMING_FUNCTION,
> +            "Transition timing set for " + toolbar.id
> +          );
> +        }
> +
> +        Assert.equal(
> +          toolbarCS.getPropertyValue("transition-delay").split(",")[index],
> +          TRANSITION_DELAY,
> +          "Transition delay set for " + toolbar.id
> +        );
> +      }
> +    }

Instead of looping over the properties and just executing on the one that matches, can we:

1. Find the index of the right transition property, and assert that the index > -1.
2. Compute the split of the durations, timing functions, and delay, and get the same index
3. Do our comparisons?

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:82
(Diff revision 6)
> +        if (transitionProperties.length == 1) {
> +          Assert.equal(
> +            toolbarCS.getPropertyValue("transition-timing-function"),
> +            TRANSTITION_TIMING_FUNCTION,
> +            "Transition timing set for " + toolbar.id
> +          );
> +        } else {
> +          Assert.equal(
> +            toolbarCS.getPropertyValue("transition-timing-function").split(",")[index],
> +            TRANSTITION_TIMING_FUNCTION,
> +            "Transition timing set for " + toolbar.id
> +          );
> +        }

Why is this conditional required?

	Dão Gottwald [:dao]
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review226772

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:32
(Diff revision 6)
>    });
>  
>    await extension.startup();
>  
>    let docEl = window.document.documentElement;
> +  await BrowserTestUtils.waitForEvent(docEl, "transitionend");

Please check the event target and property name so that you don't catch random unrelated transitions:

https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/browser/base/content/browser.js#8501

	Jared Wein [:jaws] (please needinfo? me)
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review226924

::: browser/base/content/browser.css:8
(Diff revision 6)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  @namespace html url("http://www.w3.org/1999/xhtml");
>  
> +%include ../../themes/shared/browser.inc.css

You can delete this line based on my comment below.

::: browser/base/content/browser.css:26
(Diff revision 6)
>  :root:-moz-lwtheme {
>    background-color: var(--lwt-accent-color) !important;
>    background-image: var(--lwt-header-image), var(--lwt-additional-images) !important;
>    background-position: var(--lwt-background-alignment) !important;
>    background-repeat: var(--lwt-background-tiling) !important;
> +  transition: @themeTransitionProperty@ @themeTransitionDuration@ @themeTransitionTimingFunction@ @themeTransitionDelay@;

This line can be moved to /browser/themes/shared/browser.inc.css since we don't have any production (non-test) code that will be relying on the transitionend event.

	Code Review Bot [:reviewbot]
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review228764


Code analysis found 8 defects in this patch:
 - 8 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:48
(Diff revision 7)
> -  Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR.join(", ") + ")",
> +      Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR.join(", ") + ")",
>                 "Expected correct background color");
> +
> +      docEl.removeEventListener("transitionend", themeTransitionHandler);
> +    }
> +  });  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:92
(Diff revision 7)
> -  Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR.join(", ") + ")",
> +      Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR.join(", ") + ")",
>                 "Window background is set to the colors.frame property");
>  
> +      docEl.removeEventListener("transitionend", themeTransitionHandler);
> +    }
> +  });  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:98
(Diff revision 7)
> +
>    // Now we'll open a new window to see if the inactive browser accent color changed
>    let window2 = await BrowserTestUtils.openNewBrowserWindow();
>  
>    Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR_INACTIVE.join(", ") + ")",
> -               `Inactive window background color should be ${FRAME_COLOR_INACTIVE}`);
> +           `Inactive window background color should be ${FRAME_COLOR_INACTIVE}`);

Error: Expected indentation of 15 spaces but found 11. [eslint: indent]

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:138
(Diff revision 7)
> +    if (e.target == docEl && e.propertyName == "background-color") {
> -  Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR.join(", ") + ")",
> +      Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR.join(", ") + ")",
>                 "Window background is set to the colors.frame property");
> +      docEl.removeEventListener("transitionend", themeTransitionHandler);
> +    }
> +  });  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/components/extensions/test/browser/browser_ext_themes_dynamic_updates.js:42
(Diff revision 7)
> +    docEl.addEventListener("transitionend", function themeTransitionHandler(e) {
> +      if (e.target == docEl && e.propertyName == "background-color") {
> -  Assert.equal(style.backgroundColor, accentColor, "Expected correct accent color");
> +        Assert.equal(style.backgroundColor, accentColor, "Expected correct accent color");
> +        docEl.removeEventListener("transitionend", themeTransitionHandler);
> +      }
> +    });  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/components/extensions/test/browser/browser_ext_themes_dynamic_updates.js:44
(Diff revision 7)
> -  Assert.equal(style.backgroundColor, accentColor, "Expected correct accent color");
> +        Assert.equal(style.backgroundColor, accentColor, "Expected correct accent color");
> +        docEl.removeEventListener("transitionend", themeTransitionHandler);
> +      }
> +    });  
> +  } else {
> +      Assert.equal(style.backgroundColor, accentColor, "Expected correct accent color");

Error: Expected indentation of 4 spaces but found 6. [eslint: indent]

::: toolkit/components/extensions/test/browser/browser_ext_themes_lwtsupport.js:45
(Diff revision 7)
> -  Assert.equal(style.backgroundColor, "rgb(" + hexToRGB(ACCENT_COLOR).join(", ") + ")",
> +      Assert.equal(style.backgroundColor, "rgb(" + hexToRGB(ACCENT_COLOR).join(", ") + ")",
> -               "Expected correct background color");
> +                   "Expected correct background color");
> +
> +      docEl.removeEventListener("transitionend", themeTransitionHandler);
> +    }
> +  });  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbars.js:54
(Diff revision 7)
> -                 hexToCSS(TOOLBAR_COLOR),
> +                     hexToCSS(TOOLBAR_COLOR),
> -                 "Toolbar background color should be set.");
> +                     "Toolbar background color should be set.");
> +
> +        toolbar.removeEventListener("transitionend", themeTransitionHandler);
> +      }
> +    });  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

	Tim Nguyen :ntim Reporter
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review228788

::: toolkit/components/extensions/test/browser/browser_ext_themes_alpha_accentcolor.js:26
(Diff revision 7)
> +  docEl.addEventListener("transitionend", function themeTransitionHandler(e) {
> +    if (e.target == docEl && e.propertyName == "background-color") {
> -  Assert.equal(style.backgroundColor, "rgb(230, 128, 0)",
> +      Assert.equal(style.backgroundColor, "rgb(230, 128, 0)",
> -               "Window background color should be opaque");
> +                   "Window background color should be opaque");
>  
> +      docEl.removeEventListener("transitionend", themeTransitionHandler);
> +    }
> +  });

Please make a helper function in toolkit/components/extensions/test/browser/head.js for this:

function waitForTransition(doc, property) {
  return new Promise(resolve => {
    let listener = docEl.addEventListener("transitionend", e => {
      if (e.target == doc && e.propertyName == property) {
        doc.removeEventListener("transitionend", listener);
        resolve();
      }
    });
  });
}

and then you can use it like:

await waitForTransition(docEl, "background-color");

	Tim Nguyen :ntim Reporter
	Comment 28 • 6 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review232350

::: browser/themes/shared/browser.inc.css:36
(Diff revision 10)
> +#navigator-toolbox > toolbar:-moz-lwtheme {
> +  transition: @themeTransition@;
> +}
> +

Seems like this is only useful for the #nav-bar, since other toolbars are covered by other rules.

Can you change the selector to #nav-bar:-moz-lwtheme ?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 30 • 6 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review232418

r=me with the three minor changes fixed.

::: browser/themes/shared/browser.inc.css:9
(Diff revision 12)
>  
>  %include downloads/indicator.inc.css
>  
>  %filter substitution
>  %define navbarTabsShadowSize 1px
> +%define themeTransition background-color 0.1s 0s cubic-bezier(.17,.67,.83,.67)

The `0s` you have here, for transition-delay, is not necessary since that is the default value. You can remove it.

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:95
(Diff revision 12)
>    let window2 = await BrowserTestUtils.openNewBrowserWindow();
>  
>    Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR_INACTIVE.join(", ") + ")",
>                 `Inactive window background color should be ${FRAME_COLOR_INACTIVE}`);
>  
> +

Can you delete this extra line that got added? I *think* two blank lines will cause an eslint error. Did you run this through `./mach eslint` ?

::: toolkit/components/extensions/test/browser/browser_ext_themes_theme_transition.js:7
(Diff revision 12)
> +
> +// This test checks whether the applied theme transition effects are applied
> +// correctly.
> +
> +add_task(async function test_theme_transition_effects() {
> +  const ACC_COLOR = "#aaf442";

please rename this to ACCENT_COLOR

	Vivek Assignee
	Comment 32 • 6 years ago
mozreview-review-reply

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review232418

> Can you delete this extra line that got added? I *think* two blank lines will cause an eslint error. Did you run this through `./mach eslint` ?

I did run linter and this was not an issue caught by it. I have still deleted the extra line

	Mike Conley (:mconley) (:⚙️)
	Comment 34 • 6 years ago

Hey vivek,

Can you please trigger a try push, and post the resulting link here?

The try syntax I suggest:

try: -b do -p linux64,macosx64,win32 -u mochitest-e10s-bc -t none --artifact

	Mike Conley (:mconley) (:⚙️)
	Comment 35 • 6 years ago

Whoops, thanks. :/

	Mike Conley (:mconley) (:⚙️)
	Comment 36 • 6 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13c8633388aecf8590d876424190ec0a4d77c3b2

	Jared Wein [:jaws] (please needinfo? me)
	Comment 37 • 6 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review233342

::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:90
(Diff revision 14)
>    let window2 = await BrowserTestUtils.openNewBrowserWindow();
>  
>    Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR_INACTIVE.join(", ") + ")",

You need to await the theme transition here too because the theme will change when the new window is opened and the current window becomes inactive.

Adding the await here fixed this specific failure for me.

	Mike Conley (:mconley) (:⚙️)
	Comment 40 • 6 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review233666

Hey vivek, can you give us a sense of how many tests are still failing intermittently with this patch?

::: toolkit/components/extensions/test/browser/browser_ext_themes_alpha_accentcolor.js:26
(Diff revision 16)
>    await extension.startup();
>  
>    let docEl = window.document.documentElement;
>    let style = window.getComputedStyle(docEl);
>  
> +  await new Promise(resolve => setTimeout(resolve, 100));

Can't use this, I'm afraid - timeouts are too fragile to depend on.

Instead, we need to wait for the transition to end.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 41 • 6 years ago
mozreview-review

Comment on attachment 8950020 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/219298/#review233742

All tests are fixed with this latest try push,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f99bd84e17e66fc3ca6f4aa420fda4564d3ce247

Feel free to look at the changes in https://hg.mozilla.org/try/rev/abfe93da693d002ed3f47d6d6ddda6948c3aac2e.

I'll upload a patch with the changes folded together so mconley can review them and land it if he approves of it.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 44 • 6 years ago

All green try run (with exception of tests suites failing due to --artifact being used),
https://treeherder.mozilla.org/#/jobs?repo=try&revision=77566df4779d672603ef7deac10e75fde8272263

The test view shows no test failures related to theming,
https://treeherder.mozilla.org/testview.html?repo=try&revision=77566df4779d672603ef7deac10e75fde8272263

	Tim Nguyen :ntim Reporter
	Comment 45 • 6 years ago
mozreview-review

Comment on attachment 8959056 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/227948/#review233778

::: browser/themes/shared/browser.inc.css:36
(Diff revision 2)
>  
>  :root[customizing] #navigator-toolbox::after {
>    border-bottom-style: none;
>  }
>  
> + #nav-bar:-moz-lwtheme {

nit: remove space before rule

	Mike Conley (:mconley) (:⚙️)
	Comment 47 • 6 years ago
mozreview-review

Comment on attachment 8959056 [details]
Bug 1397393 - Make transitioning between 2 themes with a dynamic theme smooth

https://reviewboard.mozilla.org/r/227948/#review234196

Yeah, let's give this a shot. Thanks vivek and jaws!

	Mike Conley (:mconley) (:⚙️)
	Comment 48 • 6 years ago

Unfortunately, I can't autoland this until the opened issue (which I believe jaws fixed in his most recent revision) is marked FIXED or DROPPED. jaws is on PTO today - ntim, since you opened the issue, can you close it so I can autoland?

If I don't hear back by this afternoon, I'll just land on inbound.

	Pulsebot
	Comment 49 • 6 years ago

Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/27d65d2c5909
Make transitioning between 2 themes with a dynamic theme smooth r=mconley

	Raul Gurzau (:RaulG)
	Comment 50 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/27d65d2c5909