Use the tree of trees for the lang attributes in Shadow DOM

Use the tree of trees for the lang attributes in Shadow DOM

To accommodate the spec change, computing the lang attribute was changed
to traverse tree of trees.

The relevant W3C bug is 27222
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27222

BUG=450115
TEST=fast/dom/shadow/attr-lang-inherit.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189903

[kojii, 2015-01-28 07:45:42]

kojii@chromium.org changed reviewers:
+ hayato@chromium.org

[kojii, 2015-01-28 07:45:43]

hayato@, PTAL!

[hayato, 2015-01-28 09:56:12]

Nit: The first line of CL should start with 'verb' in general.

https://codereview.chromium.org/878743003/diff/1/LayoutTests/fast/dom/shadow/...
File LayoutTests/fast/dom/shadow/attr-lang-inherit.html (right):

https://codereview.chromium.org/878743003/diff/1/LayoutTests/fast/dom/shadow/...
LayoutTests/fast/dom/shadow/attr-lang-inherit.html:24: [[function (root) {
return root.lastChild.previousSibling; }, fr],
You can use shadowRoot.querySelector(..) to get the node.

https://codereview.chromium.org/878743003/diff/1/LayoutTests/fast/dom/shadow/...
LayoutTests/fast/dom/shadow/attr-lang-inherit.html:27: testLangInShadow("lang of
distributed content traverses tree of trees recursively",
"Recursively" is misleading because this test only checks the case of one-level
nesting.

Since this is the first patch for 'attribute inheritance', I'd like to have more
test coverage.
Could you add more tests?

e.g.
- Multiple shadow roots
- The usage of <shadow>
- Disconnected node with shadow trees.

I'm not confident how much we should cover, however, the current test coverage
looks too small.

https://codereview.chromium.org/878743003/diff/1/LayoutTests/fast/dom/shadow/...
LayoutTests/fast/dom/shadow/attr-lang-inherit.html:39: if (typeof(element) ===
"function")
In this test, elementAndExpected[0] is always a function, isn't it?

[kojii, 2015-01-29 11:05:24]

hayato@, PTAL!

https://codereview.chromium.org/878743003/diff/1/LayoutTests/fast/dom/shadow/...
File LayoutTests/fast/dom/shadow/attr-lang-inherit.html (right):

https://codereview.chromium.org/878743003/diff/1/LayoutTests/fast/dom/shadow/...
LayoutTests/fast/dom/shadow/attr-lang-inherit.html:24: [[function (root) {
return root.lastChild.previousSibling; }, fr],
On 2015/01/28 09:56:12, hayato wrote:
> You can use shadowRoot.querySelector(..) to get the node.

Done.

https://codereview.chromium.org/878743003/diff/1/LayoutTests/fast/dom/shadow/...
LayoutTests/fast/dom/shadow/attr-lang-inherit.html:27: testLangInShadow("lang of
distributed content traverses tree of trees recursively",
On 2015/01/28 09:56:12, hayato wrote:
> "Recursively" is misleading because this test only checks the case of
one-level
> nesting.
> 
> Since this is the first patch for 'attribute inheritance', I'd like to have
more
> test coverage.
> Could you add more tests?
> 
> e.g.
> - Multiple shadow roots
> - The usage of <shadow>
> - Disconnected node with shadow trees.
> 
> I'm not confident how much we should cover, however, the current test coverage
> looks too small.

Added:
- Multiple shadow roots to one host
- Multiple levels (shadow root to a host in a shadow tree)
- Content in olderShadowRoot distributed to a shadow insertion point
- Content in the document distributed to olderShadowRoot and then re-distributed
to youngerShadowroot

Could not test nodes that are not in document because this test relies on
getComputedStyle() which works only when the element is in document.

https://codereview.chromium.org/878743003/diff/1/LayoutTests/fast/dom/shadow/...
LayoutTests/fast/dom/shadow/attr-lang-inherit.html:39: if (typeof(element) ===
"function")
On 2015/01/28 09:56:12, hayato wrote:
> In this test, elementAndExpected[0] is always a function, isn't it?

Done.

[hayato, 2015-02-03 02:22:27]

https://codereview.chromium.org/878743003/diff/20001/LayoutTests/fast/dom/sha...
File LayoutTests/fast/dom/shadow/attr-lang-inherit.html (right):

https://codereview.chromium.org/878743003/diff/20001/LayoutTests/fast/dom/sha...
LayoutTests/fast/dom/shadow/attr-lang-inherit.html:80: var element =
elementAndExpected[0](root);
I think that calling function with a shadow root here doesn't contribute to
readability of the tests. Rather, it would make it difficult to understand the
tests.

Since we can access the youngest shadow root via host.shadowRoot, I think we
don't have to calling functions with a root here. Could you make it simpler?

https://codereview.chromium.org/878743003/diff/20001/LayoutTests/fast/dom/sha...
LayoutTests/fast/dom/shadow/attr-lang-inherit.html:87: function
createShadowRoot(host, shadowHtml) {
Can we have a better name for this function?
It'd be better to have a different name than (Element.)createShadowRoot.

[hayato, 2015-02-03 02:23:59]

On 2015/01/29 11:05:24, koji wrote:
> hayato@, PTAL!
> 
>
https://codereview.chromium.org/878743003/diff/1/LayoutTests/fast/dom/shadow/...
> File LayoutTests/fast/dom/shadow/attr-lang-inherit.html (right):
> 
>
https://codereview.chromium.org/878743003/diff/1/LayoutTests/fast/dom/shadow/...
> LayoutTests/fast/dom/shadow/attr-lang-inherit.html:24: [[function (root) {
> return root.lastChild.previousSibling; }, fr],
> On 2015/01/28 09:56:12, hayato wrote:
> > You can use shadowRoot.querySelector(..) to get the node.
> 
> Done.
> 
>
https://codereview.chromium.org/878743003/diff/1/LayoutTests/fast/dom/shadow/...
> LayoutTests/fast/dom/shadow/attr-lang-inherit.html:27: testLangInShadow("lang
of
> distributed content traverses tree of trees recursively",
> On 2015/01/28 09:56:12, hayato wrote:
> > "Recursively" is misleading because this test only checks the case of
> one-level
> > nesting.
> > 
> > Since this is the first patch for 'attribute inheritance', I'd like to have
> more
> > test coverage.
> > Could you add more tests?
> > 
> > e.g.
> > - Multiple shadow roots
> > - The usage of <shadow>
> > - Disconnected node with shadow trees.
> > 
> > I'm not confident how much we should cover, however, the current test
coverage
> > looks too small.
> 
> Added:
> - Multiple shadow roots to one host
> - Multiple levels (shadow root to a host in a shadow tree)
> - Content in olderShadowRoot distributed to a shadow insertion point
> - Content in the document distributed to olderShadowRoot and then
re-distributed
> to youngerShadowroot
> 
> Could not test nodes that are not in document because this test relies on
> getComputedStyle() which works only when the element is in document.

I'm not aware of this.
Is there an alternative way to test the attribute inheritance without using
getComputedStyle()?

[kojii, 2015-02-03 07:36:01]

On 2015/02/03 02:23:59, hayato wrote:
> On 2015/01/29 11:05:24, koji wrote:
> > 
> > Could not test nodes that are not in document because this test relies on
> > getComputedStyle() which works only when the element is in document.
> 
> I'm not aware of this.
> Is there an alternative way to test the attribute inheritance without using
> getComputedStyle()?

We could add a method to Internals. Doing so allows more direct test; i.e.,
attribute inheritance occurs in the node tree, but since the computed value of
these attributes are internal, I'm using :lang() selector to test, but it's
testing computed lang after going through the style engine.

Shall I go the Internals direction? Downsides I can think of are
1) such test is only testable by test runner (not interactive content_shell nor
in chrome)
2) the test will be Blink-only (not upstreamable.)

The title attribute is a bit more complex, because its computed value is only
available through HitTestResult. I haven't come up with an idea to test "not in
document" case for the title attribute.

But we could take this approach for lang and dir. Do you think we should?

[hayato, 2015-02-03 22:14:30]

If there is no way to test an 'inherited attribute' from JS, we have to use
Internals. However, using Internals should be the last hope, as you know the
downsides.

Is my understanding correct?

- I think window.getComputedStyle() should work for an element even in shadow
trees. There are a lot of layout tests which uses getComputedStyle for an
element in shadow trees.
- We can't use getComputedStyle() to test an 'inherited attribute'.

If you can produce any *visible* change by an inherited attribute, that's a
preferred way. However, if that's difficult, you don't have to hesitate to use
Internals.

[kojii, 2015-02-04 02:04:33]

On 2015/02/03 22:14:30, hayato wrote:
> If there is no way to test an 'inherited attribute' from JS, we have to use
> Internals. However, using Internals should be the last hope, as you know the
> downsides.
> 
> Is my understanding correct?
> 
> - I think window.getComputedStyle() should work for an element even in shadow
> trees. There are a lot of layout tests which uses getComputedStyle for an
> element in shadow trees.
> - We can't use getComputedStyle() to test an 'inherited attribute'.
> 
> If you can produce any *visible* change by an inherited attribute, that's a
> preferred way. However, if that's difficult, you don't have to hesitate to use
> Internals.

Sorry, I confused you, let me recap.

Among the suggested scenarios: > e.g.
> - Multiple shadow roots
> - The usage of <shadow>
> - Disconnected node with shadow trees.

Only the last one, "disconnected note with shadow trees" cannot use
getComputedStyle(). Other cases are fine.

So, options are:
A. Give up testing of disconnected nodes ONLY
B. Testing disconnected nodes is critical, add a method to Internals just for it
C. Testing disconnected nodes is critical, but think more and come up with other
methods than Internals

and the choice does not affect other cases that are already working in the
current patch:
- Simple case
- Distribution
- Multiple shadow roots to one host
- Multiple levels (shadow root to a host in a shadow tree)
- Content in olderShadowRoot distributed to a shadow insertion point
- Content in the document distributed to olderShadowRoot and then re-distributed
to youngerShadowroot

I'm leaning option A. WDYT?

Note, a minor correction. getComputedStyle() on disconnected nodes does not
throw, probably something else was wrong when I tried before. But it does not
compute inheritance, and can't use selectors. See this example:
http://jsbin.com/kibode/2/edit?html,js,output

[kojii, 2015-02-04 02:10:05]

Comments fixed. Please let me know WDYT on testing disconnected nodes in my
previous reply.

https://codereview.chromium.org/878743003/diff/20001/LayoutTests/fast/dom/sha...
File LayoutTests/fast/dom/shadow/attr-lang-inherit.html (right):

https://codereview.chromium.org/878743003/diff/20001/LayoutTests/fast/dom/sha...
LayoutTests/fast/dom/shadow/attr-lang-inherit.html:80: var element =
elementAndExpected[0](root);
On 2015/02/03 02:22:27, hayato wrote:
> I think that calling function with a shadow root here doesn't contribute to
> readability of the tests. Rather, it would make it difficult to understand the
> tests.
> 
> Since we can access the youngest shadow root via host.shadowRoot, I think we
> don't have to calling functions with a root here. Could you make it simpler?
> 

Done.

https://codereview.chromium.org/878743003/diff/20001/LayoutTests/fast/dom/sha...
LayoutTests/fast/dom/shadow/attr-lang-inherit.html:87: function
createShadowRoot(host, shadowHtml) {
On 2015/02/03 02:22:27, hayato wrote:
> Can we have a better name for this function?
> It'd be better to have a different name than (Element.)createShadowRoot.

Done.

[hayato, 2015-02-10 10:16:17]

lgtm

[kojii, 2015-02-10 15:41:34]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2015-02-10 15:41:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/878743003/40001

[commit-bot: I haz the power, 2015-02-10 15:45:20]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189903