-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal: beforematch event and hidden=until-found attribute #6040
Comments
FWIW, I'm very excited and supportive of this, and happy to help.
This looks pretty nice. You'll need to do a bit of extra work to track what iteration of "update-the-rendering" you're in; probably some kind of per-event-loop state variable with three values? Also, instead of calling scrollIntoView() directly, you'll want to call https://drafts.csswg.org/cssom-view/#scroll-an-element-into-view |
Talking as the developer of the Link to Text Fragment extension (60K+ users), it's a top feature request of users to link to expandable items (e.g., accordions), so adding a platform capability that would allow sites to detect when and react to when a match is about to happen is highly desirable. |
<3
The way I implemented this was with a single cancellable callback variable for both of the two spots where we wait for the next update-the-rendering. This way, if another find-in-page request comes in, we will cancel the callback by replacing it with a new callback. The callback itself has the next method which will run (step 1 vs step 2) as well as a bunch of state to call the methods with, including the DOM range to scroll to as well as a flag to say whether the match originally had content-visibility:hidden-matchable in an ancestor when we first found it. I see now that the spec steps I listed don't really cover this. What is a per-event-loop state variable? Are there any examples of this or anything else that sounds similar to the behavior I described?
Thanks! Would we be "calling" or "running" this step?
Awesome! Do you have any links to these feature requests? |
Coming here from the TAG review. We were curious about what the timing constraints are on developers for the requirement to show content with a match in order to continue having the feature enabled on the page. The spec edits above make it look like every single beforematch event must result in content being shown, even if another event happens very quickly afterwards (say, because the user was typing a word or phrase and the find in page algorithm eagerly matched a partial word before they finished typing). Will there be any throttling on the event being fired, to avoid jittering as find in page matches "L", then "Lo", then, "Lor", etc? Or have I misunderstood something? |
By this I meant declaring something in the spec like "Each event loop has an X, initially 0." But perhaps sticking closer to your implementation structure might be good. In which case it would be something like "Each event loop has a beforematch steps, which is null or a series of steps" plus more "Each event loop has..." for the other state you mention. A similar example where the spec sets steps and later runs then would be https://html.spec.whatwg.org/#lazy-load-resumption-steps .
We use them interchangeably, but I think we'd usually write "running" in spec prose. |
Yes, every beforematch event handler must reveal the content in this case. With regards to timing, this has been an issue since before the privacy mitigations due to the fact that we still have to choose when to make the browser scroll. We added an async step (waiting for another update-the-rendering) in response to early developer feedback since their beforematch handler didn't synchronously reveal the content.
If the "L", "Lo", and "Lor" are all part of the same text then the beforematch event will only be fired on the first "L". If they are all parts of separate text, then a beforematch event would be fired on each one and each one would need to be revealed. |
That sounds less than ideal, then, if the user is typing "Lorem ipsum" but they only get a match on "L"? Have I missed something?
Basically wait for XX msec before firing the event, and cancel it if the query text has changed in the meantime. You could potentially combine that with a longer timeout for shorter text (because "L" will have more matches/noise than "Lo", etc.) |
Yeah so I suppose that if the user is searching for something, but in the process of typing in their query they get an active match for some text which isn't the final query they want, and that active match is hidden and gets revealed, that would be less than ideal: <div style="content-visibility: hidden-matchable">
Lots of kittens
</div>
<div>
Lorem ipsum
</div> Is this the case you're talking about?
This sounds similar to @kenchris's comment from the TAG review:
Safari does this for all usage of find-in-page, and the delay is pretty visible and I could imagine that some users might not like this behavior for find-in-page since it may seem less responsive. |
Yes, exactly that. Or even if "Lorem Ipsum" is also in a different You could also explore having a grace period before locking the page out of the Seems like it would be sensible to do some experiments to figure out what the best user experience is, in any case. |
Another option might be to do something similar to what the Scroll To Text Fragment API did, and only match on word boundaries. Was that explored in the security/privacy review? |
This seems like the current behavior of find-in-page (more or less, in Chromium at least): each character typed will instantly find the next match that matches the whole query and scroll to it. The scroll can also be observed by script and it can react in any way it wants to after the scroll happens. The added feature with this proposal is that there's an interleaved event which is able to modify style/dom before the scroll occurs.
If I understand this correctly, then it seems like it would break existing find-in-page feature. Currently the match happens if any substring of the word (or multiple words) matches the query. If we only update the match on word boundaries, it seems like it would break use-cases. If we only fire beforematch on word boundaries, it seems like it would be hard to explain and also limit use-cases
I think the distinction here is that beforematch fires on hidden-matchable content to allow script to make the match visible; once visible, the find-in-page finds the text as before without the need for beforematch. I'd just like to clarify my understanding of what happens with a specific example. Let me know if I'm correct in understanding this. Consider: <div class=spacer></div>
<div id=foo style="content-visibility: hidden-matchable">Lorem Ipsum</div>
<div class=spacer></div>
<div>Lord of the Rings</div>
<script>
foo.addEventHandler("beforematch", () => { foo.style = "" });
</script> Consider the user taking the following steps:
The sequence of events is then the following.
|
I agree that trying to avoid locking the page out of beforematch is important. The current requestAnimationFrame delay in the algorithm should help prevent it from happening. I'm hesitant to have an actual timer, since pages shouldn't have an actual timed delay before revealing the content, right...? The current algorithm works for Earlier today I was toying with the idea of checking to see if the match has been revealed and trying to scroll both before as well as after the requestAnimationFrame in order to avoid the situation where the beforematch handler reveals the content and then something else in the page decides to hide it again or remove it from the dom before the browser checks after the requestAnimationFrame.
Yeah, I think this could be limiting and counter-intuitive, and I don't understand how it would be a useful mitigation for beforematch. We are concerned with the page being able to build out what the user is searching for iteratively, in which case all matches would be for entire words.
After thinking about this some more, I think that "rapid changes" already describes the status quo: by having instant synchronous scrolling to whatever match you type into find-in-page today, chances are that the page is going to be jumping around as it scrolls to the match for whatever character you just typed in. If the page reveals content in response to each time that happens, is it really different from the status quo? Is it really that bad?
Yes, this is the behavior that I'm proposing. |
Ahhh that's what I was missing. Thanks for the worked example showing this.
I think it has the potential to be more annoying than the status quo, because it's expanding parts of the page that you're not interested in (and thus triggering layout and content moving around) rather than just scrolling to them. I'm imagining a mobile wikipedia page potentially expanding 2-3 sections before the one you're actually interested in. Then you try to scroll back up to where you were before you did a search, but there's all this extra content there. Could we do an experiment to find out?
I'm speculating that pages may want to avoid the annoying behaviour described above by, say, waiting for either a pause in typing or for a minimum number of characters matched, before showing the match. Forcing pages to show content for every single |
How does "The beforematch event will only be fired on text hidden with the content-visibility: hidden-matchable CSS property." help with anything? The page could paint all the text in a canvas, but have the DOM in hidden-matchable. |
Thanks for the questions at TPAC @smfr and @emilio It sounds like there were two issues brought up:
The intent of beforematch is to give the page a signal when a match has been found in otherwise hidden content. We would like to prevent firing the signal on visible content, since it allows the page to annotate all content with beforematch handlers and track the user’s find-in-page activity. Such information is already available at low resolution (ie scroll offset of the match), but beforematch would be able to provide more detailed information. For this reason, we prefer to focus the use-cases on hidden content that must be revealed once the match has been found. I agree that we want to avoid downgrading the page as much as we can. I think we could look for a beforematch event listener before firing the event, and if there isn’t one, then we could avoid firing the event or prevent downgrading from occurring, since in this case the page is not capable of tracking or recording any new information provided by the beforematch event.
Since content that is occluded will still be find-in-pageable even today, yet will yield a poor/broken user experience if found, the attack surface is not increased for such content due to the feature, and developers are already dis-incented to do such things because it results in a poor UX. Although you can listen to scroll events, providing a higher precision and easier way for the page to snoop find-in-page via beforematch without the content ever being drawn on the screen (content-visibility:hidden-matchable) seemed problematic. In other words, the mitigations we are proposing for the beforematch event are meant to make it difficult to use it to track the user’s find-in-page activity, but easy to use to make hidden content visible. The tracking portion of this is largely possible without beforematch (using scroll offsets). This means that with our mitigations, beforematch is not an easier tool for getting this information. |
Also, see this comment on the CSSWG issue, clarifying that we propose to adjust the spec for text fragments, and not just find-in-page. |
Domenic suggested this here: whatwg/html#6040 (comment)
In the parallel csswg issue, it was brought up that we should also fire the event on element fragments. this means that we’ll need to modify the Navigating to a Fragment Algorithm to fire the beforematch event at the appropriate time. |
Point was, there shouldn't be a difference in behavior for a fragment targetted via text vs. ID; if someone is linking into some content using a fragID, they should get the same effect whatever type of fragID it is. |
The tag review has been approved. Regarding support for element fragments, I think they should be supported but with slightly different timing with regards to scrolling compared to find-in-page and ScrollToTextFragment. I documented the constraints and behavior in the explainer here. I anticipate that this feature will be approved by the CSSWG soon. Is there anything I can to do push this along? Can I open an HTML spec PR for this feature? |
My understanding is the CSSWG operates by consensus, so if the feature is approved, then you'll have multi-implementer interest in the sense HTML requires. So yeah, a HTML spec pull request would be a great next step. |
Based on feedback from CSSWG, I am now proposing that this feature will be an HTML attribute instead of a CSS property, as suggested here. Instead of the The user-agent stylesheet for [hidden=until-found] { content-visibility: hidden }
[hidden]:not([hidden=until-found]) { display: none } Overloading the existing Here is a quick example of how this would be used: <div id=sectionheader></div>
<div hidden=until-found id=collapsible>
hidden text
</div>
<script>
let revealed = false;
sectionheader.textContent = 'section (hidden)';
collapsible.onbeforematch = () => {
revealed = true;
sectionheader.textContent = 'section (revealed)';
};
sectionheader.onclick = () => {
if (revealed) {
revealed = false;
collapsible.setAttribute('hidden', 'until-found');
sectionheader.textContent = 'section (hidden)';
} else {
revealed = true;
sectionheader.textContent = 'section (revealed)';
}
};
</script> |
You'll also need to adjust One thing to look out for is that we don't create another event with mutation-event timing as we do more of these user-action-mutates-the-tree features. |
In the CSS issue @smaug---- raised a question about the |
I wrote a comment in the CSSWG thread about how searchability is the desired new primitive. I think I'm also not convinced that searchability belongs in HTML rather than CSS. Wouldn't a CSS property have the same advantage of not requiring JS event handling to make the feature just work by default, and require the exact same amount of computation as an HTML attribute? (Or even less, since for HTML you have to re-add <p style="user-search: always-searchable; display: none;">Hidden but searchable</p>
<!-- CSS property (hypothetical, illustrative only):
1. Load page, resolve styles, build text index (including within `user-search: always-searchable`).
2. User searches for "Hidden".
3. Find match in <p> with `user-search: always-searchable; display: none;`.
4. Unhide it automatically and re-render. -->
<p hidden="until-match">Hidden but searchable</p>
<!-- HTML attribute:
1. Load page, resolve styles, build text index (including within `hidden="until-match"`).
2. User searches for "Hidden".
3. Find match in <p> with `hidden="until-match"`.
4. Unhide it automatically and re-render. --> A CSS property would also allow concisely controlling the searchability of many elements at once using the power of selectors, while an HTML attribute would require repetitively adding an attribute on each element. |
Can you elaborate on how the "unhiding" would happen automatically in the CSS case? The problem is that the visibility (or lack thereof) can come from some class somewhere, and it's unclear to me how one would "make it visible" automatically. And even if this is possible, then how would one hide the content again? As for the attribute version, the proposal is that it would come with some automatic hiding, but that would be specified as the UA stylesheet, so the author would be able to override the default behavior |
Sorry, I was oversimplifying and meant "attempt to unhide it" instead. In both the HTML attribute and CSS property cases, isn't it equally true that visibility can derive from elsewhere? The browser still needs to compute styles fully to decide whether the matched hidden content can actually be shown, right? And isn't it CSS that ultimately sets the invisibility of <div hidden> or <div style="display: none;"> or <div class="hide"> or `div > p { display: none; }` etc.
<p hidden="until-match">Hidden but searchable</p>
</div> I'm now also wondering, is it really that desirable for this feature to work by default (i.e. not require any JS)? |
Yes, you're right that the visibility can still come from a number of properties. The nice thing about an attribute is that it affects this one element (and only this one element). So, in response to a find-in-page match, the UA can remove the attribute. This accomplishes one of two things: either this clears sufficient style making content visible and thus the match can be shown to the user, or if the content is still hidden then it makes that content no longer searchable (since it no longer has With CSS, the UA can remove the inline style since that only affects the one element, but there is no elegant way to remove other rules that would affect the element, such as As for the question of whether this should work without JS: it's definitely a nice-to-have. The previous proposal of content-visibility: hidden-matchable did not have any automatic ways of revealing content and relied exclusively on beforematch and JS to reveal content. With an attribute, we can unlock more cases that can work without script (e.g. the |
This plan looks good to me. To summarize and reiterate the tricky parts:
|
I have an updated explainer here: https://github.com/WICG/display-locking/blob/main/explainers/hidden-content-explainer.md |
I don't fully understand this concern if we are just talking about the attribute itself and not the reflected If the attribute isn't present, the element isn't hidden, like it is today.
So there's no way we can support
I'm inclined to choose this option, but maybe if people really want to avoid |
I think for the content attribute there's no concern. I was just mentioning that the spec prose will be tricky. I haven't looked at the PR yet to see how that went; will comment over there :).
I guess we could; we would just abandon the idea of
One of the big benefits of adding properties instead of forcing the use of setAttribute() is that some frameworks, like React, are property-centric. |
I would like to propose a new DOM event called “beforematch” (TAG review).
The beforematch event would be fired in response to find-in-page and ScrollToTextFragment on content which was hidden by a new CSS property,
content-visibility: hidden-matchable
.When find-in-page or ScrollToTextFragment tries to scroll to a match which is within the hidden-matchable subtree, the beforematch event will fire on the element with the hidden-matchable style before the browser scrolls, allowing the page to change the style to reveal the content so the browser can scroll to it and show it to the user.
An example usage of this feature would be to search the content of collapsed sections of mobile wikipedia, which are not currently searchable.
Sketch of edits to spec
The find-in-page algorithm will be modified as shown in the github repo - the step where the active match is “scrolled into view” will be replaced with:
content-visibility: hidden-matchable
property.content-visibility: hidden-matchable
orcontent-visibility: hidden
properties.display: none
property.visibility
property is notvisible
.Security/privacy impact
Leaking information to the page about what the user is searching for with find-in-page is a big concern with this feature, but there is already some information which find-in-page necessarily exposes to the page. To limit the information exposed by this feature, we have designed two mitigations:
content-visibility: hidden-matchable
CSS property. This prevents the page from attaching beforematch event listeners to all of the visible content in the page and snooping on what the user is searching for.content-visibility
CSS property is nothidden
orhidden-matchable
.display
CSS property is notdisplay: none
.visibility
CSS property isvisibility: visible
.Open questions
Right now, I am proposing that the beforematch event is only fired on text with the
content-visibility: hidden-matchable
property in the ancestor chain. However, we have some other options:content-visibility: hidden-matchable
.Cons:
content-visibility: hidden
properties, depending on how the spec is phrased.content-visibility: hidden-matchable
andvisibility: hidden
.Cons:
visibility: hidden
content out there on the web, and if we started firing beforematch on all of it, the page would be required to reveal it or else it would be locked out of using beforematch as described in mitigation 2 above. This means that many pages will get locked out of beforematch all the time by default, which would not be good.content-visibility: hidden
andvisibility: hidden
if they also have another new CSS property, let’s call itsearchable: true
.Cons:
visibility: hidden
case, you’d also have to add even more CSS to get it to stop taking up space in the layout if you want it to function likecontent-visibility: hidden
.The text was updated successfully, but these errors were encountered: