[webkit-reviews] review granted: [Bug 194764] Some refinements for Node and Document : [Attachment 362295] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 194764: Some refinements for Node and Document
https://bugs.webkit.org/show_bug.cgi?id=194764

Attachment 362295: Patch

https://bugs.webkit.org/attachment.cgi?id=362295&action=review




--- Comment #17 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 362295
  --> https://bugs.webkit.org/attachment.cgi?id=362295
Patch

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

> Source/WebCore/dom/ContainerNode.cpp:748
> +    switch (change.type) {

Maybe we can add a helper like affectsElements() to ChildChange?

> Source/WebCore/dom/Document.cpp:749
> +	   for (auto& node : composedTreeDescendants(*this)) {
> +	       if (!is<Element>(node))

This will make access key work in shadow trees.
We probably want to add a test for that.

e.g. add an element with accesskey inside a shadow tree, then emulate it via
eventSender.
The element should be focused and such.

> Source/WebCore/dom/Element.cpp:1560
> +	   else if (name == classAttr)

Weird that this one doesn't have HTMLNames qualification...

> Source/WebCore/dom/Node.cpp:1013
> +    // FIXME: Why is this implementation so different from
containsIncludingShadowDOM
> +    // function, given that the two functions' purposes are nearly
identical?
> +    return other && (isDescendantOf(*other) ||
other->contains(shadowHost()));

This function is clearly broken.
This element's shadow tree's host could be inside another shadow tree.
And this function doesn't take care of that.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:172
> -	   && !(shadowAncestorNode && shadowAncestorNode->renderer() &&
shadowAncestorNode->renderer()->isTextControl())
> +	   && !(shadowHost && shadowHost->renderer() &&
shadowHost->renderer()->isTextControl())

We should probably just use
enclosingTextFormControl(firstPositionInNode(shadowHost)).

> Source/WebCore/html/HTMLAreaElement.cpp:72
> -    } else if (name == altAttr || name == accesskeyAttr) {
> +    } else if (name == altAttr) {

Hm.. I wonder if we can write a test for this? Is there any behavior difference
here??

> Source/WebCore/html/HTMLSelectElement.cpp:-310
> -    else if (name == accesskeyAttr) {

Ditto. I feel like this might be artifact of WebKit not making these elements
focusable somehow...
If we had accesskey, we need to make the element focusable.

> Source/WebCore/html/HTMLTextAreaElement.cpp:-197
> -    } else if (name == accesskeyAttr) {
> -	   // ignore for the moment

Ditto. We should probably add a test for these three elements.

> Source/WebCore/loader/DocumentWriter.cpp:193
> -TextResourceDecoder* DocumentWriter::createDecoderIfNeeded()
> +TextResourceDecoder& DocumentWriter::decoder()

ensureEncoder per our naming convention?

> Source/WebCore/page/DragController.cpp:353
> +static bool hasEnabledColorInputAsShadowHost(Node& node)

I'd call this isInShadowTreeOfEnabledColorInput instead.