Skip to content
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

Expand <details> for find-in-page and element fragments #6466

Merged
merged 18 commits into from
Aug 6, 2021

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Mar 8, 2021

Addresses #4051


/browsing-the-web.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/interaction.html ( diff )
/interactive-elements.html ( diff )
/references.html ( diff )
/rendering.html ( diff )

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty solid now. Thanks for reminding me about how details is specified in terms of shadow trees already.

In particular, the way in which it's specified ("The details element's second slot is expected to be removed from the rendering when the details element does not have an open attribute.") means that what you've done here is just about as rigorous as that section already is, so I'm happy.

We've got some editorial work to do on the revealing algorithm, starting refactoring from gotos to loops, but that's relatively easy. I'll resolve our previous discussions as I think we're on a clear road to success here.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, but there's a number of style issues to be finished.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic domenic added the agenda+ To be discussed at a triage meeting label May 5, 2021
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @annevk might want to take a final look.

Now we just need tests. The fragment navigation case is definitely testable using web platform tests.

The find-in-page case, I'm not so sure. Maybe it's testable using window.find()? But that's nonstandard so I'm not sure what to think about that. It'd be worth trying to add a tentative test using window.find() and see if it works; if not then oh well.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't have find-in-page itself defined in enough detail to know what kind of tasks it queues? If we ever add an API there it would be good to ensure the timing between that and these events here are solid.

source Show resolved Hide resolved
source Outdated
@@ -76140,6 +76167,25 @@ body { display:none }
can navigate through the <span data-x="fip-matches">matches</span> by advancing the <span
data-x="fip-active-match">active match</span> using the <span>find-in-page interface</span>.</p>

<p>When find-in-page begins searching for matches, all <code>details</code> elements in the page
which are not <code data-x="attr-details-open">open</code> should have their second slot be added
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "which do not have their open attribute set"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thanks!

source Outdated
to the rendering without modifying the <code data-x="attr-details-open">open</code> attribute.
After find-in-page finishes searching for matches, those <code>details</code> elements should have
their second slot be removed from the rendering again. This entire process must happen
synchronously (and so is not observable to users or to author code).</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to use "act as if" here as the exact implementation strategy does not matter, as long as you can search through them. That might also allow for removal of the final sentence.

Copy link
Member

@domenic domenic May 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for as-if, but I think the final sentence is still pretty important for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added "act as if", and I also said that it is to make sure the slot can be searched by find-in-page. How does it look?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see "act as if" in the spec text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added "act as if" in b2dc599 and removed it in 1717601 when I added content-visibility:hidden. We don't need to be vague by using the "act as if" phrase now that we are speccing it to explicitly be content-visibility:hidden.

the following steps:</p>

<ol>
<li><p>Let <var>node</var> be the first node in the <span data-x="fip-active-match">active
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: single-space indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@josepharhar
Copy link
Contributor Author

josepharhar commented May 11, 2021

I guess we don't have find-in-page itself defined in enough detail to know what kind of tasks it queues? If we ever add an API there it would be good to ensure the timing between that and these events here are solid.

The timing is already important for scrolling - the details revealing algorithm must run before find-in-page scrolls to the active match.
Maybe I should add scrolling after the details revealing...?

@annevk
Copy link
Member

annevk commented May 17, 2021

Yeah. I wonder if @zcorpan or @whatwg/css has insights on that as CSSOM View does attempt to define scrolling.

@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label May 17, 2021
@emilio
Copy link
Contributor

emilio commented May 17, 2021

So I'm a bit concerned about the way this is defined, because the "acts as" implies that you actually need to create boxes for the <details>s contents (to handle display: none, visibility, etc), but make that have no side effects (which seems hard because creating boxes can trigger e.g., CSS animations, which trigger DOM events eventually).

I would've thought that the way we'd define this would allow for some false positives. The way I would go about implementing something like this would be to do a search on the DOM, and if that might match then open the element. It'd open the element in cases where there might be no match like:

<details>
  <span hidden>text that if searched for would open the details element, but then not match after all</span>
</details>

The alternative to that would be opening all details elements on the page, but that seems potentially more annoying to users... Curious, how were you planning on implementing this @josepharhar? (Just in case I've missed something obvious or what not)

@josepharhar
Copy link
Contributor Author

josepharhar commented May 17, 2021

So I'm a bit concerned about the way this is defined, because the "acts as" implies that you actually need to create boxes for the <details>s contents (to handle display: none, visibility, etc), but make that have no side effects (which seems hard because creating boxes can trigger e.g., CSS animations, which trigger DOM events eventually).

Thanks for pointing out this event case!
Yes, we actually need to create boxes when searching for details contents, since text finding requires that layout is run. The way I'm implementing this in chrome with content-visibility should hopefully make it re-use boxes so it's less expensive though.
As you pointed out, it sounds like this will trigger DOM events eventually, which isn't great. But as long as find-in-page renders-as-if-open, find-in-page searches, and then closes again before any events are fired, I think we are in a pretty good state:

  1. The page can't do anything funny with the details content which could interfere with find-in-page.
  2. With regards to privacy, the delayed timing makes this less powerful than the existing find-in-page privacy issues like synchronous scroll events.

I would've thought that the way we'd define this would allow for some false positives. The way I would go about implementing something like this would be to do a search on the DOM, and if that might match then open the element. It'd open the element in cases where there might be no match like:

<details>
  <span hidden>text that if searched for would open the details element, but then not match after all</span>
</details>

I was also very disappointed when working on this feature when I found out that we have to run layout in order to do searching instead of just relying on the DOM. However, in the <span hidden> case you described, style and layout will run including the hidden attribute, and the hidden text will not be searched, so I think we are OK here. The whole "render as-if" thing just affects the styles used on the details element's content slot, at least in chrome and safari (I couldn't figure out how details hiding is implemented in firefox).

Curious, how were you planning on implementing this @josepharhar?

Since you asked, I'll explain everything :)

The way I'm implementing this in chrome does three things with regards to find-in-page:

  1. Changes the styles used on the details element's content slot:
    • Before this change:
      • open: display: contents
      • closed: display: none
    • After this change:
      • open: display: contents
      • closed and render-as-if-open: display: block; content-visibility: hidden
  2. Makes find-in-page "activate" the content-visibility:hidden display locks on all details content slots while it is searching. This doesn't affect the style, but makes it render as if it were visible. This basically re-uses all of the code from my content-visibility: hidden-matchable proposal, but instead of using a separate CSS property, I'm manually configuring the display locks on the slots to act as if they were just like hidden-matchable. I'll have to make this behavior more concrete when speccing the hidden=until-found attribute, but with regards to this spec change, it's just an internal performance optimization since incremental searches should re-use the same generated layout objects (I think).
  3. Actually opens all ancestor details elements of the active match right before the active match is scrolled into view, just like this spec PR says to.

source Outdated
Comment on lines 57116 to 57118
<li><p><span data-x="concept-element-attributes-set-value">Set</span> the <code
data-x="attr-details-open">open</code> attribute on <var>currentNode</var> to the empty
string.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better for this step to do nothing if the open attribute is already set (the value could be something other than the empty string)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I added some text in this sentence to account for this.

@zcorpan
Copy link
Member

zcorpan commented May 18, 2021

CSSOM View says this for scrolling: https://drafts.csswg.org/cssom-view/#scrolling-events

and "run the scroll steps" is called from https://html.spec.whatwg.org/multipage/webappapis.html#update-the-rendering

The effect of that is only about when and where to fire scroll events. How scrolling works more generally is still UA-defined as far as I know.

@zcorpan
Copy link
Member

zcorpan commented May 18, 2021

And https://drafts.csswg.org/cssom-view/#scroll-an-element-into-view which is used by scrollIntoView(), focus(), scrollPathIntoView() and scrolling to fragments.

https://html.spec.whatwg.org/multipage/interaction.html#find-in-page says, only as a non-normative statement of fact, "It is highlighted and scrolled into view." -- the new algorithm added in this PR could formalize the scrolling into view for find-in-page.

The sequential focus navigation section doesn't say to scroll into view, only running the focusing steps (which also don't say to scroll into view, as far as I can tell).

@josepharhar
Copy link
Contributor Author

@emilio do you have any thoughts on my last comments? Is there anything left?

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable, modulo the bit below which I think needs fixing...

It seems this should be testable at least with window.find()?

source Show resolved Hide resolved
@josepharhar
Copy link
Contributor Author

It seems this should be testable at least with window.find()?

Supporting window.find() for hidden=until-found/content-visibility:hidden-matchable (and for this feature) was something that never fully got resolved...
It didn't seem worth doing since window.find() is still unspecced, except that it could be used to test these features like you just pointed out.
I haven't implemented window.find() for this in chrome yet, but it wouldn't be hard for me to.
I feel like if we dont spec window.find(), then we shouldn't make WPTs that test this feature with it, right...? Do you agree? Do you think that we should also spec window.find() in order to test this? If so, would it be OK to do as a follow up?

@emilio
Copy link
Contributor

emilio commented Aug 4, 2021

It might be worth landing some .tentative. tests at least. But yeah I guess follow-up for tests / spec work on find() is fair.

@josepharhar
Copy link
Contributor Author

We just talked about also testing the use of content-visibility inside <details>, which I have a test for here. It was put in WPT and then taken out because my patch was reverted, but it will be back in the same spot very soon.

@domenic
Copy link
Member

domenic commented Aug 5, 2021

I pushed a commit with some minor editorial tweaks., and added a note explaining the style attribute thing. Would you be able to take a final look at that commit, @josepharhar, to confirm my changes were good?

After that, what remains is to file browser bugs, then we can merge.

@josepharhar
Copy link
Contributor Author

I pushed a commit with some minor editorial tweaks., and added a note explaining the style attribute thing. Would you be able to take a final look at that commit, @josepharhar, to confirm my changes were good?

Thanks, looks great!

After that, what remains is to file browser bugs, then we can merge.

Here is a WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=228843
And here is a Firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1724299
And here is my chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1185950

@domenic domenic added addition/proposal New features or enhancements and removed needs tests Moving the issue forward requires someone to write tests agenda+ To be discussed at a triage meeting labels Aug 6, 2021
@domenic domenic merged commit 0050553 into whatwg:main Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements
Development

Successfully merging this pull request may close these issues.

6 participants