Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

beforematch: what to do when the element is no longer matching after callback. #150

Closed
vmpstr opened this issue Apr 2, 2020 · 3 comments

Comments

@vmpstr
Copy link
Collaborator

vmpstr commented Apr 2, 2020

Beforematch puts in a script callback before we scroll to whatever caused us to fire the signal (scroll-to-text, find-in-page, or fragment link navigation).

The question is what should we do if the conditions that caused us to find the match no longer hold after beforematch runs.

For example, in find-in-page case, the callback can:

  • Add display: none to some ancestor
  • Change text
  • Move the node before/after other matches

We need to know how to handle all of those. Hopefully, whatever the decision, we can apply to other situations that we might not have thought of yet.

/cc @josepharhar @chrishtr @domenic

@josepharhar
Copy link
Collaborator

Some thoughts:

  • find-in-page, ScrollToTextFragment, and ElementFragment each have their way of deciding to match something or not, maybe it would make sense to run that a second time after the beforematch event, perhaps based on what changes in css properties we see.
  • If an element is display locked, we should fire the beforematch event on it. If the displaylock isn't removed, then it is an important for that use case that we don't scroll to it, right? In find-in-page, ScrollToTextFragment, and ElementFragment is it important to run another search again in this case?

josepharhar added a commit to josepharhar/display-locking that referenced this issue Apr 24, 2020
I made this to address this code review comment:
https://chromium-review.googlesource.com/c/chromium/src/+/2068873/21#message-93c7132faae9655a9a22e6b38ed73ff90315d6cb

This information is very relevant to this bug where we will try to
decide on what the scrolling behavior should be:
WICG#150
@josepharhar
Copy link
Collaborator

I documented a bunch of current scrolling behavior here: #154

Another thought brought up in the PR: When we choose to scroll to a different element due to DOM/style changes in the beforematch handler, should said different element also get a beforematch event?
My gut feeling is yes, we should fire two events. At the moment, ScrollToTextFragment doesn't fire a second beforematch event when the target element changes due to the original target element being removed from the DOM.

@josepharhar
Copy link
Collaborator

Some more thoughts from @vmpstr: We should continue to fire events when the element we are trying to scroll to is changing, but we should limit it to like 10 to prevent infinite loops

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants