Bug 208097 - Use DOM element iterators more, and more consistently
Summary: Use DOM element iterators more, and more consistently
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks: 207816
  Show dependency treegraph
 
Reported: 2020-02-22 17:03 PST by Darin Adler
Modified: 2020-02-24 16:48 PST (History)
29 users (show)

See Also:


Attachments
Patch (77.45 KB, patch)
2020-02-22 17:24 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (75.84 KB, patch)
2020-02-22 18:07 PST, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-02-22 17:03:53 PST
Use DOM element iterators more, and more consistently
Comment 1 Darin Adler 2020-02-22 17:24:03 PST Comment hidden (obsolete)
Comment 2 Darin Adler 2020-02-22 17:25:44 PST
Antti already reviewed this as part of the patch in bug 207816. The changes are in response to comments he made in that review, and of course this is also a subset of that patch as suggested.

Uploading for EWS analysis, and also another round of review would be OK although maybe not necessary.
Comment 3 Darin Adler 2020-02-22 18:07:28 PST
Created attachment 391472 [details]
Patch
Comment 4 Anders Carlsson 2020-02-22 19:17:05 PST
Comment on attachment 391472 [details]
Patch

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

> Source/WebCore/html/HTMLOptionElement.cpp:169
> +        for (auto& dataList : ancestorsOfType<HTMLDataListElement>(*this))

Is this a functionality change? Will this end up calling optionElementChildrenChanged on all the HTMLDataList ancestors now?
Comment 5 Darin Adler 2020-02-22 20:54:02 PST
Comment on attachment 391472 [details]
Patch

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

>> Source/WebCore/html/HTMLOptionElement.cpp:169
>> +        for (auto& dataList : ancestorsOfType<HTMLDataListElement>(*this))
> 
> Is this a functionality change? Will this end up calling optionElementChildrenChanged on all the HTMLDataList ancestors now?

Yes.

What the specification says is "each option element that is a descendant of the datalist element, that is not disabled, and whose value is a string that isn't the empty string, represents a suggestion". Nothing excluding elements that happen to be inside another datalist that is itself a descendant of the datalist element.

Thus even options inside a nested HTMLDataListElement should appear also as suggestions, when the datalist element is used, and also should appear in HTMLDataListElement.options because of a different sentence in the specification. Our code properly iterates all suggestions and doesn’t skip subtrees that themselves happen to be rooted in a nested datalist. But this change notification mechanism notified only a single enclosing HTMLDataListElement. This is insufficient.

This led me to a few thoughts:

1) It might be helpful if the specification made this explicit, perhaps with a non-normative example of proper behavior when there is a nested datalist.

2) I could, and probably should, construct a test to show a case where the old code did not properly dynamically update something when multiple datalist elements are nested inside one another.

3) I found and fixed this error in HTMLOptionElement::parseAttribute and HTMLOptionElement::childrenChanged, and preserved the behavior in WebAutomationSessionProxy, because there it does seem to be what is intended, and that code is about how WebDriver works, not how the actual datalist element works.
Comment 6 Darin Adler 2020-02-22 20:56:11 PST
Committed r257188: <https://trac.webkit.org/changeset/257188>
Comment 7 Radar WebKit Bug Importer 2020-02-22 20:57:18 PST
<rdar://problem/59703408>
Comment 8 Antti Koivisto 2020-02-23 02:19:27 PST
Comment on attachment 391472 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:3438
> +    // FIXME: Not clear it's valuable to do this to all body elements rather than just doing it on the single one returned by Document::body.
> +    Vector<Ref<HTMLBodyElement>> bodies;
> +    for (auto& body : descendantsOfType<HTMLBodyElement>(document()))
> +        bodies.append(body);

I suspect this could at least be limited to the children of the documentElement(), avoiding full document crawl.
Comment 9 Antti Koivisto 2020-02-23 02:23:12 PST
Comment on attachment 391472 [details]
Patch

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

>> Source/WebCore/editing/Editor.cpp:3438
>> +    // FIXME: Not clear it's valuable to do this to all body elements rather than just doing it on the single one returned by Document::body.
>> +    Vector<Ref<HTMLBodyElement>> bodies;
>> +    for (auto& body : descendantsOfType<HTMLBodyElement>(document()))
>> +        bodies.append(body);
> 
> I suspect this could at least be limited to the children of the documentElement(), avoiding full document crawl.

Note that there is a cache for getElementsByTagName so the new code is potentially much slower than the old.
Comment 10 Darin Adler 2020-02-23 14:43:18 PST
Comment on attachment 391472 [details]
Patch

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

>>> Source/WebCore/editing/Editor.cpp:3438
>>> +        bodies.append(body);
>> 
>> I suspect this could at least be limited to the children of the documentElement(), avoiding full document crawl.
> 
> Note that there is a cache for getElementsByTagName so the new code is potentially much slower than the old.

Both good points.

This is only called when the API editable property is set on a legacy WebView or when the SPI _editable property is set on a WKWebView. And also for legacy WebKit it’s called once for each newly loaded page after it is loaded in a WebView that has the editable property. We never implemented that second feature in WKWebView; I think that means in practice that clients don’t expect to be able to navigate to new pages and edit them using the editable property.

Iterating the entire document once is probably still affordable for those cases. I don’t think the caching was protecting us from the cost, before: unlikely anyone had called getElementsByTagName("body") by the time this is called, nor any reason to think that's a typical thing to do afterward.

But also, given how it’s used I think we could take the risk of changing this to only do the work on the actual body element. Or just the children of the document element as you suggested.
Comment 11 Antti Koivisto 2020-02-24 01:04:36 PST
Really, it is probably fine to limit this and similar things to body(). The parser doesn't generate additional <body> elements and there is no reason for anyone to construct them explicitly.
Comment 12 Darin Adler 2020-02-24 16:48:49 PST
(In reply to Antti Koivisto from comment #11)
> Really, it is probably fine to limit this and similar things to body(). The
> parser doesn't generate additional <body> elements and there is no reason
> for anyone to construct them explicitly.

I’ll do it.

Seems like there is a tiny risk that this breaks someone doing something crazy in an app, perhaps by accident.