Bug 45640 – Navigating dark background websites results in blinding white flashes between pages.

• Component: Layout and Rendering
• Status: REOPENED

[Hajime Morrita]

This bug is a placeholder of two-sided patch for http://code.google.com/p/chromium/issues/detail?id=1373.

[Hajime Morrita]

Created attachment 67368 [details]
Patch

[Hajime Morrita]

Chromium side is: http://codereview.chromium.org/3316015/show

[Hajime Morrita]

Created attachment 67527 [details]
Patch

[Hajime Morrita]

Revised the patch based on the advice on http://codereview.chromium.org/3316015/show.
This patch is no longer 2-sided. It is now self contained.

[Kent Tamura]

Comment on attachment 67527 [details]
Patch

> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> +        (WebKit::WebFrameImpl::isReadyToPaint): Added.
> +        * src/WebFrameImpl.h:
> +        * src/WebViewImpl.cpp:
> +        (WebKit::WebViewImpl::mainFrameImpl): Qualified with const.
> +        (WebKit::WebViewImpl::isReadytoPaint): Added, just delegating to the  main frame.

I guess you'd like to rename isReadytoPaint to isReadyToPaint.

[Hajime Morrita]

Created attachment 67531 [details]
fixed typo

[Hajime Morrita]

(In reply to comment #5)
> I guess you'd like to rename isReadytoPaint to isReadyToPaint.
Sure. Thank you for pointing this out.

[Darin Fisher (:fishd, Google)]

Comment on attachment 67531 [details]
fixed typo

View in context: https://bugs.webkit.org/attachment.cgi?id=67531&action=prettypatch

> WebKit/chromium/ChangeLog:5
> +        [Chromium] WebView should provide the way to query paint readiness
this summary is no longer accurate since you are not exposing a webkit api

> WebKit/chromium/src/ChromeClientImpl.cpp:499
> +        if (m_webView->client() && m_webView->isReadyToPaint())
what about the case where accelerated compositing is active?

> WebKit/chromium/src/WebFrameImpl.cpp:1854
> +    // including <style>s are totally loaded. Also note that: 
this suggests that DOM construction is deferred until external stylesheets
are downloaded.  i didn't think that was the case.  i know it is for
external scripts, but that's because an external script may contain a
document.write() call.

> WebKit/chromium/src/WebFrameImpl.cpp:1857
> +    // - A <frameset> can be retunred for body().
nit: retunred -> returned

> WebKit/chromium/src/WebViewImpl.h:235
> +    WebFrameImpl* mainFrameImpl() const;
why make this const?  it does not return a const WebFrameImpl, so it
would appear to function much like a const_cast stripping away the
const on the WebViewImpl.

By the way, it seems like it would be possible to implement this entire
patch on the Chromium side.  We have WebDocument::isHTMLDocument() and
WebDocument::body().  Ignoring a didInvalidateRect call would be easy to
do on the Chromium side too.

It may be a bit simpler to just skip the PostTask(CallDoDeferredUpdate)
in chrome/renderer/render_widget.cc when there is no body element.

[Hajime Morrita]

Hi Darin, 
Thank you for reviewing and I'm sorry for my slow response.

> By the way, it seems like it would be possible to implement this entire
> patch on the Chromium side.  We have WebDocument::isHTMLDocument() and
> WebDocument::body().  Ignoring a didInvalidateRect call would be easy to
> do on the Chromium side too.
> 
> It may be a bit simpler to just skip the PostTask(CallDoDeferredUpdate)
> in chrome/renderer/render_widget.cc when there is no body element.
This totally makes sense.
So I ported these code to chromium side with your feedback in mind:
http://codereview.chromium.org/3397008
As you guessed, the intention get clear.
Could you take a look?

I'll close this bug when chromium side lands.

[Hajime Morrita]

Created attachment 68205 [details]
A server script to reproduce

[Hajime Morrita]

I changed a summary line because this problem reproduces even in Safari.
(It does reproduce more often in Chromium though.)

You can reproduce the problem with running the script https://bugs.webkit.org/attachment.cgi?id=68205
and accessing http://localhost:8080/, then reloading it.

[Hajime Morrita]

Created attachment 70521 [details]
Patch

[Hajime Morrita]

Hi Darin, and folks familiar with this area, could you take a look?
Original problem is frequently complained by so many users...

The latest patch is stays solely in WebCore and tried to tackle the root cause instead of 
working around in WebKit layer.

[Dave Hyatt]

Comment on attachment 70521 [details]
Patch

Whether or not you can paint safely is based off whether you have pending stylesheets.  You can see some code that checks this and prevents the painting of children in that case in (for example) in RenderLayer.cpp.


// Avoid painting layers when stylesheets haven't loaded.  This eliminates FOUC.
    // It's ok not to draw, because later on, when all the stylesheets do load, updateStyleSelector on the Document
    // will do a full repaint().
    if (renderer()->document()->didLayoutWithPendingStylesheets() && !renderer()->isRenderView() && !renderer()->isRoot())
        return;

That code is essentially running "too late", so you get a flash of white when the <body> on down don't paint.  The goal of your patch should be to pull that check up.  I think if your hasPaintableContent just does the didLayoutWithPendingStylesheets() check that it might fix the bug.

In theory you could remove the places that are already checking this if you're going to choke off the painting at a higher level like this.

[Hajime Morrita]

Created attachment 70587 [details]
Patch

[Hajime Morrita]

Hi Hyatt, thank you for reviewing!
I updated the patch.

> 
> That code is essentially running "too late", so you get a flash of white when the <body> on down don't paint.  The goal of your patch should be to pull that check up.  I think if your hasPaintableContent just does the didLayoutWithPendingStylesheets() check that it might fix the bug.

This totally makes sense. Thanks much for your suggestion.
The point is that many web sites specify its background style to <body> element, 
instead of <html> element. So before <body> is available, the FOUC is possible
even if their stylesheets are loaded.
On the other hand, checking <body> isn't enough as you mentioned.
So the updated patch extracted didLayoutWithPendingStylesheets() checks on renderers 
to mayCauseFlashOfUnstyledContent() and added a check for <body> availability. 

> In theory you could remove the places that are already checking this if you're going to choke off the painting at a higher level like this.

Sure. I did cleanup the patch to use mayCauseFlashOfUnstyledContent() both 
for skipping painting requests and skipping actual paint.
And I also removed the guard on the FrameView::paintContent(). So there is no longer duplication
on the FOUC guards (I hope).

[Dave Hyatt]

Comment on attachment 70587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70587&action=review

> WebCore/dom/Document.cpp:4770
> +    if (document()->didLayoutWithPendingStylesheets())

Don't need document()-> here.

> WebCore/dom/Document.cpp:4772
> +    // Many websites give thier background style using <body> instead of <html>.

Typo. "thier" should be "their"

> WebCore/dom/Document.cpp:4776
> +    if ((isHTMLDocument() || isXHTMLDocument()) && !body())
> +        return true;
> +    return false;

Can just write this as:

return (isHTMLDocument() || isXHtmlDocument()) && !body();

I'd also flip the check.  It's cheaper to ask if body() is present then to make the virtual function call to ask what kind of document you are.

> WebCore/page/FrameView.cpp:306
> +        if (hostWindow() && shouldUpdate(false))

Don't need "false" here.  It's the default value.

[Hajime Morrita]

Created attachment 71411 [details]
Patch

[Hajime Morrita]

Hi Dave, thank you for taking a look agian!
I update the patch to address your feedback.
 
(In reply to comment #17)
> > WebCore/dom/Document.cpp:4770
> > +    if (document()->didLayoutWithPendingStylesheets())
> 
Fixed.

> Don't need document()-> here.
> 
> > WebCore/dom/Document.cpp:4772
> > +    // Many websites give thier background style using <body> instead of <html>.
> 
> Typo. "thier" should be "their"
Oops. Done.

> 
> > WebCore/dom/Document.cpp:4776
> > +    if ((isHTMLDocument() || isXHTMLDocument()) && !body())
> > +        return true;
> > +    return false;
> 
> Can just write this as:
> 
> return (isHTMLDocument() || isXHtmlDocument()) && !body();
> 
> I'd also flip the check.  It's cheaper to ask if body() is present then to make the virtual function call to ask what kind of document you are.
Fixed. and made "!body()" first.

> 
> > WebCore/page/FrameView.cpp:306
> > +        if (hostWindow() && shouldUpdate(false))
> 
> Don't need "false" here.  It's the default value.
Fixed removing "false".

[Hajime Morrita]

Committed r70850: <http://trac.webkit.org/changeset/70850>

[Nikolas Zimmermann]

(In reply to comment #20)
> Committed r70850: <http://trac.webkit.org/changeset/70850>

This broke several SVG tests, that now stopped rendering aka. show blank instead of content.

svg-in-xhtml.xhtml 
svg/custom/createImageElement2.xhtml 
svg-element-in-xhtml-auto.xhtml 
svg-element-in-xhtml-defaults.xhtml 
svg-element-in-xhtml-hidden.xhtml 
svg-element-in-xhtml-scroll.xhtml 
svg-element-in-xhtml-visible.xhtml 

Can you investigate or roll it out?

[Tony Chang]

Reverted r70850 for reason:

chromium and qt pixel test regressions

Committed r70896: <http://trac.webkit.org/changeset/70896>

[Tony Chang]

(In reply to comment #22)
> Reverted r70850 for reason:
> 
> chromium and qt pixel test regressions
> 
> Committed r70896: <http://trac.webkit.org/changeset/70896>

Reverted so Morita can investigate the test failures next week.  The chromium pixel test failures are tracked here:
https://bugs.webkit.org/show_bug.cgi?id=48627

[Andreas Kling]

(In reply to comment #21)
> (In reply to comment #20)
> > Committed r70850: <http://trac.webkit.org/changeset/70850>
> 
> This broke several SVG tests, that now stopped rendering aka. show blank instead of content.
> 
> svg-in-xhtml.xhtml 
> svg/custom/createImageElement2.xhtml 
> svg-element-in-xhtml-auto.xhtml 
> svg-element-in-xhtml-defaults.xhtml 
> svg-element-in-xhtml-hidden.xhtml 
> svg-element-in-xhtml-scroll.xhtml 
> svg-element-in-xhtml-visible.xhtml 
> 
> Can you investigate or roll it out?

The problem appears to be the lack of an xmlns attribute on the <html> elements in those test documents. This causes Document::body() (used by the new mayCauseFlashOfUnstyledContent() function) to fail since it uses Element::hasTagName(bodyTag) to identify the <body> element.

If the document is parsed as XHTML but has no xmlns attribute, the <body> element will be in the "" namespace which doesn't match bodyTag.namespaceURI().

[Hajime Morrita]

Tony, thank you for taking care of this and I'm sorry for my absence. 
I'll investigate this soon.

[Hajime Morrita]

Created attachment 72489 [details]
Patch

[Hajime Morrita]

Hi Kling,

> The problem appears to be the lack of an xmlns attribute on the <html> elements in those test documents. This causes Document::body() (used by the new mayCauseFlashOfUnstyledContent() function) to fail since it uses Element::hasTagName(bodyTag) to identify the <body> element.

Thank you for your insight!
This is completely makes sense. In broken tests, their document.body is null.
I didn't noticed that!

Could you take a look at the updated patch?
The fix looks to need eyes from non-HTML experts.
I changed Document::mayCauseFlashOfUnstyledContent() to check 
if there are elements other than <head>.
This is more conservative compared to the original one so there are smaller chance to
accidentally skip painting. 
Now all SVG pixel tests are passed on Snow Leopard.

[Andreas Kling]

Comment on attachment 72489 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72489&action=review

> WebCore/dom/Document.cpp:4808
> +        if (!i->hasTagName(headTag))
> +            return true;

This logic is nicer than the previous version IMO, but will trigger for the <head> tag, too, if the xmlns attribute is missing.
In other words, Node::hasTagName(headTag) will be false for a <head> tag in an "application/xhtml+xml" document with no xmlns attribute on the <html> tag.
I'm not sure that's what you want.

[Hajime Morrita]

Hi Kling, thank you for reviewing this!

> This logic is nicer than the previous version IMO, but will trigger for the <head> tag, too, if the xmlns attribute is missing.
> In other words, Node::hasTagName(headTag) will be false for a <head> tag in an "application/xhtml+xml" document with no xmlns attribute on the <html> tag.
> I'm not sure that's what you want.

Yeah, it's OK to trigger when non-HTML <head> tag, because the page author might want to render it.
(a <head>element from a foreign namespace.)
 
The intention here is to wait painting during only HTML <head> is given,
to prevent flushing a white screen (because page authors often give a style for <body>, not <html>.)
It's safe to return true always, except that it cannot prevent possible flushing as current as in the past.
It is not safe to return false accidentally though. Which is what the reverted patch did.

So assuming such <head> is minor, I think it's OK to return true.

[Andreas Kling]

(In reply to comment #29)
> Yeah, it's OK to trigger when non-HTML <head> tag, because the page author might want to render it.
> (a <head>element from a foreign namespace.)

I'm fine with the code change, but I'd like to see a comment in there about how this only works "properly" for HTML documents.

[Hajime Morrita]

Created attachment 72627 [details]
Patch

[Hajime Morrita]

> I'm fine with the code change, but I'd like to see a comment in there about how this only works "properly" for HTML documents.
Okay. I added an explanation as a comment on Document::mayCauseFlashOfUnstyledContent().

[Dave Hyatt]

Comment on attachment 72627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72627&action=review

Just one minor adjustment I'd like to see....

> WebCore/dom/Document.cpp:4798
> +bool Document::hasHeadSibling() const

I think this could just be a static function in the cpp file without adding it as a member function to Document.h.

[Hajime Morrita]

Created attachment 74354 [details]
Patch

[Dave Hyatt]

Comment on attachment 74354 [details]
Patch

r=me

[Hajime Morrita]

Committed r72367: <http://trac.webkit.org/changeset/72367>

[Antti Koivisto]

This is a very slow way to fix this.

https://bugs.webkit.org/show_bug.cgi?id=58512

[Hajime Morrita]

Reopening because the original change was reverted.
http://trac.webkit.org/changeset/84066

[Darin Adler]

Comment on attachment 74354 [details]
Patch

Clearing review flag because the patch was reverted.