Bug 45640

Summary: Navigating dark background websites results in blinding white flashes between pages.
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: Layout and RenderingAssignee: Hajime Morrita <morrita>
Status: REOPENED ---    
Severity: Normal CC: eric, fishd, hyatt, jamesr, jerem.selier, kling, koivisto, mahaniok, mitz, tkent, tony, webkitb, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
fixed typo
none
A server script to reproduce
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Hajime Morrita 2010-09-13 00:05:03 PDT
This bug is a placeholder of two-sided patch for http://code.google.com/p/chromium/issues/detail?id=1373.
Comment 1 Hajime Morrita 2010-09-13 00:18:27 PDT
Created attachment 67368 [details]
Patch
Comment 2 Hajime Morrita 2010-09-13 00:40:56 PDT
Chromium side is: http://codereview.chromium.org/3316015/show
Comment 3 Hajime Morrita 2010-09-14 01:53:59 PDT
Created attachment 67527 [details]
Patch
Comment 4 Hajime Morrita 2010-09-14 01:55:05 PDT
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.
Comment 5 Kent Tamura 2010-09-14 02:11:53 PDT
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.
Comment 6 Hajime Morrita 2010-09-14 02:50:25 PDT
Created attachment 67531 [details]
fixed typo
Comment 7 Hajime Morrita 2010-09-14 02:51:26 PDT
(In reply to comment #5)
> I guess you'd like to rename isReadytoPaint to isReadyToPaint.
Sure. Thank you for pointing this out.
Comment 8 Darin Fisher (:fishd, Google) 2010-09-14 09:09:05 PDT
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.
Comment 9 Hajime Morrita 2010-09-16 04:37:39 PDT
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.
Comment 10 Hajime Morrita 2010-09-21 03:22:12 PDT
Created attachment 68205 [details]
A server script to reproduce
Comment 11 Hajime Morrita 2010-09-21 03:25:24 PDT
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.
Comment 12 Hajime Morrita 2010-10-12 05:27:46 PDT
Created attachment 70521 [details]
Patch
Comment 13 Hajime Morrita 2010-10-12 05:31:42 PDT
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.
Comment 14 Dave Hyatt 2010-10-12 10:39:14 PDT
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.
Comment 15 Hajime Morrita 2010-10-13 01:19:47 PDT
Created attachment 70587 [details]
Patch
Comment 16 Hajime Morrita 2010-10-13 01:34:55 PDT
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).
Comment 17 Dave Hyatt 2010-10-20 14:57:49 PDT
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.
Comment 18 Hajime Morrita 2010-10-21 02:34:29 PDT
Created attachment 71411 [details]
Patch
Comment 19 Hajime Morrita 2010-10-21 02:36:51 PDT
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".
Comment 20 Hajime Morrita 2010-10-29 00:50:02 PDT
Committed r70850: <http://trac.webkit.org/changeset/70850>
Comment 21 Nikolas Zimmermann 2010-10-29 06:14:02 PDT
(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?
Comment 22 Tony Chang 2010-10-29 10:32:45 PDT
Reverted r70850 for reason:

chromium and qt pixel test regressions

Committed r70896: <http://trac.webkit.org/changeset/70896>
Comment 23 Tony Chang 2010-10-29 10:34:05 PDT
(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
Comment 24 Andreas Kling 2010-10-29 11:46:07 PDT
(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().
Comment 25 Hajime Morrita 2010-10-31 19:14:27 PDT
Tony, thank you for taking care of this and I'm sorry for my absence. 
I'll investigate this soon.
Comment 26 Hajime Morrita 2010-11-01 00:55:10 PDT
Created attachment 72489 [details]
Patch
Comment 27 Hajime Morrita 2010-11-01 01:01:37 PDT
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.
Comment 28 Andreas Kling 2010-11-01 05:25:49 PDT
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.
Comment 29 Hajime Morrita 2010-11-01 05:42:29 PDT
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.
Comment 30 Andreas Kling 2010-11-01 06:03:28 PDT
(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.
Comment 31 Hajime Morrita 2010-11-01 20:49:14 PDT
Created attachment 72627 [details]
Patch
Comment 32 Hajime Morrita 2010-11-01 20:52:11 PDT
> 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().
Comment 33 Dave Hyatt 2010-11-18 21:49:31 PST
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.
Comment 34 Hajime Morrita 2010-11-18 22:21:35 PST
Created attachment 74354 [details]
Patch
Comment 35 Dave Hyatt 2010-11-18 22:22:56 PST
Comment on attachment 74354 [details]
Patch

r=me
Comment 36 Hajime Morrita 2010-11-18 22:25:52 PST
Committed r72367: <http://trac.webkit.org/changeset/72367>
Comment 37 Antti Koivisto 2011-04-14 07:10:21 PDT
This is a very slow way to fix this.

https://bugs.webkit.org/show_bug.cgi?id=58512
Comment 38 Hajime Morrita 2011-04-15 17:50:22 PDT
Reopening because the original change was reverted.
http://trac.webkit.org/changeset/84066
Comment 39 Darin Adler 2011-06-18 12:17:03 PDT
Comment on attachment 74354 [details]
Patch

Clearing review flag because the patch was reverted.