New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a test for connectedCallback #11131

Merged
merged 1 commit into from May 29, 2018

Conversation

@rwlbuis
Copy link
Contributor

rwlbuis commented May 23, 2018

Add a test to verify connectedCallback is not called during HTML fragment parsing

Also see https://bugs.webkit.org/show_bug.cgi?id=183586

@rniwa
Copy link
Contributor

rniwa left a comment

r-

<div id="log"></div>
<script>
var calls = 0;

This comment has been minimized.

@rniwa

rniwa May 23, 2018

Contributor

Use let.

<script>
var calls = 0;
class Parenter extends HTMLElement {

This comment has been minimized.

@rniwa

rniwa May 23, 2018

Contributor

This class should be re-defined for each document type.
e.g. you should be declaring these classes inside promise_test itself.

document_types().forEach(function (entry) {
var documentName = entry.name;
var getDocument = entry.create;

This comment has been minimized.

@rniwa

rniwa May 23, 2018

Contributor

Use let.

promise_test(function () {
return getDocument().then(function (doc) {
calls = 0;
document.body.innerHTML = '<x-parenter><x-child></x-child></x-parenter>';

This comment has been minimized.

@rniwa

rniwa May 23, 2018

Contributor

We need to be testing this inside doc, not document, which is the top-level document we have.

@rwlbuis

This comment has been minimized.

Copy link
Contributor

rwlbuis commented May 24, 2018

I updated the test to try to address the review comments.
Note that the test passes in chromium, and passes with WebKit with my patch applied (not without it).

@rniwa
Copy link
Contributor

rniwa left a comment

Looks much better. Let's go through one more iteration before merging.

connectedCallback() { calls++; }
}
let parenter = 'x-parenter' + iteration;
let child = 'x-child' + iteration++;

This comment has been minimized.

@rniwa

rniwa May 25, 2018

Contributor

Please increment iteration before at the beginning of each test instead

This comment has been minimized.

@rwlbuis

rwlbuis May 25, 2018

Contributor

Done.

customElements.define(parenter, Parenter);
customElements.define(child, Child);
return getDocument().then(function (doc) {
document.documentElement.innerHTML = '<' + parenter + '><' + child + '></' + child + '></' + parenter + '>';

This comment has been minimized.

@rniwa

rniwa May 25, 2018

Contributor

Use template literal as in <${parenter}><${child}></${child}></${parenter}>.

This comment has been minimized.

@rwlbuis

rwlbuis May 25, 2018

Contributor

Done.

document_types().forEach(function (entry) {
let documentName = entry.name;
let getDocument = entry.create;

This comment has been minimized.

@rniwa

rniwa May 25, 2018

Contributor

These two variables are only used once. There is no need to define them as local variables.

This comment has been minimized.

@rwlbuis

rwlbuis May 25, 2018

Contributor

Fixed.

<!DOCTYPE html>
<html>
<head>
<title>Custom Elements: connectedCallback not called during HTML fragment parsing</title>

This comment has been minimized.

@rniwa

rniwa May 25, 2018

Contributor

This title and the assertion description is a bit misleading. connectedCallback is indeed called on elements parsed via fragment parsing. It's just that "synchronous custom elements flag" must be unset when creating an element during fragment parsing. Put it more pragmatically, "the HTML fragment parsing algorithm must not create a custom element synchronously" or more verbosely "the HTML fragment parsing algorithm must enqueue a custom element upgrade reaction instead of synchronously invoking its constructor"

This comment has been minimized.

@rwlbuis

rwlbuis May 25, 2018

Contributor

Fixed.

doc.documentElement.appendChild(document.documentElement.firstChild);
assert_equals(calls, 1);
});
}, 'Inserting a custom element into ' + documentName + ' using HTML fragment parsing must not call connectedCallback.');

This comment has been minimized.

@rniwa

rniwa May 25, 2018

Contributor

Again, this description should not be as misleading as it is right now.

This comment has been minimized.

@rwlbuis

rwlbuis May 25, 2018

Contributor

Fixed.

@rwlbuis

This comment has been minimized.

Copy link
Contributor

rwlbuis commented May 25, 2018

Thanks for the review, I think I addressed everything now.

<div id="log"></div>
<script>
var iteration = 0;

This comment has been minimized.

@rniwa

rniwa May 26, 2018

Contributor

Use let?

doc.documentElement.appendChild(document.documentElement.firstChild);
assert_equals(calls, 1);
});
}, 'Inserting a custom element into ' + documentName + ' using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor');

This comment has been minimized.

@rniwa

rniwa May 26, 2018

Contributor

Use template literal?

@rniwa

rniwa approved these changes May 26, 2018

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented May 26, 2018

Please consider making those changes before merging.

Add a test for connectedCallback
Add a test to verify connectedCallback is not called during HTML fragment parsing

@rwlbuis rwlbuis force-pushed the Igalia:webkit_bug_183586 branch from 6478faf to f86c408 May 29, 2018

@mrego mrego merged commit a4d78aa into web-platform-tests:master May 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rwlbuis rwlbuis deleted the Igalia:webkit_bug_183586 branch Jun 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment