Bug 149157

Summary: Allow the page to render before <link> stylesheet tags in body
Product: WebKit Reporter: Jake Archibald <jaffathecake>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, eoconnor, koivisto, ryanhaddad, sam, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 169079, 169277, 169465, 170040    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch
none
patch none

Description Jake Archibald 2015-09-15 03:49:01 PDT
Test page: https://wiki-offline.jakearchibald.com/wiki/Hulk_Hogan?use-url-flags&prevent-sw=1&no-css-async-hack=1

Results:
Chrome http://www.webpagetest.org/video/compare.php?tests=150424_57_12PJ-r:1-c:0
IE http://www.webpagetest.org/video/compare.php?tests=150424_NH_12XE-r:1-c:0

In the test above, the page has inline CSS, 'defer'd javascript, then the HTML for the top toolbar. After the toolbar, there's a <link> for some CSS (load.php in the waterfall), followed by the article content.

In Chrome/Safari, the preparser discovers the CSS and blocks further rendering, including content before the <link>. IE will allow content before the <link> to render, and block afterwards. Firefox is similar to IE

The spec is unclear here, but developers seem to want this behaviour https://www.w3.org/Bugs/Public/show_bug.cgi?id=27303#c37, and there are hacks to work around the current block-everything behaviour https://github.com/filamentgroup/loadCSS

Chrome is looking to align with Firefox & IE on this https://code.google.com/p/chromium/issues/detail?id=481122#c17

Further explanation of the problem & benefits of this solution https://www.w3.org/Bugs/Public/show_bug.cgi?id=27303#c36
Comment 1 Jake Archibald 2015-09-15 03:54:19 PDT
I realise the videos in the previous comment don't show the effect in Safari specifically, unfortunately WPT doesn't support it, but testing with the Network Link Conditioner shows it to have the same issue as Chrome.
Comment 2 Simon Fraser (smfr) 2016-02-15 10:57:36 PST
Chrome is planning to ship this:
https://jakearchibald.com/2016/link-in-body/
Comment 3 Radar WebKit Bug Importer 2016-02-15 10:57:59 PST
<rdar://problem/24658830>
Comment 4 Antti Koivisto 2016-08-04 07:22:40 PDT
> In Chrome/Safari, the preparser discovers the CSS and blocks further
> rendering, including content before the <link>. 

This is not true in WebKit. Stylesheet loads triggered by preload scanner don't block rendering. Rendering is only blocked when the real parser reaches a loading <link> element (or <style> with @import).
Comment 5 Antti Koivisto 2017-03-08 12:28:42 PST
Created attachment 303826 [details]
patch
Comment 6 Antti Koivisto 2017-03-08 12:52:01 PST
*** Bug 169345 has been marked as a duplicate of this bug. ***
Comment 7 Simon Fraser (smfr) 2017-03-08 13:38:42 PST
Comment on attachment 303826 [details]
patch

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

> Source/WebCore/rendering/RenderBlock.cpp:1575
> +    // Style is non-final if the element has a pending stylesheet before it. We end up with renderers with such styles if a script
> +    // forces renderer construction by querying something layout dependent.

Is this comment accurate with <link rel="stylesheet"> in the body? Won't we force renderer construction anyway?

> Source/WebCore/style/StyleTreeResolver.cpp:171
> +        m_document.setHasNodesWithNonFinalStyle();

Wehre is this cleared? I see no calls to m_document.setHasNodesWithNonFinalStyle(false)

> Source/WebCore/style/StyleTreeResolver.cpp:184
> +        m_document.setHasNodesWithNonFinalStyle();

Ditto.
Comment 8 Antti Koivisto 2017-03-08 13:54:23 PST
> Is this comment accurate with <link rel="stylesheet"> in the body? Won't we
> force renderer construction anyway?

In normal case we only compute style (and so construct renderers) up to the loading in-body stylesheet. Call to updateLayoutIgnorePendingStylesheet() forces renderer construction for all elements. In that case we generate renderers with non-final style.

> Wehre is this cleared? I see no calls to
> m_document.setHasNodesWithNonFinalStyle(false)

Document::resolveStyle() has

m_hasNodesWithNonFinalStyle = false;
Comment 9 Antti Koivisto 2017-03-08 13:55:23 PST
> Document::resolveStyle() has
> 
> m_hasNodesWithNonFinalStyle = false;

...when we do full style rebuild.
Comment 10 Simon Fraser (smfr) 2017-03-08 13:57:37 PST
(In reply to comment #9)
> > Document::resolveStyle() has
> > 
> > m_hasNodesWithNonFinalStyle = false;
> 
> ...when we do full style rebuild.

It would make searching easier if this called the setHasNodesWithNonFinalStyle() function
Comment 11 Antti Koivisto 2017-03-09 00:15:30 PST
> It would make searching easier if this called the
> setHasNodesWithNonFinalStyle() function

I don't want to make setHasNodesWithNonFinalStyle take an argument since no one except Document itself should reset the flag.
Comment 12 WebKit Commit Bot 2017-03-09 01:07:12 PST
Comment on attachment 303826 [details]
patch

Clearing flags on attachment: 303826

Committed r213633: <http://trac.webkit.org/changeset/213633>
Comment 13 WebKit Commit Bot 2017-03-09 01:07:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Ryan Haddad 2017-03-09 21:11:43 PST
*** Bug 169345 has been marked as a duplicate of this bug. ***
Comment 15 Ryan Haddad 2017-03-09 21:12:32 PST
Reverted r213633 for reason:

This change caused LayoutTest imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute.html to become a flaky failure.

Committed r213701: <http://trac.webkit.org/changeset/213701>
Comment 16 Ryan Haddad 2017-03-09 21:13:33 PST
*** Bug 169452 has been marked as a duplicate of this bug. ***
Comment 17 Antti Koivisto 2017-03-10 07:28:50 PST
Fix for bug 169465 should make imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute.html reliable.
Comment 18 WebKit Commit Bot 2017-03-10 07:30:54 PST
Comment on attachment 303826 [details]
patch

Rejecting attachment 303826 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 303826, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3283697
Comment 19 Antti Koivisto 2017-03-10 07:33:07 PST
Created attachment 304042 [details]
patch
Comment 20 WebKit Commit Bot 2017-03-10 08:01:54 PST
Comment on attachment 304042 [details]
patch

Clearing flags on attachment: 304042

Committed r213712: <http://trac.webkit.org/changeset/213712>
Comment 21 WebKit Commit Bot 2017-03-10 08:02:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Antti Koivisto 2017-03-23 22:37:31 PDT
Reverted in http://trac.webkit.org/changeset/214333 due to iPad PLT regression.
Comment 23 Antti Koivisto 2017-03-27 14:30:10 PDT
Created attachment 305510 [details]
patch
Comment 24 WebKit Commit Bot 2017-03-27 15:56:36 PDT
Comment on attachment 305510 [details]
patch

Clearing flags on attachment: 305510

Committed r214435: <http://trac.webkit.org/changeset/214435>
Comment 25 WebKit Commit Bot 2017-03-27 15:56:40 PDT
All reviewed patches have been landed.  Closing bug.