Bug 234214

Summary: Add support for detecting modal containers that contain a given search term
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: Layout and RenderingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, changseok, esprehn+autocc, ews-watchlist, glenn, hi, kangil.han, koivisto, kondapallykalyan, pdr, simon.fraser, thorton, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 234216    
Attachments:
Description Flags
Patch
none
Slightly simpler approach
none
Minor style tweaks
none
Fix names
none
Patch
none
Patch
none
Remove unneeded #include
none
For EWS ews-feeder: commit-queue-

Wenson Hsieh
Reported 2021-12-12 13:59:02 PST
.
Attachments
Patch (11.31 KB, patch)
2021-12-12 14:44 PST, Wenson Hsieh
no flags
Slightly simpler approach (10.46 KB, patch)
2021-12-12 21:43 PST, Wenson Hsieh
no flags
Minor style tweaks (10.52 KB, patch)
2021-12-13 09:52 PST, Wenson Hsieh
no flags
Fix names (10.28 KB, patch)
2021-12-13 13:50 PST, Wenson Hsieh
no flags
Patch (20.21 KB, patch)
2021-12-16 12:39 PST, Wenson Hsieh
no flags
Patch (20.31 KB, patch)
2021-12-16 12:41 PST, Wenson Hsieh
no flags
Remove unneeded #include (19.74 KB, patch)
2021-12-16 12:56 PST, Wenson Hsieh
no flags
For EWS (19.85 KB, patch)
2021-12-17 16:34 PST, Wenson Hsieh
ews-feeder: commit-queue-
Wenson Hsieh
Comment 1 2021-12-12 14:44:09 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 2 2021-12-12 21:43:37 PST
Created attachment 446964 [details] Slightly simpler approach
Wenson Hsieh
Comment 3 2021-12-13 07:37:55 PST
Comment on attachment 446964 [details] Slightly simpler approach The webrtc and XHR test failures do not appear to be related.
Wenson Hsieh
Comment 4 2021-12-13 09:52:08 PST
Created attachment 447020 [details] Minor style tweaks
Simon Fraser (smfr)
Comment 5 2021-12-13 13:29:09 PST
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
Comment 6 2021-12-13 13:36:19 PST
(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
Comment 7 2021-12-13 13:50:44 PST
Created attachment 447058 [details] Fix names
Antti Koivisto
Comment 8 2021-12-16 04:11:33 PST
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 9 2021-12-16 10:26:11 PST
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)
Comment 10 2021-12-16 11:00:10 PST
Note that FrameView already has: std::unique_ptr<WeakHashSet<RenderLayerModelObject>> m_viewportConstrainedObjects;
Wenson Hsieh
Comment 11 2021-12-16 11:12:57 PST
(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
Comment 12 2021-12-16 12:39:21 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 13 2021-12-16 12:41:21 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 14 2021-12-16 12:56:14 PST
Created attachment 447387 [details] Remove unneeded #include
Wenson Hsieh
Comment 15 2021-12-17 14:53:25 PST
Comment on attachment 447387 [details] Remove unneeded #include Thanks for the review!
zalan
Comment 16 2021-12-17 14:57:33 PST
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 17 2021-12-17 14:58:43 PST
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 18 2021-12-17 15:24:07 PST
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
Comment 19 2021-12-17 15:41:59 PST
>NIT: Does this really need to be a lambda? my answer would be yes, always.
zalan
Comment 20 2021-12-17 15:43:06 PST
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
Comment 21 2021-12-17 15:43:30 PST
(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 22 2021-12-17 15:45:54 PST
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
Comment 23 2021-12-17 15:47:45 PST
(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
Comment 24 2021-12-17 15:59:56 PST
(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
Comment 25 2021-12-17 16:34:07 PST
EWS
Comment 26 2021-12-17 19:07:57 PST
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
Comment 27 2021-12-17 19:08:18 PST
Note You need to log in before you can comment on or make changes to this bug.