Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
859943002:
Use the composed tree for the title attributes in Shadow DOM (Closed)
|
Created:
5 years, 11 months ago by kojii Modified:
5 years, 10 months ago Reviewers:
hayato CC:
blink-reviews, pdr+renderingwatchlist_chromium.org, zoltan1, sof, eae+blinkwatch, leviw+renderwatch, Dominik Röttsches, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews-rendering, jchaffraix+rendering, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionUse the composed tree for the title attributes in Shadow DOM
To accommodate the spec change, compute title for tooltip
was changed to traverse the composed tree.
The relevant W3C bug is 27222
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27222
BUG=450115, 420772
TEST=fast/dom/shadow/attr-title-inherit.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190138
Patch Set 1 #
Total comments: 6
Patch Set 2 : tests fixed #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Trimmed to lang only, split others to other CLs #Patch Set 7 : #Patch Set 8 : Update tests to reflect comments #Patch Set 9 : rebase-update after directory rename (rendering->layout) #
Total comments: 4
Patch Set 10 : Fix tests #
Messages
Total messages: 19 (4 generated)
kojii@chromium.org changed reviewers: + hayato@chromium.org
PTAL!
https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... File LayoutTests/fast/dom/shadow/lang-attribute-inherit.html (right): https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... LayoutTests/fast/dom/shadow/lang-attribute-inherit.html:14: assert_equals(japanese.offsetHeight, 0); Could you also assert "<div lang='fr'>Should be visible</div>" is visible here as well as root.lastChild? https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... File LayoutTests/fast/dom/shadow/spellcheck-attribute-inherit.html (right): https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... LayoutTests/fast/dom/shadow/spellcheck-attribute-inherit.html:20: test(function () { I guess this is checking whether 'spelling' is enabled or not for the editable element. Could you extract this check logic as a separate function and name it explicitly? e.g. function isSpellingEnabled(element) { ... } That would make the intention more clear. https://codereview.chromium.org/859943002/diff/1/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/859943002/diff/1/Source/core/dom/Element.cpp#... Source/core/dom/Element.cpp:2582: n = ComposedTreeTraversal::parent(*n); One tough issue around here is that ComposedTreeTraversal::parent() (and similar functions in ComposedTreeTraversal) requires that distribution is already calculated. Unless that, it hits ASSERTION. We must guarantee it by calling Document::updateDistributionIfNeeded(). However, calling it should be *minimized* because it should be considered non-trivial tasks. That has been always our headache since it's non-trivial for us to judge whether distribution is dirty or not at each place where we need ComposedTreeTraversal. Could you be aware of that and make sure distribution isn't dirty at each place? I guess it's hard task in general. I'm happy to help that in offline if you prefer.
Tests were fixed. For updateDistributionIfNeeded(), please see below inline. https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... File LayoutTests/fast/dom/shadow/lang-attribute-inherit.html (right): https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... LayoutTests/fast/dom/shadow/lang-attribute-inherit.html:14: assert_equals(japanese.offsetHeight, 0); On 2015/01/21 09:04:50, hayato wrote: > Could you also assert "<div lang='fr'>Should be visible</div>" is visible here > as well as root.lastChild? Done. https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... File LayoutTests/fast/dom/shadow/spellcheck-attribute-inherit.html (right): https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... LayoutTests/fast/dom/shadow/spellcheck-attribute-inherit.html:20: test(function () { On 2015/01/21 09:04:50, hayato wrote: > I guess this is checking whether 'spelling' is enabled or not for the editable > element. > Could you extract this check logic as a separate function and name it > explicitly? > > e.g. > function isSpellingEnabled(element) { > ... > } > > That would make the intention more clear. Done. https://codereview.chromium.org/859943002/diff/1/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/859943002/diff/1/Source/core/dom/Element.cpp#... Source/core/dom/Element.cpp:2582: n = ComposedTreeTraversal::parent(*n); On 2015/01/21 09:04:50, hayato wrote: > One tough issue around here is that ComposedTreeTraversal::parent() (and similar > functions in ComposedTreeTraversal) requires that distribution is already > calculated. Unless that, it hits ASSERTION. > > We must guarantee it by calling Document::updateDistributionIfNeeded(). > However, calling it should be *minimized* because it should be considered > non-trivial tasks. > > That has been always our headache since it's non-trivial for us to judge whether > distribution is dirty or not at each place where we need ComposedTreeTraversal. > > Could you be aware of that and make sure distribution isn't dirty at each place? > I guess it's hard task in general. I'm happy to help that in offline if you > prefer. Understood, thank you for this. I originally tried |NodeRenderingTraversal::parent()|, which has |ASSERT(!node.document().childNeedsDistributionRecalc())|. The patch passed LayoutTests, but browser_tests hit the AF. I guessed it's because browser_tests does not run full document life cycles, and LayoutTests didn't hit AF means that the recalc was done. Could you mind to tell me how I should interpret this result?
On 2015/01/21 16:46:20, koji wrote: > Tests were fixed. For updateDistributionIfNeeded(), please see below inline. > > https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... > File LayoutTests/fast/dom/shadow/lang-attribute-inherit.html (right): > > https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... > LayoutTests/fast/dom/shadow/lang-attribute-inherit.html:14: > assert_equals(japanese.offsetHeight, 0); > On 2015/01/21 09:04:50, hayato wrote: > > Could you also assert "<div lang='fr'>Should be visible</div>" is visible here > > as well as root.lastChild? > > Done. > > https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... > File LayoutTests/fast/dom/shadow/spellcheck-attribute-inherit.html (right): > > https://codereview.chromium.org/859943002/diff/1/LayoutTests/fast/dom/shadow/... > LayoutTests/fast/dom/shadow/spellcheck-attribute-inherit.html:20: test(function > () { > On 2015/01/21 09:04:50, hayato wrote: > > I guess this is checking whether 'spelling' is enabled or not for the editable > > element. > > Could you extract this check logic as a separate function and name it > > explicitly? > > > > e.g. > > function isSpellingEnabled(element) { > > ... > > } > > > > That would make the intention more clear. > > Done. > > https://codereview.chromium.org/859943002/diff/1/Source/core/dom/Element.cpp > File Source/core/dom/Element.cpp (right): > > https://codereview.chromium.org/859943002/diff/1/Source/core/dom/Element.cpp#... > Source/core/dom/Element.cpp:2582: n = ComposedTreeTraversal::parent(*n); > On 2015/01/21 09:04:50, hayato wrote: > > One tough issue around here is that ComposedTreeTraversal::parent() (and > similar > > functions in ComposedTreeTraversal) requires that distribution is already > > calculated. Unless that, it hits ASSERTION. > > > > We must guarantee it by calling Document::updateDistributionIfNeeded(). > > However, calling it should be *minimized* because it should be considered > > non-trivial tasks. > > > > That has been always our headache since it's non-trivial for us to judge > whether > > distribution is dirty or not at each place where we need > ComposedTreeTraversal. > > > > Could you be aware of that and make sure distribution isn't dirty at each > place? > > I guess it's hard task in general. I'm happy to help that in offline if you > > prefer. > > Understood, thank you for this. > > I originally tried |NodeRenderingTraversal::parent()|, which has > |ASSERT(!node.document().childNeedsDistributionRecalc())|. The patch passed > LayoutTests, but browser_tests hit the AF. I guessed it's because browser_tests > does not run full document life cycles, and LayoutTests didn't hit AF means that > the recalc was done. > > Could you mind to tell me how I should interpret this result? Could you elaborate what browser_test failed and what layout tests succeeded? Are they the same test conceptually?
hayato@, PTAL!
On 2015/01/27 08:03:55, koji wrote: > hayato@, PTAL! Could you separate this patch into three patches? I'm afraid that title of this CL is misleading because two of threes don't use the composed tree. I am feeling that it'd be better to split and handle each use case separately.
One more concern is that there is a gap between the semantics of parentOrShadowHost and the definition of 'a tree of trees'. parentOrShadowHost() doesn't traverse ancestors in "the tree of trees" strictly.
hayato@, the test coverage is in sync with the other CL for the lang attribute. https://codereview.chromium.org/878743003/ You don't have to add the same comment if any, I'll keep all tests in sync, but in case you want to see this one together, here's for your review.
Patchset #8 (id:130001) has been deleted
tests upgraded in sync with the latest comments for lang CL: https://codereview.chromium.org/878743003/#ps20001
I think we can improve the tests. - We should have a test case where toolTipText is not 'PASS'. In the current test sets, if the implementation always return 'PASS' regardless of the condition, all tests will pass. - We should have a test case where the implementation actually uses the composed tree. For example, the near ancestor in the composed tree should win the far ancestor. In the current tests sets, all test will pass even when the implementation uses a tree of trees. https://codereview.chromium.org/859943002/diff/170001/LayoutTests/fast/dom/sh... File LayoutTests/fast/dom/shadow/attr-title-inherit.html (right): https://codereview.chromium.org/859943002/diff/170001/LayoutTests/fast/dom/sh... LayoutTests/fast/dom/shadow/attr-title-inherit.html:30: testTooltipTextInShadow("The title of elements in shadow inherits from the document", root.querySelector("div")); root.querySelector(".target") might be better in terms of readability. https://codereview.chromium.org/859943002/diff/170001/LayoutTests/fast/dom/sh... LayoutTests/fast/dom/shadow/attr-title-inherit.html:34: testTooltipTextInShadow("The title of distributed elements inherits in the composed tree", host); Looks this should be |#multiple .target| rather than |host|.
hayato@, PTAL. > I think we can improve the tests. > > - We should have a test case where toolTipText is not 'PASS'. In the current > test sets, if the implementation always return 'PASS' regardless of the > condition, all tests will pass. Found a bug in the test by following the advice, so much thank you. > - We should have a test case where the implementation actually uses the composed > tree. For example, the near ancestor in the composed tree should win the far > ancestor. In the current tests sets, all test will pass even when the > implementation uses a tree of trees. Absolutely right, changed to test the diff between the tree of trees and the composed tree. https://codereview.chromium.org/859943002/diff/170001/LayoutTests/fast/dom/sh... File LayoutTests/fast/dom/shadow/attr-title-inherit.html (right): https://codereview.chromium.org/859943002/diff/170001/LayoutTests/fast/dom/sh... LayoutTests/fast/dom/shadow/attr-title-inherit.html:30: testTooltipTextInShadow("The title of elements in shadow inherits from the document", root.querySelector("div")); On 2015/02/10 11:41:18, hayato wrote: > root.querySelector(".target") might be better in terms of readability. Done. https://codereview.chromium.org/859943002/diff/170001/LayoutTests/fast/dom/sh... LayoutTests/fast/dom/shadow/attr-title-inherit.html:34: testTooltipTextInShadow("The title of distributed elements inherits in the composed tree", host); On 2015/02/10 11:41:18, hayato wrote: > Looks this should be |#multiple .target| rather than |host|. Done.
lgtm
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/859943002/190001
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/859943002/190001
Message was sent while issue was closed.
Committed patchset #10 (id:190001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190138 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
