Skip to content

Commit

Permalink
[renderblocking] Don't cancel implicit render-blocking when blocking …
Browse files Browse the repository at this point in the history
…attribute is removed

1. This patch adds tests for whatwg/html#7857.
   When `blocking=render` is removed, if the element is implicitly
   render-blocking, we shouldn't unblock rendering for it.

2. This patch fixes how <link> element respond to `blocking` attribute
   changes. Previously, the element simply reprocesses (cancel and then
   restart) on every `blocking` attribute change, causing unnecessary
   work and also some bugs on stylesheets. This patch changes that into
   unblock rendering if needed to match the spec.

Bug: 1271296
Change-Id: I54a2f33dc1ed9971eaeceeb677a165d598d5d4d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3608747
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#998568}
NOKEYCHECK=True
GitOrigin-RevId: 831456d6a2d55d251875cdf15de4066e66d817e9
  • Loading branch information
xiaochengh authored and copybara-github committed May 2, 2022
1 parent d6620ac commit 2b86e00
Show file tree
Hide file tree
Showing 10 changed files with 134 additions and 1 deletion.
16 changes: 15 additions & 1 deletion blink/renderer/core/html/html_link_element.cc
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,12 @@ void HTMLLinkElement::ParseAttribute(
RuntimeEnabledFeatures::BlockingAttributeEnabled()) {
blocking_attribute_->DidUpdateAttributeValue(params.old_value, value);
blocking_attribute_->CountTokenUsage();
Process();
if (!IsRenderBlocking()) {
if (GetLinkStyle() && GetLinkStyle()->StyleSheetIsLoading())
GetLinkStyle()->UnblockRenderingForPendingSheet();
if (link_loader_)
link_loader_->UnblockRenderingForPendingLinkPreload();
}
} else if (name == html_names::kHrefAttr) {
// Log href attribute before logging resource fetching in process().
LogUpdateAttributeIfIsolatedWorldAndInDocument("link", params);
Expand Down Expand Up @@ -331,6 +336,15 @@ void HTMLLinkElement::SetToPendingState() {
GetLinkStyle()->SetToPendingState();
}

bool HTMLLinkElement::IsImplicitlyRenderBlocking() const {
return IsCreatedByParser() && rel_attribute_.IsStyleSheet();
}

bool HTMLLinkElement::IsRenderBlocking() const {
return blocking_attribute_->IsExplicitlyRenderBlocking() ||
IsImplicitlyRenderBlocking();
}

bool HTMLLinkElement::IsURLAttribute(const Attribute& attribute) const {
return attribute.GetName().LocalName() == html_names::kHrefAttr ||
HTMLElement::IsURLAttribute(attribute);
Expand Down
5 changes: 5 additions & 0 deletions blink/renderer/core/html/html_link_element.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,11 @@ class CORE_EXPORT HTMLLinkElement final : public HTMLElement,
void LinkLoaded() override;
void LinkLoadingErrored() override;

// https://html.spec.whatwg.org/C/#implicitly-render-blocking
bool IsImplicitlyRenderBlocking() const;
// https://html.spec.whatwg.org/C/#render-blocking
bool IsRenderBlocking() const;

Member<LinkResource> link_;
Member<LinkLoader> link_loader_;

Expand Down
9 changes: 9 additions & 0 deletions blink/renderer/core/html/link_style.cc
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,15 @@ void LinkStyle::OwnerRemoved() {
ClearSheet();
}

void LinkStyle::UnblockRenderingForPendingSheet() {
DCHECK(StyleSheetIsLoading());
if (pending_sheet_type_ == PendingSheetType::kDynamicRenderBlocking) {
GetDocument().GetStyleEngine().RemovePendingBlockingSheet(
*owner_, pending_sheet_type_);
pending_sheet_type_ = PendingSheetType::kNonBlocking;
}
}

void LinkStyle::Trace(Visitor* visitor) const {
visitor->Trace(sheet_);
LinkResource::Trace(visitor);
Expand Down
2 changes: 2 additions & 0 deletions blink/renderer/core/html/link_style.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ class LinkStyle final : public LinkResource, ResourceClient {

CSSStyleSheet* Sheet() const { return sheet_.Get(); }

void UnblockRenderingForPendingSheet();

private:
// From ResourceClient
void NotifyFinished(Resource*) override;
Expand Down
5 changes: 5 additions & 0 deletions blink/renderer/core/loader/link_loader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,11 @@ void LinkLoader::Abort() {
}
}

void LinkLoader::UnblockRenderingForPendingLinkPreload() {
if (pending_preload_)
pending_preload_->UnblockRendering();
}

void LinkLoader::Trace(Visitor* visitor) const {
visitor->Trace(client_);
visitor->Trace(pending_preload_);
Expand Down
2 changes: 2 additions & 0 deletions blink/renderer/core/loader/link_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ class CORE_EXPORT LinkLoader final : public GarbageCollected<LinkLoader> {
void NotifyModuleLoadFinished(ModuleScript*);
void NotifyFinished(Resource*);

void UnblockRenderingForPendingLinkPreload();

void Trace(Visitor*) const;

private:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<!DOCTYPE html>
<title>Synchronous script element still blocks rendering after removing `blocking=render`</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="support/test-render-blocking.js"></script>

<script>
// Test script must be added before the synchronous script because the
// synchronous script is parser-blocking.

promise_setup(async () => {
let script = await nodeInserted(document.head, node => node.id === 'script');
script.blocking = '';

// Also inserts some contents for non-compliant UA to render
document.body = document.createElement('body');
document.body.appendChild(document.createTextNode('Some text'));
});

test_render_blocking(
() => assert_equals(window.dummy, 1),
'Render-blocking script is loaded and evaluated');
</script>

<script id="script" blocking="render" src="support/dummy-1.js?pipe=trickle(d1)"></script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<!DOCTYPE html>
<title>Parser-inserted style element still blocks rendering after removing `blocking=render`</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="support/test-render-blocking.js"></script>

<script>
// Test script must be added before the style element because the style
// element is script-blocking.

promise_setup(async () => {
let sheet = await nodeInserted(document.head, node => node.id === 'sheet');
sheet.blocking = '';
});

test_render_blocking(
() => {
let color = getComputedStyle(document.querySelector('.target')).color;
assert_equals(color, 'rgb(255, 0, 0)');
},
'Render-blocking stylesheet is applied');
</script>

<style id="sheet" blocking="render">
@import url("support/target-red.css?pipe=trickle(d1)");
</style>

<div class="target">Some text</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<!DOCTYPE html>
<title>Parser-inserted stylesheet link still blocks rendering after removing `blocking=render`</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="support/test-render-blocking.js"></script>

<script>
// Test script must be added before the stylesheet link because the stylesheet
// link is script-blocking.

promise_setup(async () => {
let sheet = await nodeInserted(document.head, node => node.id === 'sheet');
sheet.blocking = '';
});

test_render_blocking(
() => {
let color = getComputedStyle(document.querySelector('.target')).color;
assert_equals(color, 'rgb(255, 0, 0)');
},
'Render-blocking stylesheet is applied');
</script>

<link id="sheet" rel="stylesheet" blocking="render"
href="support/target-red.css?pipe=trickle(d1)">

<div class="target">Some text</div>
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,22 @@ class LoadObserver {
}
}

// Observes the insertion of a script/parser-blocking element into DOM via
// MutationObserver, so that we can access the element before it's loaded.
function nodeInserted(parentNode, predicate) {
return new Promise(resolve => {
function callback(mutationList) {
for (let mutation of mutationList) {
for (let node of mutation.addedNodes) {
if (predicate(node))
resolve(node);
}
}
}
new MutationObserver(callback).observe(parentNode, {childList: true});
});
}

function createAutofocusTarget() {
const autofocusTarget = document.createElement('textarea');
autofocusTarget.setAttribute('autofocus', '');
Expand Down

0 comments on commit 2b86e00

Please sign in to comment.