Bug 234214 – Add support for detecting modal containers that contain a given search term

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 446944 [details]
Patch

[Wenson Hsieh]

Created attachment 446964 [details]
Slightly simpler approach

[Wenson Hsieh]

Comment on attachment 446964 [details]
Slightly simpler approach

The webrtc and XHR test failures do not appear to be related.

[Wenson Hsieh]

Created attachment 447020 [details]
Minor style tweaks

[Simon Fraser (smfr)]

Comment on attachment 447020 [details]
Minor style tweaks

If this is just about fixed position, please just use the term "fixed". "Viewport constrained" means "fixed or sticky" in the rest of the code.

[Wenson Hsieh]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 447020 [details]
> Minor style tweaks
> 
> If this is just about fixed position, please just use the term "fixed".
> "Viewport constrained" means "fixed or sticky" in the rest of the code.

Will do!

[Wenson Hsieh]

Created attachment 447058 [details]
Fix names

[Antti Koivisto]

Comment on attachment 447058 [details]
Fix names

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

I don't think this code should be in style resolution. Tracking the fixed position ancestor could be more naturally tracked during render tree building (in RenderTreeUpdater) where the rest of the related code is going too. It should be really simple too, a single RenderTreeUpdater scope variable I think.

Looking at the other patches the mechanism seems to be doing content scanning synchronously and then triggering another style update. Is the idea to make some portion of this asynchronous?

> Source/WebCore/style/StyleUpdate.cpp:143
> +Element* Update::fixedPositionRoot(Element& element, const RenderStyle& style, Element* parentFixedPositionRoot)
> +{
> +    if (!m_document->shouldObserveFixedPositionRoots())
> +        return parentFixedPositionRoot;
> +
> +    if (!composedTreeAncestors(element).first() || is<HTMLBodyElement>(element))
> +        return parentFixedPositionRoot;
> +
> +    if (!isFixedPosition(style) || parentFixedPositionRoot)
> +        return parentFixedPositionRoot;
> +
> +    return &element;
> +}

This non-static Style::Update member function doesn't access the object at all (besides m_document). It could just be a static function in the caller.

The code here is bit silly in general. If you pass a non-null parentFixedPositionRoot the function just returns it, after various tests.

[Wenson Hsieh]

Comment on attachment 447058 [details]
Fix names

(In reply to Antti Koivisto from comment #8)
> Comment on attachment 447058 [details]
> Fix names
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447058&action=review
> 
> I don't think this code should be in style resolution. Tracking the fixed
> position ancestor could be more naturally tracked during render tree
> building (in RenderTreeUpdater) where the rest of the related code is going
> too. It should be really simple too, a single RenderTreeUpdater scope
> variable I think.
> 

👍🏻

Changing this so that it runs as a post-layout task, as we discussed!

[Simon Fraser (smfr)]

Note that FrameView already has:
    std::unique_ptr<WeakHashSet<RenderLayerModelObject>> m_viewportConstrainedObjects;

[Wenson Hsieh]

(In reply to Simon Fraser (smfr) from comment #10)
> Note that FrameView already has:
>     std::unique_ptr<WeakHashSet<RenderLayerModelObject>>
> m_viewportConstrainedObjects;

Thank you, that works great!

[Wenson Hsieh]

Created attachment 447385 [details]
Patch

[Wenson Hsieh]

Created attachment 447386 [details]
Patch

[Wenson Hsieh]

Created attachment 447387 [details]
Remove unneeded #include

[Wenson Hsieh]

Comment on attachment 447387 [details]
Remove unneeded #include

Thanks for the review!

[zalan]

Comment on attachment 447387 [details]
Remove unneeded #include

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

> Source/WebCore/page/ModalContainerObserver.cpp:125
> +        RefPtr element = object.element();
> +        if (!element || is<HTMLBodyElement>(*element) || element->isDocumentNode() || !m_elementsToIgnoreWhenSearching.add(*element).isNewEntry)
> +            continue;
> +
> +        for (auto& textNode : descendantsOfType<Text>(*element)) {
> +            if (!matchesSearchTerm(textNode, searchTerm))
> +                continue;
> +
> +            m_container = element.get();

Any particular reason why you traverse the DOM tree and not the render tree? (the renderer tree in general is "closer" to what ends up on the screens)

[Devin Rousso]

Comment on attachment 447387 [details]
Remove unneeded #include

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

r=me as well :)

> Source/WebCore/page/ModalContainerObserver.cpp:49
> +

Style: I'd remove this newline

> Source/WebCore/page/ModalContainerObserver.cpp:57
> +        // FIXME: Need to properly support subframes.

Is there a bug for this?

> Source/WebCore/page/ModalContainerObserver.cpp:64
> +    RefPtr frame = document.frame();

NIT: my understanding is that since `frame` is only used for simple getters (i.e. no complicated logic) it doesn't need to be ref'd

> Source/WebCore/page/ModalContainerObserver.cpp:81
> +    bool shouldSearchNode = ([&] {

NIT: Does this really need to be a lambda?

> Source/WebCore/page/ModalContainerObserver.cpp:113
> +    if (searchTerm.isNull())

NIT: you could `!searchTerm`

> Source/WebCore/page/ModalContainerObserver.cpp:118
> +        if (!element || is<HTMLBodyElement>(*element) || element->isDocumentNode() || !m_elementsToIgnoreWhenSearching.add(*element).isNewEntry)

Style: I'd split this into separate lines so that the `WeakHashSet` operation is more obvious

[Wenson Hsieh]

Comment on attachment 447387 [details]
Remove unneeded #include

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

Thanks for taking a look!

>> Source/WebCore/page/ModalContainerObserver.cpp:49
>> +
> 
> Style: I'd remove this newline

Fixed, good catch.

>> Source/WebCore/page/ModalContainerObserver.cpp:57
>> +        // FIXME: Need to properly support subframes.
> 
> Is there a bug for this?

Filed: https://bugs.webkit.org/show_bug.cgi?id=234446. I'll add a FIXME referencing it, too.

>> Source/WebCore/page/ModalContainerObserver.cpp:64
>> +    RefPtr frame = document.frame();
> 
> NIT: my understanding is that since `frame` is only used for simple getters (i.e. no complicated logic) it doesn't need to be ref'd

Sure.

>> Source/WebCore/page/ModalContainerObserver.cpp:81
>> +    bool shouldSearchNode = ([&] {
> 
> NIT: Does this really need to be a lambda?

I guess it doesn't. Will remove.

>> Source/WebCore/page/ModalContainerObserver.cpp:113
>> +    if (searchTerm.isNull())
> 
> NIT: you could `!searchTerm`

AtomString doesn't have an `operator bool()`.

>> Source/WebCore/page/ModalContainerObserver.cpp:118
>> +        if (!element || is<HTMLBodyElement>(*element) || element->isDocumentNode() || !m_elementsToIgnoreWhenSearching.add(*element).isNewEntry)
> 
> Style: I'd split this into separate lines so that the `WeakHashSet` operation is more obvious

Sure.

>> Source/WebCore/page/ModalContainerObserver.cpp:125
>> +            m_container = element.get();
> 
> Any particular reason why you traverse the DOM tree and not the render tree? (the renderer tree in general is "closer" to what ends up on the screens)

That's a good point! I suppose I can traverse `RenderText` instead of `Text` nodes here — will give that a try.

[zalan]

>NIT: Does this really need to be a lambda?
my answer would be yes, always.

[zalan]

Comment on attachment 447387 [details]
Remove unneeded #include

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

> Source/WebCore/page/ModalContainerObserver.cpp:116
> +    for (auto& object : *view.viewportConstrainedObjects()) {

btw, there are renderers. let's call them that.

[zalan]

(In reply to zalan from comment #20)
> Comment on attachment 447387 [details]
> Remove unneeded #include
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447387&action=review
> 
> > Source/WebCore/page/ModalContainerObserver.cpp:116
> > +    for (auto& object : *view.viewportConstrainedObjects()) {
> 
> btw, there are renderers. let's call them that.
these are even

[Wenson Hsieh]

Comment on attachment 447387 [details]
Remove unneeded #include

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

>>> Source/WebCore/page/ModalContainerObserver.cpp:116
>>> +    for (auto& object : *view.viewportConstrainedObjects()) {
>> 
>> btw, there are renderers. let's call them that.
> 
> these are even

True — renamed to `renderer`!

[Wenson Hsieh]

(In reply to zalan from comment #19)
> >NIT: Does this really need to be a lambda?
> my answer would be yes, always.

I'm also a fan of "inline lambdas w/ early returns"! I suppose I'll keep this one here after all.

[zalan]

(In reply to Wenson Hsieh from comment #23)
> (In reply to zalan from comment #19)
> > >NIT: Does this really need to be a lambda?
> > my answer would be yes, always.
> 
> I'm also a fan of "inline lambdas w/ early returns"! I suppose I'll keep
> this one here after all.
Yeah + great for scoping + replaces comments like // Check if we should search in this node

[Wenson Hsieh]

Created attachment 447493 [details]
For EWS

[EWS]

Committed r287217 (245380@main): <https://commits.webkit.org/245380@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447493 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86659823>