(inactive) Nihanth Subramanya [:nhnt11] Reporter
	Description • 7 years ago

STR:
a) Open a private window
b) Open a new tab in that window

The about:privatebrowsing page has a purple background, and a white flash can be observed before the page is visible. I haven't taken any precise measurements, but the duration of the flash ranges between a split second (barely noticeable) to nearly a full second on my fast MacBook.

Additionally, the stop/reload button animates (IMO, it shouldn't), but I'll file a separate bug for that (though it's possible that the fix for one will also fix the other).

	B.J. Herbison
	Comment 1 • 7 years ago

The flash can also be seen when opening additional private tabs in a private window (repeated control-T). The window should stay purple but flashes white well over half the time for me.

Less frequently (five to ten percent of the time) the Tracking Protection toggle is initially displayed as off and transitions to on after the tab is drawn.

	Florian Quèze [:florian] Assignee
	Comment 2 • 7 years ago

This fixes most of the problem, but there are still some issues:
- when opening a new tab in a normal window, because activity stream uses a custom color for its background instead of the --in-content-page-background standard color: http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/browser/extensions/activity-stream/data/content/activity-stream.css#82
- when opening a new private window, the background flashes white. That doesn't happen when opening new tabs in an existing private window. And there are lots of other things that flicker when opening a new private window (pocket icon in the urlbar, 'Nightly' in the identity block, ...).
- when switching tabs and the content process is not responsive, the throbber icon in the center of the content area expects a white background, but gets a gray or purple one.

	Florian Quèze [:florian] Assignee
	Comment 4 • 7 years ago

This is most visible for private tabs (due to the high contrast between white and purple), but also happens in normal tabs.

	hitesh
	Comment 5 • 7 years ago

The white flash in Firefox 57 also occurs when opening certain web pages but not others and is very noticeable of the user is using a dark theme for styling web pages.

On Firefox 55, it is possible to avoid the white flash using

@namespace xul url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul);

@-moz-document url(chrome://browser/content/browser.xul){
browser, browser.display.background_color, tabbrowser tabpanels { background: #050508 !important}  /*prevent white flash; also set "about:blank" to same*/

and

@-moz-document url(about:blank) {
* { background: #050508 !important; }
}

	u601173
	Comment 6 • 7 years ago

(In reply to Florian Quèze [:florian] [:flo] (away until August 28th) from comment #4)
> This is most visible for private tabs (due to the high contrast between
> white and purple), but also happens in normal tabs.

This happens to me when opening many bookmarks and switching between them , white flashes occur before the page is shown especially when the tabs contain only images and then open a newtab

	hitesh
	Comment 7 • 7 years ago

Having userChrome.css with

@namespace xul url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul);

@-moz-document url(chrome://browser/content/browser.xul){
browser, browser.display.background_color, tabbrowser tabpanels { background: #050508 !important}

works in Firefox 57 build 20170828100127 for both normal and private browsing pages.

	Florian Quèze [:florian] Assignee
	Comment 8 • 7 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #2)

> - when opening a new tab in a normal window, because activity stream uses a
> custom color for its background instead of the --in-content-page-background
> standard color:
> http://searchfox.org/mozilla-central/rev/
> b258e6864ee3e809d40982bc5d0d5aff66a20780/browser/extensions/activity-stream/
> data/content/activity-stream.css#82

This was fixed at https://hg.mozilla.org/mozilla-central/rev/e6aa4869c635#l5.44

> - when opening a new private window, the background flashes white.

I'll attach a second patch for this.

> - when switching tabs and the content process is not responsive, the
> throbber icon in the center of the content area expects a white background,
> but gets a gray or purple one.

This is because my CSS rules had a !important cause caused them to override the rule at http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/browser/base/content/tabbrowser.css#95

The !important was needed to override the inline rule set by this obsolete code from bug 558585: http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/browser/base/content/tabbrowser.xml#5703

	Dão Gottwald [:dao]
	Comment 11 • 7 years ago

Comment on attachment 8904532 [details] [diff] [review]
Avoid white flash when opening new tabs, v2

I just learned about this bug. :( As I explained in bug 1382373, this doesn't seem like the right solution. Seems like you already realized that this affects more than just about:newtab in comment 2.

	Florian Quèze [:florian] Assignee
	Comment 12 • 7 years ago

Comment on attachment 8904535 [details] [diff] [review]
Patch - part 2 Avoid white flash when opening new windows

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

::: browser/base/content/tab-content.js
@@ +1059,5 @@
>           .allowScriptsToClose();
>  });
>  
>  addEventListener("MozAfterPaint", function onFirstPaint() {
> +  if (content.document.documentURI == "about:blank")

This unfortunately causes some test failures, at least browser/base/content/test/general/browser_bug495058.js and browser/base/content/test/general/browser_newwindow_focus.js

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e05dd90da269732d1afdf9a0d9fa594e50a7946f&filter-tier=1

	Florian Quèze [:florian] Assignee
	Comment 13 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #11)

While constructive feedback is always welcome, unclear and unactionable drive-by comments like comment 11 are not. Next time, instead of saying "this doesn't seem like the right solution", please take a moment to explain what issues you are seeing in the patch.

> As I explained in bug 1382373, this
> doesn't seem like the right solution.

Now trying to guess what you mean about bug 1382373, no this is not a "rather constructed test case". And it can't be blamed just on activity stream, as about:privatebrowsing exhibits the same behavior. We can't guarantee a tab will always be preloaded, especially now that we preload them during idle time (bug 1353013). And on slower machines these white flashes are more visible than on our fast developer hardware.

My patch only affects the tabbrowser background, the default browser background is unchanged.

> Seems like you already realized that
> this affects more than just about:newtab in comment 2.

All the issues I listed in comment 2 are fixed.

	Dão Gottwald [:dao]
	Comment 14 • 7 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> (In reply to Dão Gottwald [::dao] from comment #11)
> 
> While constructive feedback is always welcome, unclear and unactionable
> drive-by comments like comment 11 are not. Next time, instead of saying
> "this doesn't seem like the right solution", please take a moment to explain
> what issues you are seeing in the patch.
> 
> > As I explained in bug 1382373, this
> > doesn't seem like the right solution.
> 
> Now trying to guess what you mean about bug 1382373, no this is not a
> "rather constructed test case".

That was a misunderstanding of the bug as was cleared up in the following comments. Here are the relevant parts:

> The default background color is relevant not only to the new tab page but also to web pages that aren't going to use the photon color palette

> > This happens every single time a new tab is opened, not just when several
> > are opened in quick succession (if that was the implication?).
> 
> This shouldn't happen since this page is preloaded.
> 
> That delay seems to be the real problem here, not the default browser
> background which I don't think we'll change.


> And it can't be blamed just on activity
> stream, as about:privatebrowsing exhibits the same behavior. We can't
> guarantee a tab will always be preloaded, especially now that we preload
> them during idle time (bug 1353013). And on slower machines these white
> flashes are more visible than on our fast developer hardware.

So we're at a point where we know this only happens when the browser isn't preloaded?

> My patch only affects the tabbrowser background, the default browser
> background is unchanged.

These are just different words referring to the same thing as far as bug 1353013 and this one are concerned.

> > Seems like you already realized that
> > this affects more than just about:newtab in comment 2.
> 
> All the issues I listed in comment 2 are fixed.

The throbber issue doesn't exist anymore? Good. It sounded like another symptom of the same problem I described in bug 1353013: about:newtab's background leaking into contexts where it makes no sense.

	Dão Gottwald [:dao]
	Comment 15 • 7 years ago

Copy/paste fail: I meant to refer to bug 1382373 in my previous comment, not bug 1353013.

	Florian Quèze [:florian] Assignee
	Comment 17 • 7 years ago

Comment on attachment 8904535 [details] [diff] [review]
Patch - part 2 Avoid white flash when opening new windows

Setting the nodefaultsrc (bug 1397365) attribute on the initial browser is a simpler and nicer way to fix the white flash on new windows, so I'm removing this review request (and also not working on fixing the related test failures).

	Florian Quèze [:florian] Assignee
	Comment 18 • 7 years ago

Comment on attachment 8904532 [details] [diff] [review]
Avoid white flash when opening new tabs, v2

(In reply to Dão Gottwald [::dao] from comment #14)

> > The default background color is relevant not only to the new tab page but also to web pages that aren't going to use the photon color palette

I think you are talking about the <browser> background here.

> > > This happens every single time a new tab is opened, not just when several
> > > are opened in quick succession (if that was the implication?).
> > 
> > This shouldn't happen since this page is preloaded.
> > 
> > That delay seems to be the real problem here, not the default browser
> > background which I don't think we'll change.

> > My patch only affects the tabbrowser background, the default browser
> > background is unchanged.
> 
> These are just different words referring to the same thing as far as bug
> 1353013 and this one are concerned.

I'm not changing the default <browser> background, only the <tabbrowser> background.

These are 2 different things. The <browser> background is shown for pages that don't specify a background color (eg. about:blank). These are probably rare these days, but the patch doesn't affect them.

The <tabbrowser> background is shown when we create a new browser, before that browser has painted. It's this background that causes the flash when creating a new tab. I think it makes sense to make this background match the background of new tabs, and that's what my patch does.
The <tabbrowser> background will also be visible when opening a link in a new window, or when opening a link in a new foreground tab (probably a rare action, it requires cmd+shift+click). Given that these days most pages use a non-default background, there'll be a flash of the wrong color anyway, and the new tab background color isn't worse than the default. Actually, showing the purple 'private browsing' background briefly is a nice reminder that something is being loaded in a new private window, I quite like it when testing the patch.

I'm open to keeping the existing <tabbrowser> background for the case where browser.display.use_system_colors is true. That's not the default for builds we ship, but may be the default on some linux distributions. Let me know if you would like me to keep it for that case.

> > And it can't be blamed just on activity
> > stream, as about:privatebrowsing exhibits the same behavior. We can't
> > guarantee a tab will always be preloaded, especially now that we preload
> > them during idle time (bug 1353013). And on slower machines these white
> > flashes are more visible than on our fast developer hardware.
> 
> So we're at a point where we know this only happens when the browser isn't
> preloaded?

We know that a browser not being preloaded is a very simple way to reproduce. I think there are other edge cases, but I don't have good steps to reproduce. By the way, I think we never preload about:privatebrowsing.

> > > Seems like you already realized that
> > > this affects more than just about:newtab in comment 2.
> > 
> > All the issues I listed in comment 2 are fixed.
> 
> The throbber issue doesn't exist anymore? Good. It sounded like another
> symptom of the same problem I described in bug 1353013: about:newtab's
> background leaking into contexts where it makes no sense.

Yeah, it was just a problem with a !important I had in the initial WIP version of the patch, as I said in comment 8.

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

(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> Comment on attachment 8904532 [details] [diff] [review]
> Avoid white flash when opening new tabs, v2
> 
> (In reply to Dão Gottwald [::dao] from comment #14)
> 
> > > The default background color is relevant not only to the new tab page but also to web pages that aren't going to use the photon color palette
> 
> I think you are talking about the <browser> background here.

No, I am not.

> > > > This happens every single time a new tab is opened, not just when several
> > > > are opened in quick succession (if that was the implication?).
> > > 
> > > This shouldn't happen since this page is preloaded.
> > > 
> > > That delay seems to be the real problem here, not the default browser
> > > background which I don't think we'll change.
> 
> > > My patch only affects the tabbrowser background, the default browser
> > > background is unchanged.
> > 
> > These are just different words referring to the same thing as far as bug
> > 1353013 and this one are concerned.
> 
> I'm not changing the default <browser> background, only the <tabbrowser>
> background.
> 
> These are 2 different things. The <browser> background is shown for pages
> that don't specify a background color (eg. about:blank). These are probably
> rare these days, but the patch doesn't affect them.

Like I said, we were talking about the same thing, just using different words. This background is displayed what opening a web page in a new tab.

> The <tabbrowser> background is shown when we create a new browser, before
> that browser has painted. It's this background that causes the flash when
> creating a new tab. I think it makes sense to make this background match the
> background of new tabs, and that's what my patch does.
> The <tabbrowser> background will also be visible when opening a link in a
> new window, or when opening a link in a new foreground tab (probably a rare
> action, it requires cmd+shift+click). Given that these days most pages use a
> non-default background, there'll be a flash of the wrong color anyway, and
> the new tab background color isn't worse than the default.

Do you have data on this? I expect that -moz-default-background-color / white is the most common color, and the best guess we can make for web content.

> I'm open to keeping the existing <tabbrowser> background for the case where
> browser.display.use_system_colors is true. That's not the default for builds
> we ship, but may be the default on some linux distributions. Let me know if
> you would like me to keep it for that case.

We should not regress bug 558585. Note that, as I understand it, your patch would break not only non-default settings but also high-contrast mode.

> > > And it can't be blamed just on activity
> > > stream, as about:privatebrowsing exhibits the same behavior. We can't
> > > guarantee a tab will always be preloaded, especially now that we preload
> > > them during idle time (bug 1353013). And on slower machines these white
> > > flashes are more visible than on our fast developer hardware.
> > 
> > So we're at a point where we know this only happens when the browser isn't
> > preloaded?
> 
> We know that a browser not being preloaded is a very simple way to
> reproduce. I think there are other edge cases, but I don't have good steps
> to reproduce. By the way, I think we never preload about:privatebrowsing.

We should have a better understanding of this before wallpapering over a potential bug here.

	Florian Quèze [:florian] Assignee
	Comment 20 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #19)

> > I'm not changing the default <browser> background, only the <tabbrowser>
> > background.
> > 
> > These are 2 different things. The <browser> background is shown for pages
> > that don't specify a background color (eg. about:blank). These are probably
> > rare these days, but the patch doesn't affect them.
> 
> Like I said, we were talking about the same thing, just using different
> words. This background is displayed what opening a web page in a new tab.

The <tabbrowser> background is not displayed when opening a web page in a background tab (which is the default), because when the user switches tab, either we paint the new browser, or we display the spinner that has its own background.

This <tabbrowser> background is visible when opening a web page in a new foreground tab. I think that's rare, because it requires the cmd+shift modifiers, or a non default value for the "When you open a link in a new tab, switch to it immediately" preference.


> > The <tabbrowser> background is shown when we create a new browser, before
> > that browser has painted. It's this background that causes the flash when
> > creating a new tab. I think it makes sense to make this background match the
> > background of new tabs, and that's what my patch does.
> > The <tabbrowser> background will also be visible when opening a link in a
> > new window, or when opening a link in a new foreground tab (probably a rare
> > action, it requires cmd+shift+click). Given that these days most pages use a
> > non-default background, there'll be a flash of the wrong color anyway, and
> > the new tab background color isn't worse than the default.
> 
> Do you have data on this? I expect that -moz-default-background-color /
> white is the most common color, and the best guess we can make for web
> content.

I don't have scientific data, but Google.com and Youtube.com have some gray background in the top of their pages. amazon.com also has a dark bar at the top and more gray than white on the home page. Facebook has a gray background (maybe with some blue in it). Wikipedia has gray backgrounds at the top and left of the page. None of these major websites look worse with a transition from light gray instead of white before they are first painted.


> > I'm open to keeping the existing <tabbrowser> background for the case where
> > browser.display.use_system_colors is true. That's not the default for builds
> > we ship, but may be the default on some linux distributions. Let me know if
> > you would like me to keep it for that case.
> 
> We should not regress bug 558585.

Bug 558585 was about a white flash when opening a new tab at a time when we didn't have about:newtab (introduced 2 years later in bug 455553), so making the <tabbrowser> background match the about:blank background was the right fix at the time.

> Note that, as I understand it, your patch
> would break not only non-default settings but also high-contrast mode.

It doesn't "break" high-contrast mode, in that flashing light gray is not worse (and I would even say slightly better) than flashing white. Is there a CSS rule I can add to improve the high-contrast situation?

For the non default "use system color" case (which was also part of the steps to reproduce bug 558585), as I said in my previous comment, I'm open to doing:
          if (Services.prefs.getBoolPref("browser.display.use_system_colors"))
            this.style.backgroundColor = "-moz-default-background-color";


> > We know that a browser not being preloaded is a very simple way to
> > reproduce. I think there are other edge cases, but I don't have good steps
> > to reproduce. By the way, I think we never preload about:privatebrowsing.
> 
> We should have a better understanding of this before wallpapering over a
> potential bug here.

I don't think now is the right time in the 57 cycle to change how browser preloading works, so a simple CSS change seems a much nicer fix. I think it actually fixes the problem, but I don't mind if you want to say I'm wallpapering over the problem. Even without the white flash, I think we'll still want to make new tabs load faster in the future, so I don't think the CSS fix will stop us from improving preloading in the future.

	Mike Conley (:mconley) (:⚙️)
	Comment 21 • 7 years ago

Comment on attachment 8904532 [details] [diff] [review]
Avoid white flash when opening new tabs, v2

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

The code looks good, but there's clearly disagreement here, and we might need sign-off / decision from a higher authority before landing. We might want to rope in canuckistani, or perhaps someone from UX.

::: browser/base/content/tabbrowser.xml
@@ -5798,5 @@
>            this._tabListeners.set(this.mCurrentTab, tabListener);
>            this._tabFilters.set(this.mCurrentTab, filter);
>            this.webProgress.addProgressListener(filter, nsIWebProgress.NOTIFY_ALL);
>  
> -          this.style.backgroundColor =

As you mention in the previous comment, I suggest we attempt to keep the !browser.display.use_system_colors && browser.display.background_color combination working if possible.

	Florian Quèze [:florian] Assignee
	Comment 22 • 7 years ago

Updated to preserve the existing "use system color" behavior.

	Florian Quèze [:florian] Assignee
	Comment 23 • 7 years ago

Dao, as I explained in comment 20, I don't think this patch is regressing anything, and I think it's too late to try risky changes to the tab preloading logic for 57. Are you ok with me landing this, or would you like me to involve someone from UX or Product?

	Dão Gottwald [:dao]
	Comment 24 • 7 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> This <tabbrowser> background is visible when opening a web page in a new
> foreground tab. I think that's rare, because it requires the cmd+shift
> modifiers, or a non default value for the "When you open a link in a new
> tab, switch to it immediately" preference.

No, target="_blank" links are affected as well AFAIK. They're not rare at all.

> > Do you have data on this? I expect that -moz-default-background-color /
> > white is the most common color, and the best guess we can make for web
> > content.
> 
> I don't have scientific data, but Google.com and Youtube.com have some gray
> background in the top of their pages. amazon.com also has a dark bar at the
> top and more gray than white on the home page. Facebook has a gray
> background (maybe with some blue in it). Wikipedia has gray backgrounds at
> the top and left of the page. None of these major websites look worse with a
> transition from light gray instead of white before they are first painted.

How is a colored bar at the top relevant? White seems to be the dominant background for most of your examples, so I don't understand why you think transitioning from gray doesn't look worse than transitioning from white.

Obviously lots of sites use random custom backgrounds, but white beats any other color.

> > > I'm open to keeping the existing <tabbrowser> background for the case where
> > > browser.display.use_system_colors is true. That's not the default for builds
> > > we ship, but may be the default on some linux distributions. Let me know if
> > > you would like me to keep it for that case.
> > 
> > We should not regress bug 558585.
> 
> Bug 558585 was about a white flash when opening a new tab at a time when we
> didn't have about:newtab (introduced 2 years later in bug 455553), so making
> the <tabbrowser> background match the about:blank background was the right
> fix at the time.

I remember quite well that it was already relevant for web content opened in a new tab at that time.

> > Note that, as I understand it, your patch
> > would break not only non-default settings but also high-contrast mode.
> 
> It doesn't "break" high-contrast mode, in that flashing light gray is not
> worse (and I would even say slightly better) than flashing white.

No, there's currently no white flash in high-contrast mode. -moz-default-background-color takes care of this.

> > > We know that a browser not being preloaded is a very simple way to
> > > reproduce. I think there are other edge cases, but I don't have good steps
> > > to reproduce. By the way, I think we never preload about:privatebrowsing.
> > 
> > We should have a better understanding of this before wallpapering over a
> > potential bug here.
> 
> I don't think now is the right time in the 57 cycle to change how browser
> preloading works, so a simple CSS change seems a much nicer fix. I think it
> actually fixes the problem, but I don't mind if you want to say I'm
> wallpapering over the problem. Even without the white flash, I think we'll
> still want to make new tabs load faster in the future, so I don't think the
> CSS fix will stop us from improving preloading in the future.

The expectation should be that this bug isn't an issue as long as about:newtab is preloaded. That's what I see. Another expectation should be that about:newtab is preloaded most of the time under normal circumstances (e.g. unless you open it twice in a row). Regressing that behavior significantly certainly wasn't my intention when I started working on bug 1353013.

So it's still not clear to me that this bug is a big deal, and I'd rather land nothing at all than this patch that prioritizes about:newtab over web content.

	Dão Gottwald [:dao]
	Comment 25 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #24)
> So it's still not clear to me that this bug is a big deal

More precisely, the about:newtab issue. That's also minor even if it does happen, because about:newtab's gray background is very close to white.

Apparently this bug was originally filed for about:privatebrowsing, and I agree it's more severe there. Maybe it's easy enough to preload about:privatebrowsing like we do for about:newtab; that would be my preferred fix. If that's not an option, a more targeted CSS fix specifically for about:privatebrowsing would be appropriate.

	Florian Quèze [:florian] Assignee
	Comment 26 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #24)

> > > Note that, as I understand it, your patch
> > > would break not only non-default settings but also high-contrast mode.
> > 
> > It doesn't "break" high-contrast mode, in that flashing light gray is not
> > worse (and I would even say slightly better) than flashing white.
> 
> No, there's currently no white flash in high-contrast mode.

Really? See the attached video, captured with today's nightly on the quantum reference machine in high contrast mode.

	Florian Quèze [:florian] Assignee
	Comment 27 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #25)
> (In reply to Dão Gottwald [::dao] from comment #24)
> > So it's still not clear to me that this bug is a big deal
> 
> More precisely, the about:newtab issue. That's also minor even if it does
> happen, because about:newtab's gray background is very close to white.

I don't understand how you can assert here that a flash from white to light gray is a minor thing, but also argue in the previous comments that using the same light gray instead of white as the background before loading web content with an unknown content-provided background color is a significant regression.

	Dão Gottwald [:dao]
	Comment 28 • 7 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> Created attachment 8906036 [details]
> High contrast screen recording
> 
> (In reply to Dão Gottwald [::dao] from comment #24)
> 
> > > > Note that, as I understand it, your patch
> > > > would break not only non-default settings but also high-contrast mode.
> > > 
> > > It doesn't "break" high-contrast mode, in that flashing light gray is not
> > > worse (and I would even say slightly better) than flashing white.
> > 
> > No, there's currently no white flash in high-contrast mode.
> 
> Really? See the attached video, captured with today's nightly on the quantum
> reference machine in high contrast mode.

Apparently browser.display.use_system_colors defaults to false, which makes no sense in dark high-contrast mode. With the pref flipped there should be no white flash. I'll have to file a new bug to get us reasonable default behavior without requiring high-contrast users to jump through hoops...

(In reply to Florian Quèze [:florian] [:flo] from comment #27)
> (In reply to Dão Gottwald [::dao] from comment #25)
> > (In reply to Dão Gottwald [::dao] from comment #24)
> > > So it's still not clear to me that this bug is a big deal
> > 
> > More precisely, the about:newtab issue. That's also minor even if it does
> > happen, because about:newtab's gray background is very close to white.
> 
> I don't understand how you can assert here that a flash from white to light
> gray is a minor thing, but also argue in the previous comments that using
> the same light gray instead of white as the background before loading web
> content with an unknown content-provided background color is a significant
> regression.

The mismatch is pretty minor either way. I just don't think we should regress one for the other.

	Florian Quèze [:florian] Assignee
	Comment 29 • 7 years ago

Comment on attachment 8905843 [details] [diff] [review]
Patch v3

Requesting a final UI decision on this from Philipp. There's a try build with the patches at https://treeherder.mozilla.org/#/jobs?repo=try&revision=c76d2107c4c129c7827c8f1354b7540f37ed0fb4

	Florian Quèze [:florian] Assignee
	Comment 30 • 7 years ago

Comment on attachment 8904535 [details] [diff] [review]
Patch - part 2 Avoid white flash when opening new windows

I obsoleted this patch in comment 17 because bug 1397365 is a better solution, but it's unclear at this point if it'll make 57, so requesting review here again.

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 31 • 7 years ago

Comment on attachment 8905843 [details] [diff] [review]
Patch v3

Just played with a try build of this for a while. It actually does have some positive impact in the new tab case (especially in PBM, but also outside of it)! It is one fewer flashes new tabs and windows.

IMO, the key aspect here is that when loading a web page, there is more of an expectation of change in the viewport than on a tab open. Therefore, if we can't avoid a (slight) flicker in both cases, it makes more sense to have it in some rare cases when a website is loading, rather than when the newtab page is loading.

	Mike Conley (:mconley) (:⚙️)
	Comment 32 • 7 years ago

Comment on attachment 8904535 [details] [diff] [review]
Patch - part 2 Avoid white flash when opening new windows

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

Seems okay on the surface, but I'd like to see the patch in context with the changes from bug 1394765, and an explanation for the tab-content.js change please. Thanks!

::: browser/base/content/browser.js
@@ +1258,5 @@
>            remoteType = linkedBrowser.remoteType;
>            isRemote = remoteType != E10SUtils.NOT_REMOTE;
>            sameProcessAsFrameLoader = linkedBrowser.frameLoader;
>          }
> +        initBrowser.removeAttribute("blank");

Pretty sure this is going to bitrot since bug 1394765 landed... can you update this patch, please?

::: browser/base/content/tab-content.js
@@ +1059,5 @@
>           .allowScriptsToClose();
>  });
>  
>  addEventListener("MozAfterPaint", function onFirstPaint() {
> +  if (content.document.documentURI == "about:blank")

Is this test failure still a problem?

I'm also not sure why we need this part. Can you explain it to me?

	Florian Quèze [:florian] Assignee
	Comment 33 • 7 years ago

(In reply to Mike Conley (:mconley) (:⚙️) from comment #32)

> ::: browser/base/content/tab-content.js
> @@ +1059,5 @@
> >           .allowScriptsToClose();
> >  });
> >  
> >  addEventListener("MozAfterPaint", function onFirstPaint() {
> > +  if (content.document.documentURI == "about:blank")
> 
> Is this test failure still a problem?

I don't remember... so it likely still is :-(.

> I'm also not sure why we need this part. Can you explain it to me?

Very similar to the bug 1397365 story: we sometimes get an about:blank paint in the browser before the real page. Removing the "blank" attribute in that case defeats the purpose of the patch and causes a white flash to still occur.

I don't have proof of why we sometimes have this about:blank, but it seems to happen very little for the first 4 windows and much more after that, so my guess is: when we have to create a new content process for the <browser> in the new window, creating the content process takes some time and by the time it's ready to load something, we have already sent it the URL it needs to load, so the first page it paints is the page we care about.
When reusing existing content processes, the new browser (ready immediately) quickly sends a first paint message for about:blank before loading the page we care about.

	Florian Quèze [:florian] Assignee
	Comment 34 • 7 years ago

Unbitrotted and avoids the test failure, at the cost of adding an additional message exchanged between the content and parent processes that I would be happy to remove once bug 1397365 is fixed... but this whole patch should be backed out anyway once that bug is fixed.

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

Comment on attachment 8908277 [details] [diff] [review]
Patch part 2 - v2

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

r=me assuming a green try run, and responses to the below comments. Thanks!

::: browser/base/content/browser.js
@@ +1262,5 @@
>          remoteType = linkedBrowser.remoteType;
>          isRemote = remoteType != E10SUtils.NOT_REMOTE;
>          sameProcessAsFrameLoader = linkedBrowser.frameLoader;
>        }
> +      initBrowser.removeAttribute("blank");

Is this necessary? Won't this just get removed once the Browser:FirstNonBlankPaint message fires?

::: browser/base/content/tab-content.js
@@ +1055,5 @@
> +addEventListener("MozAfterPaint", function onFirstNonBlankPaint() {
> +  if (content.document.documentURI == "about:blank")
> +    return;
> +  removeEventListener("MozAfterPaint", onFirstNonBlankPaint);
> +  sendAsyncMessage("Browser:FirstNonBlankPaint");

We could have probably sent this with the Browser:FirstPaint message with some data in the message saying whether or not it's for a blank paint, but splitting it out like this probably allows us to remove it more easily. Can you please file a bug (or even better, update gandalf's bug with a patch!) to remove this when the nodefaultsrc stuff is ready?

	Florian Quèze [:florian] Assignee
	Comment 36 • 7 years ago

(In reply to Mike Conley (:mconley) (:⚙️) from comment #35)

> ::: browser/base/content/browser.js
> @@ +1262,5 @@
> >          remoteType = linkedBrowser.remoteType;
> >          isRemote = remoteType != E10SUtils.NOT_REMOTE;
> >          sameProcessAsFrameLoader = linkedBrowser.frameLoader;
> >        }
> > +      initBrowser.removeAttribute("blank");
> 
> Is this necessary? Won't this just get removed once the
> Browser:FirstNonBlankPaint message fires?

Yes, we very much want this attribute to be removed before first paint in that case to ensure the new window is first painted with the docshell already swapped in (or we would regress bug 1391704). The Browser:FirstNonBlankPaint message listener is added in _handleURIToLoad which runs during _delayedStartup, so after first paint.

> ::: browser/base/content/tab-content.js
> @@ +1055,5 @@
> > +addEventListener("MozAfterPaint", function onFirstNonBlankPaint() {
> > +  if (content.document.documentURI == "about:blank")
> > +    return;
> > +  removeEventListener("MozAfterPaint", onFirstNonBlankPaint);
> > +  sendAsyncMessage("Browser:FirstNonBlankPaint");
> 
> We could have probably sent this with the Browser:FirstPaint message with
> some data in the message saying whether or not it's for a blank paint,

I considered that (sending the painted url as data in the existing message), but it adds complication on both sides: on the content side we can't remove the eventListener until we have a non-blank paint. And on the parent side we also need to wait until the non-blank paint to remove the message listener, but still resolve the promise for the very first paint to support loading about:blank. I decided it wasn't worth it.

> splitting it out like this probably allows us to remove it more easily.

Indeed!

> Can
> you please file a bug (or even better, update gandalf's bug with a patch!)
> to remove this when the nodefaultsrc stuff is ready?

Once this lands, I'll tell gandalf to add a backout of it in his patchset, and r+ it :-).

	Florian Quèze [:florian] Assignee
	Comment 37 • 7 years ago

These 2 tests rely on scrolling on browsers that are not painted yet, so the blank attribute gets in their way. Just waiting 100ms (enough time for the message to be received from the content process and processed by the parent process) with a setTimeout is enough to unbreak this locally, but I figured removing the blank attribute is less fragile.

	Pulsebot
	Comment 38 • 7 years ago

Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e14317d2f6c
White flash when opening new tabs, r=mconley, ui-r=phlsa.

	Florian Quèze [:florian] Assignee
	Comment 39 • 7 years ago

For now I landed only the first part that doesn't cause test failures.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08695fcb17ec0e0897a2bb3872626cbc1a2b389e is a try push with my test fix. If this turns out to be green and Mike gets time to review the test fix today, we'll decide if we are still ok with landing the second part for 57.

	Ryan VanderMeulen [:RyanVM]
	Comment 40 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0e14317d2f6c

	Mike Conley (:mconley) (:⚙️)
	Comment 41 • 7 years ago

Comment on attachment 8908634 [details] [diff] [review]
Fix tests

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

r=me

Let's be extra vigilant about watching Bugzilla these next few days - if we hear anybody talking about initial tabs staying blank or something, we should back this stuff right out.

Thanks!

::: dom/events/test/test_bug574663.html
@@ +102,5 @@
> +
> +  let waitUntilPainted = function(callback) {
> +    // Until the first non-blank paint, the parent will set the opacity of our
> +    // browser to 0 using the 'blank' attribute.
> +    // Until the blank attribute is removed, we can't send scroll events.

I assume this is something we'll want to remove once gandalf's nodefaultsrc stuff lands? Probably worth pointing him at it, if so.

::: dom/events/test/window_wheel_default_action.html
@@ +56,5 @@
> +    Components.utils.import("resource://gre/modules/Services.jsm");
> +    Services.wm.getMostRecentWindow("navigator:browser")
> +            .gBrowser.selectedBrowser.removeAttribute("blank");
> +  });
> +  runTests();

Shouldn't we wait until we paint? Unless I'm mistaken, we're not waiting until the blank has been removed in the parent.

	Florian Quèze [:florian] Assignee
	Comment 42 • 7 years ago

(In reply to Mike Conley (:mconley) (:⚙️) from comment #41)
> Comment on attachment 8908634 [details] [diff] [review]
> Fix tests
> 
> Review of attachment 8908634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me

Thanks!

> Let's be extra vigilant about watching Bugzilla these next few days - if we
> hear anybody talking about initial tabs staying blank or something, we
> should back this stuff right out.

Agreed.

> ::: dom/events/test/test_bug574663.html
> @@ +102,5 @@
> > +
> > +  let waitUntilPainted = function(callback) {
> > +    // Until the first non-blank paint, the parent will set the opacity of our
> > +    // browser to 0 using the 'blank' attribute.
> > +    // Until the blank attribute is removed, we can't send scroll events.
> 
> I assume this is something we'll want to remove once gandalf's nodefaultsrc
> stuff lands? Probably worth pointing him at it, if so.

Yes.

> ::: dom/events/test/window_wheel_default_action.html
> @@ +56,5 @@
> > +    Components.utils.import("resource://gre/modules/Services.jsm");
> > +    Services.wm.getMostRecentWindow("navigator:browser")
> > +            .gBrowser.selectedBrowser.removeAttribute("blank");
> > +  });
> > +  runTests();
> 
> Shouldn't we wait until we paint? Unless I'm mistaken, we're not waiting
> until the blank has been removed in the parent.

Maybe. I wondered the same thing, but in practice this seems to be enough to fix the test, so I didn't want to duplicate all the waitForPaint code from the other test. I think this works because, before really starting, the test does a SpecialPowers.pushPrefEnv call and waits for the callback, so we already wait for the other process.

	Pulsebot
	Comment 43 • 7 years ago

Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95af17933c45
Avoid white flash when opening new windows, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0533dbf29568
fix tests that rely on scrolling on a not-yet-painted browser, r=mconley.

	Florian Quèze [:florian] Assignee
	Comment 44 • 7 years ago

(In reply to Pulsebot from comment #43)
> Pushed by florian@queze.net:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/95af17933c45
> Avoid white flash when opening new windows, r=mconley.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0533dbf29568
> fix tests that rely on scrolling on a not-yet-painted browser, r=mconley.

Note for sheriffs: dom/events/test/test_bug574663.html is known to fail intermittently even with my test fix, on https://treeherder.mozilla.org/#/jobs?repo=try&revision=08695fcb17ec0e0897a2bb3872626cbc1a2b389e it failed twice out of 11 runs of test-macosx64/debug-mochitest-e10s-2. Without the test fix it was a perma fail on all OSes on both opt and debug.

We decided to land tonight anyway so that we can maximize nightly testing for the Firefox change, in order to reduce risk for 57. If dom/events/test/test_bug574663.html fails intermittently in the next few days, I would like to request that we either reopen bug 1319655 and star it there, or disable the test for debug builds. I can try to fix the intermittent failure next week. Thanks!

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 45 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/95af17933c45
https://hg.mozilla.org/mozilla-central/rev/0533dbf29568

	Anika Alam Raha
	Comment 46 • 7 years ago

I have reproduced this bug with Nightly 56.0a1 (2017-07-10)  on Windows 8, 64-bit.

The bug's fix is now verified on Latest Nightly

Build ID 	20170916220246
User Agent 	Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170913]

	Khvoinitsky Mikhail [:m_khvoinitsky]
	Comment 51 • 6 years ago

There is still a problem when using Dark Firefox theme with dark websites and/or add-ons that make web dark (note: I'm a developer of such one). Again, because of #tabbrowser-tabpanels.

I've created a patch which fixes it. However, I'm not quite sure that var(--chrome-background-color) is the best choice for that. Please, take a look.

Or should I create separate bug for this?

	Dão Gottwald [:dao]
	Comment 52 • 6 years ago

Comment on attachment 9005907 [details] [diff] [review]
dark theme fix

(In reply to Khvoinitsky Mikhail [:m_khvoinitsky] from comment #51)
> Or should I create separate bug for this?

Yep.

	Khvoinitsky Mikhail [:m_khvoinitsky]
	Comment 53 • 6 years ago

Done: https://bugzilla.mozilla.org/show_bug.cgi?id=1488384