Bug 64587 – Add support for retrieving an element in TreeScope by id to window.internals object.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hayato Ito]

It would be nice that we can get an element in nested TreeScope by specifying 'id' and 'owner element'. That enables us to write the following code in tests:

  function getElementInShadow(path)
  {
    var ids = path.split('/');
    var element = document.getElementById(ids[0]);
    for (var i = 1; i < ids.length; ++i) {
        element = window.internals.getElementByIdInShadowHost(element, ids[i]);
    }
    return element;
  }

  var inputA = getElementInShadow('shadowHost/shadowHost2/inputA');

[Hayato Ito]

I am thinking that we should have

  window.internals.getElementByIdInTreeScope(shadowRoot, ids[i])

instead of

  window.internals.getElementByIdInShadowHost(shadowHost, ids[i])
.

[Hayato Ito]

Created attachment 100950 [details]
first try to know which symbols we should export.

[Hayato Ito]

Created attachment 101273 [details]
sync to ToT

[Collabora GTK+ EWS bot]

Comment on attachment 101273 [details]
sync to ToT

Attachment 101273 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9158050

[Hayato Ito]

Created attachment 101289 [details]
2nd try. Add symbols

[Hayato Ito]

Created attachment 101297 [details]
ready for review

[Hajime Morrita]

Comment on attachment 101297 [details]
ready for review

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

> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:11
> +

Please make it clear that this test need internals object and doesn't work interactively.

> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:17
> +        element.setAttribute(name, attributes[name]);

Don't need braces if the block has single statement.
JS in WebKit basically follows our C++ style.

> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:60
> +

These helper functions look useful. How about to make it separate js file?

> Source/WebCore/testing/Internals.cpp:76
> +        ec = INVALID_ACCESS_ERR;

If this function is only for ShadowRoot, the name should be getElementByIdInTreeRoot().

> Source/WebCore/testing/Internals.cpp:79
> +    return static_cast<ShadowRoot*>(treeScope)->getElementById(id);

I guess we have toShadowRoot().

[Dominic Cooney]

Comment on attachment 101297 [details]
ready for review

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

>> Source/WebCore/testing/Internals.cpp:76
>> +        ec = INVALID_ACCESS_ERR;
> 
> If this function is only for ShadowRoot, the name should be getElementByIdInTreeRoot().

What about getElementByIdIn*Shadow*Root?

Tree root sounds like something else—the document, subtree root, whatever.

[Hayato Ito]

Comment on attachment 101297 [details]
ready for review

Thank you for the review. I'll post a updated patch later.

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

>> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:11
>> +
> 
> Please make it clear that this test need internals object and doesn't work interactively.

Done.

>> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:17
>> +        element.setAttribute(name, attributes[name]);
> 
> Don't need braces if the block has single statement.
> JS in WebKit basically follows our C++ style.

Done.

>> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:60
>> +
> 
> These helper functions look useful. How about to make it separate js file?

Done. I've moved createDom and createShadow functions into a separate js file as 'resources/create-dom.js'.
I might move getElemetnInShadow also when we have another layout test which uses this function.

>>> Source/WebCore/testing/Internals.cpp:76
>>> +        ec = INVALID_ACCESS_ERR;
>> 
>> If this function is only for ShadowRoot, the name should be getElementByIdInTreeRoot().
> 
> What about getElementByIdIn*Shadow*Root?
> 
> Tree root sounds like something else—the document, subtree root, whatever.

Okay. I chose 'getElementByIdInShadowRoot'.

>> Source/WebCore/testing/Internals.cpp:79
>> +    return static_cast<ShadowRoot*>(treeScope)->getElementById(id);
> 
> I guess we have toShadowRoot().

Oh. When I wrote this patch, there was no header files which defines toShadowRoot. Now we have that in ShadowRoot.h.
Done.

[Hayato Ito]

Created attachment 101440 [details]
revised

[Hayato Ito]

ping? Could someone review this?

[Hayato Ito]

Committed r92124: <http://trac.webkit.org/changeset/92124>