Bug 188190 - XML Parsers should invoke reactions when creating/inserting new custom elements
Summary: XML Parsers should invoke reactions when creating/inserting new custom elements
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on:
Blocks: 154907
  Show dependency treegraph
 
Reported: 2018-07-31 00:40 PDT by Frédéric Wang (:fredw)
Modified: 2018-09-07 22:38 PDT (History)
7 users (show)

See Also:


Attachments
WIP Patch (10.18 KB, patch)
2018-07-31 00:44 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (13.46 KB, patch)
2018-07-31 04:21 PDT, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.72 MB, application/zip)
2018-07-31 05:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.17 MB, application/zip)
2018-07-31 05:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.65 MB, application/zip)
2018-07-31 06:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.49 MB, application/zip)
2018-07-31 06:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.45 MB, application/zip)
2018-07-31 08:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews201 for win-future (12.90 MB, application/zip)
2018-07-31 12:24 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2018-07-31 00:40:09 PDT
From https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token:

6.3 Push a new element queue onto the custom element reactions stack.
9.1 Let queue be the result of popping the current element queue from the custom element reactions stack. (This will be the same element queue as was pushed above.)
9.2 Invoke custom element reactions in queue.

This should also be done for the XML parser per https://html.spec.whatwg.org/multipage/xhtml.html#parsing-xhtml-documents
Comment 1 Frédéric Wang (:fredw) 2018-07-31 00:44:43 PDT
Created attachment 346146 [details]
WIP Patch
Comment 2 Build Bot 2018-07-31 00:46:58 PDT
Attachment 346146 [details] did not pass style-queue:


ERROR: Source/WebCore/html/parser/HTMLConstructionSite.h:107:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Frédéric Wang (:fredw) 2018-07-31 03:45:55 PDT
Comment 0 was during element creation (and I guess essentially for attributeChanged reaction).

The same thing should be done when inserting the node (and I guess essentially for connectedCallback reaction):

If it is possible to insert element at the adjusted insertion location, then:
    Push a new element queue onto the custom element reactions stack.
    Insert element at the adjusted insertion location.
    Pop the element queue from the custom element reactions stack, and invoke custom element reactions in that queue.

(https://html.spec.whatwg.org/#insert-a-foreign-element)
Comment 4 Frédéric Wang (:fredw) 2018-07-31 04:21:17 PDT
Created attachment 346152 [details]
WIP Patch
Comment 5 Build Bot 2018-07-31 05:30:39 PDT
Comment on attachment 346152 [details]
WIP Patch

Attachment 346152 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8709282

New failing tests:
fast/css/insert-rule-overflow-rule-data.html
fast/text/large-text-composed-char.html
imported/blink/fast/text/wide-preformatted.html
js/function-apply-many-args.html
http/tests/misc/webtiming-slow-load.php
fast/css/css-selector-deeply-nested.html
Comment 6 Build Bot 2018-07-31 05:30:41 PDT
Created attachment 346155 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 7 Build Bot 2018-07-31 05:37:10 PDT
Comment on attachment 346152 [details]
WIP Patch

Attachment 346152 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8709296

New failing tests:
fast/css/insert-rule-overflow-rule-data.html
fast/text/large-text-composed-char.html
imported/blink/fast/text/wide-preformatted.html
js/function-apply-many-args.html
http/tests/misc/webtiming-slow-load.php
fast/css/css-selector-deeply-nested.html
Comment 8 Build Bot 2018-07-31 05:37:12 PDT
Created attachment 346156 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 9 Build Bot 2018-07-31 06:18:14 PDT
Comment on attachment 346152 [details]
WIP Patch

Attachment 346152 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8709346

New failing tests:
fast/css/insert-rule-overflow-rule-data.html
js/function-apply-many-args.html
platform/ios/ios/storage/domstorage/5mb-quota.html
imported/blink/fast/text/wide-preformatted.html
http/tests/misc/webtiming-slow-load.php
fast/css/css-selector-deeply-nested.html
Comment 10 Build Bot 2018-07-31 06:18:15 PDT
Created attachment 346161 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 11 Build Bot 2018-07-31 06:25:52 PDT
Comment on attachment 346152 [details]
WIP Patch

Attachment 346152 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8709463

New failing tests:
fast/css/insert-rule-overflow-rule-data.html
fast/text/large-text-composed-char.html
imported/blink/fast/text/wide-preformatted.html
js/function-apply-many-args.html
http/tests/misc/webtiming-slow-load.php
fast/css/css-selector-deeply-nested.html
Comment 12 Build Bot 2018-07-31 06:25:54 PDT
Created attachment 346163 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 13 Build Bot 2018-07-31 08:05:19 PDT
Comment on attachment 346152 [details]
WIP Patch

Attachment 346152 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8710139

New failing tests:
fast/css/insert-rule-overflow-rule-data.html
fast/text/large-text-composed-char.html
imported/blink/fast/text/wide-preformatted.html
js/function-apply-many-args.html
http/tests/misc/webtiming-slow-load.php
fast/css/css-selector-deeply-nested.html
Comment 14 Build Bot 2018-07-31 08:05:21 PDT
Created attachment 346166 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 15 Build Bot 2018-07-31 12:24:01 PDT
Comment on attachment 346152 [details]
WIP Patch

Attachment 346152 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8712411

New failing tests:
js/function-apply-many-args.html
fast/css/css-selector-deeply-nested.html
fast/css/insert-rule-overflow-rule-data.html
imported/blink/fast/text/wide-preformatted.html
Comment 16 Build Bot 2018-07-31 12:24:13 PDT
Created attachment 346189 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 17 Radar WebKit Bug Importer 2018-08-01 22:41:56 PDT
<rdar://problem/42843008>
Comment 18 Ryosuke Niwa 2018-08-05 14:12:43 PDT
Comment on attachment 346152 [details]
WIP Patch

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

In general, implementing things exactly the way spec is not warranted especially in the code that affects perf.
We just need to match the end behavior.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:128
> +    if (UNLIKELY(task.mayRunConnectedCallback))
> +        customElementReactionStack.emplace();
>      insert(task);
> +    customElementReactionStack.reset();

Adding this much code in this hot function is probably not acceptable in terms of perf.
Comment 19 Ryosuke Niwa 2018-08-05 18:39:34 PDT
Comment on attachment 346152 [details]
WIP Patch

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

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:226
> +        if (willExecuteScript) {

This check is unnecessary. If we're making a fallback element, then creating a custom element reaction stack has no observable effect.
Comment 20 Ryosuke Niwa 2018-08-05 19:09:12 PDT
I posted a patch on https://bugs.webkit.org/show_bug.cgi?id=188336 to solve some of the issues this patch tries to address.