Closed Bug 1455891 Opened 6 years ago Closed 6 years ago

Can't find text content on Shadow DOM.

Categories

(Core :: Find Backend, defect, P2)

Product:
Core
Component:
Find Backend
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(10 files, 1 obsolete file)

Simple test-case.
210 bytes, text/html
Details
Bug 1455891: Improve StyleChildrenIterator.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1455891: Add a generic pre-order tree iterator given a child iterator.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1455891: Use TreeIterator in nsFind.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1455891: Remove nsFindContentIterator.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1455891: Remove nsRange::mMaySpanAnonymousSubtrees.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1455891: Use GetFlattenedTreeParent more in nsFind.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1455891: Add a test of finding text in Shadow DOM.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1455891: Don't return NAC as the first visible node in nsTypeAheadFind.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Last part with a small fixup to handle textareas where all the text nodes are anonymous.
2.76 KB, patch
smaug
: review+
heycam
: review+
Details | Diff | Splinter Review 
STR:

 * Go to https://wpt.fyi/css/cssom/interfaces.html with dom.webcomponents.shadowdom.enabled=true.
 * Press Ctrl + F, and try to search for "CSS".

Expected:

  Results appear.

Actual:

  Results don't appear, because the content is in a Shadow Root.
Attached file Simple test-case. — 
"And me" isn't greppable.
Component: DOM → Find Toolbar
Product: Core → Toolkit
Depends on: 1455894
No longer depends on: 1455894
Component: Find Toolbar → Find Backend
Product: Toolkit → Core
Depends on: 1457286
Priority: -- → P2
Attachment #8969920 - Attachment mime type: text/plain → text/html
Depends on: 1470861
Depends on: 1472529
Comment on attachment 8989349 [details]
Bug 1455891: Improve StyleChildrenIterator.

https://reviewboard.mozilla.org/r/254430/#review261240

::: dom/base/ChildIterator.h:302
(Diff revision 1)
>   * StyleChildrenIterators.
>   */
>  class MOZ_NEEDS_MEMMOVABLE_MEMBERS StyleChildrenIterator : private AllChildrenIterator
>  {
>  public:
> -  explicit StyleChildrenIterator(const nsIContent* aContent)
> +  static nsIContent* GetParent(const nsIContent& aContent)

I guess I'll understand why this returns nsIContent and not nsINode after reading some other patch.
Attachment #8989349 - Flags: review?(bugs) → review+
Comment on attachment 8989350 [details]
Bug 1455891: Add a generic pre-order tree iterator given a child iterator.

https://reviewboard.mozilla.org/r/254432/#review261242

::: dom/base/TreeIterator.h:13
(Diff revision 1)
> +#define TreeIterator_h
> +
> +#include "mozilla/Attributes.h"
> +#include "nsIContent.h"
> +
> +/* A generic pre-order tree iterator */

This needs a bit better comment. This is not a generic pre-order tree iterator. This iterates based on the ChildIterator behavior.
Or perhaps I interpret 'generic' in a bit different way. Sure, this is a generic iterator on top of ChildIterator.
Anyhow, a bit better comment would be nice.


I wonder if this could be incorporated to ChildIterator.h This is after all a small helper API on the classes there. Up to you.
But if not in the same file, the comment here should explain how this is supposed to be used. Perhaps some dummy example even in the code.

::: dom/base/TreeIterator.h:98
(Diff revision 1)
> +  mParentIterators.Clear();
> +
> +  while (current != &mRoot) {
> +    nsIContent* parent = ChildIterator::GetParent(*current);
> +    if (!parent) {
> +      mParentIterators = std::move(oldIterators);

ok, so it needs to be defined that if Seek returns false, TreeIterator state is kept as it was before the call.
Attachment #8989350 - Flags: review?(bugs) → review+
Comment on attachment 8989351 [details]
Bug 1455891: Use TreeIterator in nsFind.

https://reviewboard.mozilla.org/r/254434/#review261486

::: toolkit/components/find/nsFind.cpp:641
(Diff revision 1)
> -  // Disallow copying because copying the iterator would be a lie.
> +  State(bool aFindBackward, nsIContent& aRoot, const nsRange& aStartPoint)
> -  State(const State&) = delete;

Can aRoot be "const nsIContent&" ?
Attachment #8989351 - Flags: review?(mats) → review+
Comment on attachment 8989352 [details]
Bug 1455891: Remove nsFindContentIterator.

https://reviewboard.mozilla.org/r/254436/#review261488
Attachment #8989352 - Flags: review?(mats) → review+
Comment on attachment 8989353 [details]
Bug 1455891: Remove nsRange::mMaySpanAnonymousSubtrees.

https://reviewboard.mozilla.org/r/254438/#review261490

Nice!
Attachment #8989353 - Flags: review?(mats) → review+
Comment on attachment 8989354 [details]
Bug 1455891: Use GetFlattenedTreeParent more in nsFind.

https://reviewboard.mozilla.org/r/254440/#review261492

::: toolkit/components/find/nsFind.cpp:209
(Diff revision 1)
>    // FIXME(emilio): This should use GetFlattenedTreeParent instead to properly
>    // handle Shadow DOM.
> -  for (const nsIContent* current = aNode->GetParent(); current;
> +  for (const nsIContent* current = aNode.GetFlattenedTreeParent();

Remove the comment.
Attachment #8989354 - Flags: review?(mats) → review+
Comment on attachment 8989355 [details]
Bug 1455891: Add a test of finding text in Shadow DOM.

https://reviewboard.mozilla.org/r/254442/#review261494

Does this test finding "FooBar" where "Foo" is in light DOM and "Bar" is in shadow DOM?
If not, please add such a test.
Attachment #8989355 - Flags: review?(mats) → review+
... and vice versa too.
(In reply to Mats Palmgren (:mats) from comment #16)
> Comment on attachment 8989355 [details]
> Bug 1455891: Add a test of finding text in Shadow DOM.
> 
> https://reviewboard.mozilla.org/r/254442/#review261494
> 
> Does this test finding "FooBar" where "Foo" is in light DOM and "Bar" is in
> shadow DOM?
> If not, please add such a test.

It doesn't because we only return a range from nsFind. That's a much bigger change I'm not sure it's worth doing.
OK, but isn't it worth having a test for it anyway even it doesn't find anything?
In case it triggers an assertion or whatnot.
(In reply to Mats Palmgren (:mats) (on vacation) from comment #19)
> OK, but isn't it worth having a test for it anyway even it doesn't find
> anything?
> In case it triggers an assertion or whatnot.

Sure, will do.
Comment on attachment 8989655 [details]
Bug 1455891: Don't return NAC as the first visible node in nsTypeAheadFind.

https://reviewboard.mozilla.org/r/254658/#review261580

I'm missing the context here so can't really review.
Why this becomes a problem now?

::: commit-message-f1221:7
(Diff revision 1)
> +
> +This fixes all the test failures that can be seen in
> +
> +  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cbc0047358cb24fd3bc48437d5d598169427b0a
> +
> +They are because we were returning a <resizer> frame for a <textarea> instead of

This comment is unclear. It says we iterate first non-anonymous, yet we never find non-anonymous
Comment on attachment 8989655 [details]
Bug 1455891: Don't return NAC as the first visible node in nsTypeAheadFind.

could you explain why this becomes an issue now and ask review again, thanks.
Attachment #8989655 - Flags: review?(bugs)
Comment on attachment 8989655 [details]
Bug 1455891: Don't return NAC as the first visible node in nsTypeAheadFind.

So the issue is that we append the editor NAC after the normal frame NAC:

  https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/dom/base/nsContentUtils.cpp#10388

The editor handles are absolutely positioned, so their order in the frame tree doesn't matter (and it's not deterministic):

  https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/editor/libeditor/HTMLAnonymousNodeEditor.cpp#222

So the answer is that right now it works by chance, because the resizer happens to be in the initial position which is first in the frame tree, and nsFindContentIterator iterates the light tree after the NAC.
Attachment #8989655 - Flags: review?(bugs)
Or put in other words, the frame tree order for the resizer NAC is not guaranteed.
So why the order changes with the patches? Or is the order different when textarea is in shadow DOM?
(In reply to Olli Pettay [:smaug] from comment #33)
> So why the order changes with the patches? Or is the order different when
> textarea is in shadow DOM?

It is not, the order is the same (though again, those tests are relying on the order being the initial one), but we no longer iterate the content in the same order. Before this patch, if nsFindContentIterators::mStartNode was NAC, we iterated over the light tree children as if they were "next siblings" to some extent. AllChildrenIterator iterates NAC after explicit children, so if we start at NAC we no longer find the light tree "siblings".
That is, before, for a <textarea>e1</textarea> we were iterating like: [Text('e1'), <resizer>], and now we iterate [<resizer>, Text('e1')].
I guess I should have read 'Use TreeIterator in nsFind'
Comment on attachment 8989655 [details]
Bug 1455891: Don't return NAC as the first visible node in nsTypeAheadFind.

https://reviewboard.mozilla.org/r/254658/#review261722
Attachment #8989655 - Flags: review?(bugs) → review+
Comment on attachment 8989348 [details]
Bug 1455891: Add move-{construction,assignment} to AutoTArray.

Eric can review this.  I feel like there was some reason we didn't want this, but I cannot think of what it is...
Attachment #8989348 - Flags: review?(nfroyd) → review?(erahm)
(In reply to Nathan Froyd [:froydnj] (away until 16 July 2018) from comment #38)
> Comment on attachment 8989348 [details]
> Bug 1455891: Add move-{construction,assignment} to AutoTArray.
> 
> Eric can review this.  I feel like there was some reason we didn't want
> this, but I cannot think of what it is...

Nika has a slightly cleaner version of this in bug 1461450. We should probably just land that with tests and rebase here.
Depends on: 1461450
Comment on attachment 8989348 [details]
Bug 1455891: Add move-{construction,assignment} to AutoTArray.

I guess I can use SwapElements for now.
Attachment #8989348 - Attachment is obsolete: true
Attachment #8989348 - Flags: review?(erahm)
This was breaking one last test which set the value of the textarea dynamically.

The frame traversal iterates only leafs, so if we don't create a frame for the non-anonymous text we won't find the textarea, which is unfortunate.

All this code for finding the first visible node is a bit fishy, but this should preserve the existing behavior by starting iteration from the textarea.
Attachment #8990155 - Flags: review?(bugs)
See Also: → 1473694
Comment on attachment 8990155 [details] [diff] [review]
Last part with a small fixup to handle textareas where all the text nodes are anonymous.

Cam may be a better reviewer for this, actually... Mats would work as well, but he's on vacation.
Attachment #8990155 - Flags: review?(cam)
Comment on attachment 8989354 [details]
Bug 1455891: Use GetFlattenedTreeParent more in nsFind.

https://reviewboard.mozilla.org/r/254440/#review262096

::: toolkit/components/find/nsFind.cpp:209
(Diff revision 2)
>    // FIXME(emilio): This should use GetFlattenedTreeParent instead to properly
>    // handle Shadow DOM.

Remove this comment?
Comment on attachment 8990155 [details] [diff] [review]
Last part with a small fixup to handle textareas where all the text nodes are anonymous.

Review of attachment 8990155 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
@@ +1345,4 @@
>        return false;
> +    }
> +
> +    while (frame->GetContent() &&

This really needs a comment to explain why we're doing this.

Does this also mean that we could repeatedly grab frames for NAC out of frameTraversal, and keep iterating up to the NAC subtree root's parent?
Comment on attachment 8990155 [details] [diff] [review]
Last part with a small fixup to handle textareas where all the text nodes are anonymous.

Review of attachment 8990155 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
@@ +1345,4 @@
>        return false;
> +    }
> +
> +    while (frame->GetContent() &&

Yes, but that's not an issue.

I'll leave a comment in the spirit of the commit message and the comment that adds this attachment.
(In reply to Cameron McCormack (:heycam) (busy until July 12) from comment #43)
> Comment on attachment 8989354 [details]
> Bug 1455891: Use GetFlattenedTreeParent more in nsFind.
> 
> https://reviewboard.mozilla.org/r/254440/#review262096
> 
> ::: toolkit/components/find/nsFind.cpp:209
> (Diff revision 2)
> >    // FIXME(emilio): This should use GetFlattenedTreeParent instead to properly
> >    // handle Shadow DOM.
> 
> Remove this comment?

Sure, nice catch :)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #45)
> Yes, but that's not an issue.

Ah, understand, because we'll break out of the loop straight after the first time we do that.

> I'll leave a comment in the spirit of the commit message and the comment
> that adds this attachment.

Thanks!
Comment on attachment 8990155 [details] [diff] [review]
Last part with a small fixup to handle textareas where all the text nodes are anonymous.

r=me with the comment and with smaug's comment on the commit message from the previous version addressed.
Attachment #8990155 - Flags: review?(cam) → review+
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da1bcd64ade4
Improve StyleChildrenIterator. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8743ed4bba6d
Add a generic pre-order tree iterator given a child iterator. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f391ac72f57
Use TreeIterator in nsFind. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eca66a467ee
Remove nsFindContentIterator. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5c44bc21d31
Remove nsRange::mMaySpanAnonymousSubtrees. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1c19f0dee8
Use GetFlattenedTreeParent more in nsFind. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/19669decb75f
Add a test of finding text in Shadow DOM. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/148b5283ed5d
Don't return NAC as the first visible node in nsTypeAheadFind. r=heycam
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7edfab4b2a
followup: Fix static analysis builds. r=me CLOSED TREE
Comment on attachment 8990155 [details] [diff] [review]
Last part with a small fixup to handle textareas where all the text nodes are anonymous.

Not useful to have links to treeherder in commit messages. Those links won't work for too long.
Attachment #8990155 - Flags: review?(bugs) → review+
Assignee: nobody → emilio
Regressions: 1601118
Regressions: 1798207
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: