Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1153873004:
Handle ::content and :host-context correctly in SelectorQuery. (Closed)
|
Created:
5 years, 7 months ago by esprehn Modified:
5 years, 7 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionHandle ::content and :host-context correctly in SelectorQuery.
SelectorQuery was checking for selectors that cross tree scopes (::shadow and
/deep/) to decide if it should update distribution, but it should have been
checking for selectors that need distribution (::content and :host-context).
SelectorQuery's ::matches and ::closest modes were also missing the step to
update the distribution.
BUG=491641
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195882
Patch Set 1 #Patch Set 2 : Don't mess with scope. #Patch Set 3 : Remove wrong test. #
Total comments: 7
Patch Set 4 : Add the test back. #Patch Set 5 : Fix test path. #Patch Set 6 : No really, the right patch. #
Total comments: 5
Patch Set 7 : Expanded test #
Total comments: 1
Patch Set 8 : fix test naming. #
Total comments: 2
Messages
Total messages: 60 (14 generated)
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153873004/1
Hmm this changes the behavior of fast/selectors/element-closest-scope.html, it seems like it changed how :scope matches. I think we need to rethink how scope handling works in SelectorChecker.
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153873004/20001
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153873004/40001
esprehn@chromium.org changed reviewers: + hayato@chromium.org, kochi@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Element... File Source/core/dom/Element.cpp (left): https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Element... Source/core/dom/Element.cpp:2785: updateDistribution(); Could you explain why calling updateDistribution blindly is bad here? Do you have a concern about the performance impact of the updateDistribution()?
It's unnecessary churn wasting time and battery, we shouldn't force updates
unless we need to. I certainly wouldn't expect .matches("div") to force a
distribution.
On May 24, 2015 10:15 PM, <hayato@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Element...
> File Source/core/dom/Element.cpp (left):
>
>
>
https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Element...
> Source/core/dom/Element.cpp:2785: updateDistribution();
> Could you explain why calling updateDistribution blindly is bad here?
> Do you have a concern about the performance impact of the
> updateDistribution()?
>
> https://codereview.chromium.org/1153873004/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews+unsubscribe@chromium.org.
>
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.
Could you add regression tests for catching if ::matches() and ::closest() misses updating distribution? https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... File Source/core/dom/SelectorQuery.cpp (right): https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... Source/core/dom/SelectorQuery.cpp:226: return m_selectors.size() == 1; (just a comment/question) I'm curious why the order is reordered or what is the reason for this order (except for the addition of m_needsUpdatedDistribution). In general for faster short-circuting, most probable thing is placed first, right? https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... File Source/core/dom/SelectorQuery.h (right): https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... Source/core/dom/SelectorQuery.h:84: bool m_needsUpdatedDistribution : 1; I'm not against this, or rather like this, but I'm curious if there's a rule of thumb for Blink where to use bitfields or not to use bitfields and use booleans instead, as outside Blink. I'm asking because you recently did the opposite: https://codereview.chromium.org/1106183007 (not exactly opposite, but unbitfield'ized). Is this because the 2 conditions in canUseFastQuery() if (m_crossesTreeBoundary) return false; if (m_needsUpdateDistribution) return false; can be merged into one check (data & 0x03) != 0 by compiler?
Yeah, but this wouldn't change the number of calculation of the distribution
significantly?
I expect that this would change only the timing of the distribution in most
cases. Sooner or later, the distribution should be calculated in any cases.
I'm not opposing to this change. The change makes sense. But I don't think this
change would change the performance significantly.
On 2015/05/25 05:38:17, esprehn wrote:
> It's unnecessary churn wasting time and battery, we shouldn't force updates
> unless we need to. I certainly wouldn't expect .matches("div") to force a
> distribution.
> On May 24, 2015 10:15 PM, <mailto:hayato@chromium.org> wrote:
>
> >
> >
> >
>
https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Element...
> > File Source/core/dom/Element.cpp (left):
> >
> >
> >
>
https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Element...
> > Source/core/dom/Element.cpp:2785: updateDistribution();
> > Could you explain why calling updateDistribution blindly is bad here?
> > Do you have a concern about the performance impact of the
> > updateDistribution()?
> >
> > https://codereview.chromium.org/1153873004/
> >
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:blink-reviews+unsubscribe@chromium.org.
> >
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.
Hayato-san,
I think this CL's point is that set the needsUpdateDistribution flag only when
Element.match()'s argument contains ::content or :host-context and otherwise
don't cause update distribution. So it should reduce the number of distribution
recalc significantly, assuming ::content or :host-context is rarely used for
match().
On 2015/05/25 06:02:20, hayato wrote:
> Yeah, but this wouldn't change the number of calculation of the distribution
> significantly?
> I expect that this would change only the timing of the distribution in most
> cases. Sooner or later, the distribution should be calculated in any cases.
>
> I'm not opposing to this change. The change makes sense. But I don't think
this
> change would change the performance significantly.
>
> On 2015/05/25 05:38:17, esprehn wrote:
> > It's unnecessary churn wasting time and battery, we shouldn't force updates
> > unless we need to. I certainly wouldn't expect .matches("div") to force a
> > distribution.
> > On May 24, 2015 10:15 PM, <mailto:hayato@chromium.org> wrote:
> >
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Element...
> > > File Source/core/dom/Element.cpp (left):
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Element...
> > > Source/core/dom/Element.cpp:2785: updateDistribution();
> > > Could you explain why calling updateDistribution blindly is bad here?
> > > Do you have a concern about the performance impact of the
> > > updateDistribution()?
> > >
> > > https://codereview.chromium.org/1153873004/
> > >
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > > email to mailto:blink-reviews+unsubscribe@chromium.org.
> > >
> >
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... File Source/core/dom/SelectorQuery.cpp (right): https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... Source/core/dom/SelectorQuery.cpp:226: return m_selectors.size() == 1; On 2015/05/25 at 05:40:57, Takayoshi Kochi wrote: > (just a comment/question) > I'm curious why the order is reordered or what is the reason for this order > (except for the addition of m_needsUpdatedDistribution). > > In general for faster short-circuting, most probable thing is placed first, right? I usually put boolean checks first, they're better for memory locality, but I didn't put too much thought into this. I guess the selectors size check could be first to avoid the rest of the branches. We only do this once per query and it's fixed size cost so I wasn't too worried. https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... File Source/core/dom/SelectorQuery.h (right): https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... Source/core/dom/SelectorQuery.h:84: bool m_needsUpdatedDistribution : 1; Those objects are on the stack, so using bitfields just means more bit masking at runtime and we never get deep enough in the stack for the memory savings to matter. SelectorQuery objects are stored in the query cache on the heap so adding a new member increases memory usage of the cache entries. Using a bitfield means no memory increase.
On 2015/05/25 at 05:40:57, kochi wrote: > Could you add regression tests for catching if > ::matches() and ::closest() misses updating distribution? > That's what my test does. :)
updateDistribution() would be almost no-op, only checking the flag of document
in most cases, once the distribution is calculated and up-to-date.
It costs only when the distribution is dirty.
On 2015/05/25 06:15:26, Takayoshi Kochi wrote:
> Hayato-san,
>
> I think this CL's point is that set the needsUpdateDistribution flag only when
> Element.match()'s argument contains ::content or :host-context and otherwise
> don't cause update distribution. So it should reduce the number of
distribution
> recalc significantly, assuming ::content or :host-context is rarely used for
> match().
>
> On 2015/05/25 06:02:20, hayato wrote:
> > Yeah, but this wouldn't change the number of calculation of the distribution
> > significantly?
> > I expect that this would change only the timing of the distribution in most
> > cases. Sooner or later, the distribution should be calculated in any cases.
> >
> > I'm not opposing to this change. The change makes sense. But I don't think
> this
> > change would change the performance significantly.
> >
> > On 2015/05/25 05:38:17, esprehn wrote:
> > > It's unnecessary churn wasting time and battery, we shouldn't force
updates
> > > unless we need to. I certainly wouldn't expect .matches("div") to force a
> > > distribution.
> > > On May 24, 2015 10:15 PM, <mailto:hayato@chromium.org> wrote:
> > >
> > > >
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Element...
> > > > File Source/core/dom/Element.cpp (left):
> > > >
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Element...
> > > > Source/core/dom/Element.cpp:2785: updateDistribution();
> > > > Could you explain why calling updateDistribution blindly is bad here?
> > > > Do you have a concern about the performance impact of the
> > > > updateDistribution()?
> > > >
> > > > https://codereview.chromium.org/1153873004/
> > > >
> > > > To unsubscribe from this group and stop receiving emails from it, send
an
> > > > email to mailto:blink-reviews+unsubscribe@chromium.org.
> > > >
> > >
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:blink-reviews+unsubscribe@chromium.org.
On 2015/05/25 06:21:42, esprehn wrote: > On 2015/05/25 at 05:40:57, kochi wrote: > > Could you add regression tests for catching if > > ::matches() and ::closest() misses updating distribution? > > > > That's what my test does. :) ??, it was removed from patch2 to patch3, right?
On 2015/05/25 at 06:02:20, hayato wrote:
> Yeah, but this wouldn't change the number of calculation of the distribution
significantly?
> I expect that this would change only the timing of the distribution in most
cases. Sooner or later, the distribution should be calculated in any cases.
>
> I'm not opposing to this change. The change makes sense. But I don't think
this change would change the performance significantly.
>
Sure it does, imagine this:
<div id="host"> -> ShadowRoot(1)
ShadowRoot(1)
<content></content>
list = [10 elements]
for (var i = 0; i < list.length; ++i) {
var node = list[i];
host.appendChild(node);
if (node.matches(filter))
doSomething(node);
}
The previous code would have caused 10 distributions, each time the loop called
match(), each time redoing the distribution for host.
Or imagine I do:
host.appendChild(10 elements);
if (someElementInTheDocument.matches(filter))
// do something else
host.appendChild(10 more elements);
The previous code would have run the distribution for host twice.
Another example would be if you have two nested shadow roots,
host1 -> SR(1)
SR(1)
host2 -> SR(2)
if I modify the children of host2, call matches(), then modify the children of
host1 and call matches() we'd distribute twice.
Note that the previous code was also error prone, it didn't updateDistribution
for closest(). This new code makes sure all users of SelectorQuery get the right
answer.
On 2015/05/25 at 06:26:23, kochi wrote: > On 2015/05/25 06:21:42, esprehn wrote: > > On 2015/05/25 at 05:40:57, kochi wrote: > > > Could you add regression tests for catching if > > > ::matches() and ::closest() misses updating distribution? > > > > > > > That's what my test does. :) > > ??, it was removed from patch2 to patch3, right? Woops, I failed at uploading.
Yeah, we can create such a case intentionally. I'm aware of it. That's the
reason I'm not opposing to this change.
I just wanted to say that in most cases, we don't encounter such a case. :)
On 2015/05/25 06:30:42, esprehn wrote:
> On 2015/05/25 at 06:02:20, hayato wrote:
> > Yeah, but this wouldn't change the number of calculation of the distribution
> significantly?
> > I expect that this would change only the timing of the distribution in most
> cases. Sooner or later, the distribution should be calculated in any cases.
> >
> > I'm not opposing to this change. The change makes sense. But I don't think
> this change would change the performance significantly.
> >
>
> Sure it does, imagine this:
>
> <div id="host"> -> ShadowRoot(1)
>
> ShadowRoot(1)
> <content></content>
>
> list = [10 elements]
>
> for (var i = 0; i < list.length; ++i) {
> var node = list[i];
> host.appendChild(node);
> if (node.matches(filter))
> doSomething(node);
> }
>
> The previous code would have caused 10 distributions, each time the loop
called
> match(), each time redoing the distribution for host.
>
> Or imagine I do:
>
> host.appendChild(10 elements);
> if (someElementInTheDocument.matches(filter))
> // do something else
> host.appendChild(10 more elements);
>
> The previous code would have run the distribution for host twice.
>
> Another example would be if you have two nested shadow roots,
>
> host1 -> SR(1)
> SR(1)
> host2 -> SR(2)
>
> if I modify the children of host2, call matches(), then modify the children of
> host1 and call matches() we'd distribute twice.
>
> Note that the previous code was also error prone, it didn't updateDistribution
> for closest(). This new code makes sure all users of SelectorQuery get the
right
> answer.
On 2015/05/25 at 06:33:55, hayato wrote:
> Yeah, we can create such a case intentionally. I'm aware of it. That's the
reason I'm not opposing to this change.
>
> I just wanted to say that in most cases, we don't encounter such a case. :)
>
Maybe. Authors have no reason to think that matches("div") will force a
distribution for no reason so they're not going to be very careful about it
(unlike with offsetTop where they're better about not causing churn).
We shouldn't be causing distribution updates in places where authors would not
expect them.
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153873004/60001
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153873004/80001
Okay test is added back, ready for another look.
On 2015/05/25 06:50:40, esprehn wrote: > Okay test is added back, ready for another look. With this patch, fast/selectors/element-closest-scope.html fails locally (without, it succeeds). Probably trybots will catch this, but could you take a look?
On 2015/05/25 at 07:38:19, kochi wrote: > On 2015/05/25 06:50:40, esprehn wrote: > > Okay test is added back, ready for another look. > > With this patch, fast/selectors/element-closest-scope.html fails locally > (without, it succeeds). > Probably trybots will catch this, but could you take a look? Sigh, that's more failure with my upload. I messed up my git checkout and merged two versions of this patch.
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153873004/100001
Okay! Now this is the right patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... File Source/core/dom/SelectorQuery.cpp (right): https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... Source/core/dom/SelectorQuery.cpp:226: return m_selectors.size() == 1; On 2015/05/25 06:21:11, esprehn wrote: > On 2015/05/25 at 05:40:57, Takayoshi Kochi wrote: > > (just a comment/question) > > I'm curious why the order is reordered or what is the reason for this order > > (except for the addition of m_needsUpdatedDistribution). > > > > In general for faster short-circuting, most probable thing is placed first, > right? > > I usually put boolean checks first, they're better for memory locality, but I > didn't put too much thought into this. I guess the selectors size check could be > first to avoid the rest of the branches. We only do this once per query and it's > fixed size cost so I wasn't too worried. boolean member checks should be faster than member function calls even when inlined, so this ordering makes sense to me. Anyway, this doesn't look performance-critical path and okay with this ordering. https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... File Source/core/dom/SelectorQuery.h (right): https://codereview.chromium.org/1153873004/diff/40001/Source/core/dom/Selecto... Source/core/dom/SelectorQuery.h:84: bool m_needsUpdatedDistribution : 1; On 2015/05/25 06:21:11, esprehn wrote: > Those objects are on the stack, so using bitfields just means more bit masking > at runtime and we never get deep enough in the stack for the memory savings to > matter. > > SelectorQuery objects are stored in the query cache on the heap so adding a new > member increases memory usage of the cache entries. Using a bitfield means no > memory increase. Thanks for the explanation! https://codereview.chromium.org/1153873004/diff/100001/LayoutTests/fast/selec... File LayoutTests/fast/selectors/query-update-distribution.html (right): https://codereview.chromium.org/1153873004/diff/100001/LayoutTests/fast/selec... LayoutTests/fast/selectors/query-update-distribution.html:30: shouldBeNull("b.closest('::content #a')"); Hopefully, we have 3 separate cases, 1. createShadowRoot(), then immediately .querySelector() 2. createShadowRoot(), then immediately .matches() 3. createShadowRoot(), then immediately .closest() to check if all added .updateDistribution() calls in SelectorDataList::execute(), ::matches(), ::closest() independently. IIUC, In this test, line 24 calls querySelector(), then updateDistribution() is done, and in the following lines updateDistribution() will never be called - in that case, createShadowRoot() -> matches() -> updateDistribution() and createShadowRoot() -> closest() -> updateDistribution() paths are not exercised by this layout test. https://codereview.chromium.org/1153873004/diff/100001/Source/core/dom/Select... File Source/core/dom/SelectorQuery.cpp (right): https://codereview.chromium.org/1153873004/diff/100001/Source/core/dom/Select... Source/core/dom/SelectorQuery.cpp:134: targetElement.updateDistribution(); At first I wondered why after updateDistribution() m_needsUpdatedDistribution isn't cleared, but now I suppose all the members in SelectorDataList are immutable after initialize() is done. Then I have a question that if this data is cached in SelectorQueryCache, the m_needsUpdatedDistribution is also cached, right? Does it have any issue, once it is cached with m_needsUpdatedDistribution = false, then some shadow tree state is changed and the same query is given? SelectorQueryCache::invalidate() looks called only from Document.cpp, setCompatibilityMode() and updateBaseURL().
Okay look again. https://codereview.chromium.org/1153873004/diff/100001/LayoutTests/fast/selec... File LayoutTests/fast/selectors/query-update-distribution.html (right): https://codereview.chromium.org/1153873004/diff/100001/LayoutTests/fast/selec... LayoutTests/fast/selectors/query-update-distribution.html:30: shouldBeNull("b.closest('::content #a')"); Updated the test to dirty between each test and added a few more cases. https://codereview.chromium.org/1153873004/diff/100001/Source/core/dom/Select... File Source/core/dom/SelectorQuery.cpp (right): https://codereview.chromium.org/1153873004/diff/100001/Source/core/dom/Select... Source/core/dom/SelectorQuery.cpp:134: targetElement.updateDistribution(); SelectorQuery is just a "query plan", it's how the query should be executed, not something dynamic that should reset it's bits. The only time these plans need to be updated is when the way the selector was parsed could have changed, which is when the document mode or uri could have changed that's why those are the places we call invalidate().
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153873004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with one nit fixed. https://codereview.chromium.org/1153873004/diff/100001/Source/core/dom/Select... File Source/core/dom/SelectorQuery.cpp (right): https://codereview.chromium.org/1153873004/diff/100001/Source/core/dom/Select... Source/core/dom/SelectorQuery.cpp:134: targetElement.updateDistribution(); On 2015/05/25 20:52:26, esprehn wrote: > SelectorQuery is just a "query plan", it's how the query should be executed, not > something dynamic that should reset it's bits. The only time these plans need to > be updated is when the way the selector was parsed could have changed, which is > when the document mode or uri could have changed that's why those are the places > we call invalidate(). Thanks, I understand. https://codereview.chromium.org/1153873004/diff/120001/LayoutTests/fast/selec... File LayoutTests/fast/selectors/query-update-distribution.html (right): https://codereview.chromium.org/1153873004/diff/120001/LayoutTests/fast/selec... LayoutTests/fast/selectors/query-update-distribution.html:19: b = document.getElementById("a"); nit: getElementById("b"); ?
On 2015/05/26 at 00:59:05, kochi wrote: > ... > > https://codereview.chromium.org/1153873004/diff/120001/LayoutTests/fast/selec... > File LayoutTests/fast/selectors/query-update-distribution.html (right): > > https://codereview.chromium.org/1153873004/diff/120001/LayoutTests/fast/selec... > LayoutTests/fast/selectors/query-update-distribution.html:19: b = document.getElementById("a"); > nit: getElementById("b"); > > ? Fixed.
The CQ bit was checked by esprehn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kochi@chromium.org Link to the patchset: https://codereview.chromium.org/1153873004/#ps140001 (title: "fix test naming.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153873004/140001
https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... File LayoutTests/fast/selectors/query-update-distribution.html (right): https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... LayoutTests/fast/selectors/query-update-distribution.html:47: shouldBeNull("hostRoot.querySelector('::content #a')"); This should match, shouldn't this?
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195882
Message was sent while issue was closed.
rune@opera.com changed reviewers: + rune@opera.com
Message was sent while issue was closed.
https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... File LayoutTests/fast/selectors/query-update-distribution.html (right): https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... LayoutTests/fast/selectors/query-update-distribution.html:47: shouldBeNull("hostRoot.querySelector('::content #a')"); On 2015/05/26 02:29:18, hayato wrote: > This should match, shouldn't this? querySelector(All) would traverse descendants of hostRoot, #a is not one of them, it's just distributed there. Is there a specification for querySelector in shadow doms?
Message was sent while issue was closed.
On 2015/05/26 05:26:04, rune wrote: > https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... > File LayoutTests/fast/selectors/query-update-distribution.html (right): > > https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... > LayoutTests/fast/selectors/query-update-distribution.html:47: > shouldBeNull("hostRoot.querySelector('::content #a')"); > On 2015/05/26 02:29:18, hayato wrote: > > This should match, shouldn't this? > > querySelector(All) would traverse descendants of hostRoot, #a is not one of > them, it's just distributed there. "::content" is there. This should match #a, I think. > > Is there a specification for querySelector in shadow doms? There is no monkey patch for querySelector() in Shadow DOM and CSS Scoping. Therefore I expect that the "::content" should have the same behavior either it is used in style sheets or querySelector(). Looks there is an existing layout test for "::content" used in selector, such as LayoutTests/fast/dom/shadow/content-pseudo-element.html, but there is no existing layout tests for querySelector() for "::content".
Message was sent while issue was closed.
On 2015/05/26 06:11:59, hayato wrote: > On 2015/05/26 05:26:04, rune wrote: > > > https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... > > File LayoutTests/fast/selectors/query-update-distribution.html (right): > > > > > https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... > > LayoutTests/fast/selectors/query-update-distribution.html:47: > > shouldBeNull("hostRoot.querySelector('::content #a')"); > > On 2015/05/26 02:29:18, hayato wrote: > > > This should match, shouldn't this? > > > > querySelector(All) would traverse descendants of hostRoot, #a is not one of > > them, it's just distributed there. > > "::content" is there. This should match #a, I think. > > > > > Is there a specification for querySelector in shadow doms? > > There is no monkey patch for querySelector() in Shadow DOM and CSS Scoping. > Therefore I expect that the "::content" should have the same behavior either it > is used in style sheets or querySelector(). > > Looks there is an existing layout test for "::content" used in selector, such as > LayoutTests/fast/dom/shadow/content-pseudo-element.html, but there is no > existing layout tests for querySelector() for "::content". This isn't about whether the selector matches, it's about whether the #a node is part of the nodes that are checked at all. If #a is included in the set of visitied nodes, I agree it should match, but I wouldn't expect querySelector to traverse the composed tree looking for candidates. #a is not in the tree-of-trees subtree of hostRoot.
Message was sent while issue was closed.
On 2015/05/26 07:22:22, rune wrote: > On 2015/05/26 06:11:59, hayato wrote: > > On 2015/05/26 05:26:04, rune wrote: > > > > > > https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... > > > File LayoutTests/fast/selectors/query-update-distribution.html (right): > > > > > > > > > https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... > > > LayoutTests/fast/selectors/query-update-distribution.html:47: > > > shouldBeNull("hostRoot.querySelector('::content #a')"); > > > On 2015/05/26 02:29:18, hayato wrote: > > > > This should match, shouldn't this? > > > > > > querySelector(All) would traverse descendants of hostRoot, #a is not one of > > > them, it's just distributed there. > > > > "::content" is there. This should match #a, I think. > > > > > > > > Is there a specification for querySelector in shadow doms? > > > > There is no monkey patch for querySelector() in Shadow DOM and CSS Scoping. > > Therefore I expect that the "::content" should have the same behavior either > it > > is used in style sheets or querySelector(). > > > > Looks there is an existing layout test for "::content" used in selector, such > as > > LayoutTests/fast/dom/shadow/content-pseudo-element.html, but there is no > > existing layout tests for querySelector() for "::content". > > This isn't about whether the selector matches, it's about whether the #a node is > part of the nodes that are checked at all. If #a is included in the set of > visitied nodes, I agree it should match, but I wouldn't expect querySelector to > traverse the composed tree looking for candidates. #a is not in the > tree-of-trees subtree of hostRoot.
Message was sent while issue was closed.
On 2015/05/26 07:22:22, rune wrote: > On 2015/05/26 06:11:59, hayato wrote: > > On 2015/05/26 05:26:04, rune wrote: > > > > > > https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... > > > File LayoutTests/fast/selectors/query-update-distribution.html (right): > > > > > > > > > https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... > > > LayoutTests/fast/selectors/query-update-distribution.html:47: > > > shouldBeNull("hostRoot.querySelector('::content #a')"); > > > On 2015/05/26 02:29:18, hayato wrote: > > > > This should match, shouldn't this? > > > > > > querySelector(All) would traverse descendants of hostRoot, #a is not one of > > > them, it's just distributed there. > > > > "::content" is there. This should match #a, I think. > > > > > > > > Is there a specification for querySelector in shadow doms? > > > > There is no monkey patch for querySelector() in Shadow DOM and CSS Scoping. > > Therefore I expect that the "::content" should have the same behavior either > it > > is used in style sheets or querySelector(). > > > > Looks there is an existing layout test for "::content" used in selector, such > as > > LayoutTests/fast/dom/shadow/content-pseudo-element.html, but there is no > > existing layout tests for querySelector() for "::content". > > This isn't about whether the selector matches, it's about whether the #a node is > part of the nodes that are checked at all. If #a is included in the set of > visitied nodes, I agree it should match, but I wouldn't expect querySelector to > traverse the composed tree looking for candidates. #a is not in the > tree-of-trees subtree of hostRoot. Hmm. I don't have a strong opinion about this. However, it's okay to return #a in this case for the consistency to me. I think the content of the selector itself should decide which node should be {returned/matched} in any context. IMO, I don't think the context where the selector is used shouldn't decide which node should be {returned/matched}.
Message was sent while issue was closed.
The context here doesn't mean the context object. I meant to say a static profile, or dynamic profile, *by the term of the context* here. - e.g. as a parameter of querySelector, as appeared in a selector in style sheets, and so on.
Message was sent while issue was closed.
On 2015/05/26 07:45:53, hayato wrote: > On 2015/05/26 07:22:22, rune wrote: > > On 2015/05/26 06:11:59, hayato wrote: > > > On 2015/05/26 05:26:04, rune wrote: > > > > > > > > > > https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... > > > > File LayoutTests/fast/selectors/query-update-distribution.html (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1153873004/diff/140001/LayoutTests/fast/selec... > > > > LayoutTests/fast/selectors/query-update-distribution.html:47: > > > > shouldBeNull("hostRoot.querySelector('::content #a')"); > > > > On 2015/05/26 02:29:18, hayato wrote: > > > > > This should match, shouldn't this? > > > > > > > > querySelector(All) would traverse descendants of hostRoot, #a is not one > of > > > > them, it's just distributed there. > > > > > > "::content" is there. This should match #a, I think. > > > > > > > > > > > Is there a specification for querySelector in shadow doms? > > > > > > There is no monkey patch for querySelector() in Shadow DOM and CSS Scoping. > > > Therefore I expect that the "::content" should have the same behavior either > > it > > > is used in style sheets or querySelector(). > > > > > > Looks there is an existing layout test for "::content" used in selector, > such > > as > > > LayoutTests/fast/dom/shadow/content-pseudo-element.html, but there is no > > > existing layout tests for querySelector() for "::content". > > > > This isn't about whether the selector matches, it's about whether the #a node > is > > part of the nodes that are checked at all. If #a is included in the set of > > visitied nodes, I agree it should match, but I wouldn't expect querySelector > to > > traverse the composed tree looking for candidates. #a is not in the > > tree-of-trees subtree of hostRoot. > > Hmm. I don't have a strong opinion about this. However, it's okay to return #a > in this case for the consistency to me. > > I think the content of the selector itself should decide which node should be > {returned/matched} in any context. > IMO, I don't think the context where the selector is used shouldn't decide which > node should be {returned/matched}. Again, this is not about selector matching. Selector matching should be consistent. querySelector(All) is dom-centric, and being able to find #a through hostRoot.querySelector, while not being able to find #a using firstChild/nextSibling from hostRoot would be inconsistent, I think.
Message was sent while issue was closed.
I think it's okay to return a node which is in other node trees if a shadow boundary crossing selector is used as a parameter of querySelector(). In fact, what this bug [1] is trying to resolve depends on this kind of behavior. [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591 Let me file a bug to discuss further.
Message was sent while issue was closed.
On 2015/05/26 08:23:51, hayato wrote: > I think it's okay to return a node which is in other node trees if a shadow > boundary crossing selector is used as a parameter of querySelector(). > In fact, what this bug [1] is trying to resolve depends on this kind of > behavior. > > [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591 > > Let me file a bug to discuss further. Feel free to CC me on that.
Message was sent while issue was closed.
On 2015/05/26 08:31:23, rune wrote: > On 2015/05/26 08:23:51, hayato wrote: > > I think it's okay to return a node which is in other node trees if a shadow > > boundary crossing selector is used as a parameter of querySelector(). > > In fact, what this bug [1] is trying to resolve depends on this kind of > > behavior. > > > > [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=28591 > > > > Let me file a bug to discuss further. > > Feel free to CC me on that. Sure, filed as https://code.google.com/p/chromium/issues/detail?id=492080. |
