Bug 208097

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.

Created attachment 391472 [details] Patch

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 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.

Committed r257188: <https://trac.webkit.org/changeset/257188>

<rdar://problem/59703408>

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 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 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.

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.

(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.