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

josepharhar · 2021-03-08T20:24:58Z

Addresses #4051

At least two implementers are interested (and none opposed):
- Chrome
- Firefox?
Tests are written and can be reviewed and commented upon at:
- TODO
Implementation bugs are filed:
- Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1185950
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1724299
- Safari: https://bugs.webkit.org/show_bug.cgi?id=228843

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

Addresses whatwg#4051

source

domenic

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

domenic

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

source

domenic

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.

annevk

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

annevk · 2021-05-10T08:59:01Z

source

@@ -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


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

done, thanks!

annevk · 2021-05-10T09:01:29Z

source

+  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>


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.

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

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?

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

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.

annevk · 2021-05-10T09:01:51Z

source

+  the following steps:</p>
+
+  <ol>
+    <li><p>Let <var>node</var> be the first node in the <span data-x="fip-active-match">active


Nit: single-space indentation.

josepharhar · 2021-05-11T17:45:14Z

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 · 2021-05-17T07:39:41Z

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

emilio · 2021-05-17T08:28:47Z

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 · 2021-05-17T17:08:14Z

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:

The page can't do anything funny with the details content which could interfere with find-in-page.
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>
 text that if searched for would open the details element, but then not match after all
</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  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:

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
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).
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.

zcorpan · 2021-05-18T12:47:27Z

source

+       <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>


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)?

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

zcorpan · 2021-05-18T13:02:27Z

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 · 2021-05-18T13:21:25Z

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 · 2021-07-21T21:08:42Z

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

emilio

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

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

source

josepharhar · 2021-07-28T20:16:12Z

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 · 2021-08-04T22:26:58Z

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 · 2021-08-04T23:08:20Z

Thanks! @emilio and/or @annevk could you approve? (I'm guessing that is needed to merge this...?)

It might be worth landing some .tentative. tests at least

I'd be happy to modify these tests to use window.find for this feature and make them WPTs:

josepharhar · 2021-08-05T16:31:15Z

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 · 2021-08-05T18:30:07Z

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 · 2021-08-05T20:41:04Z

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

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

6150b78

Addresses whatwg#4051

josepharhar mentioned this pull request Mar 8, 2021

proposal: open <details> when the user navigates to an ID inside it #4051

Closed

domenic requested changes Mar 8, 2021

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

Rewrite find-in-page section and revert rendering section

2ff7386

annevk reviewed Mar 9, 2021

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

Add ancestor details revealing algorithm

738da4b

domenic reviewed Apr 12, 2021

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

Adjust algorithms

21ee9bd

domenic reviewed May 5, 2021

View reviewed changes

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 added the agenda+ To be discussed at a triage meeting label May 5, 2021

josepharhar added 2 commits May 5, 2021 18:35

Add <var>s and s, use list for conditional

0e4b44a

Fixed after </li>

60e8e57

domenic reviewed May 6, 2021

View reviewed changes

source Outdated Show resolved Hide resolved

josepharhar added 2 commits May 6, 2021 18:36

Merge 3 months of commits from main

14f3eb5

More formatting

f07c1fb

past mentioned this pull request May 7, 2021

Upcoming HTML standard issue triage meeting on 5/6/2021 #6551

Closed

Final nit

96b233d

domenic approved these changes May 7, 2021

View reviewed changes

annevk reviewed May 10, 2021

View reviewed changes

Address comments

b2dc599

annevk added the needs tests Moving the issue forward requires someone to write tests label May 17, 2021

zcorpan reviewed May 18, 2021

View reviewed changes

Add case for open attribute already set

a3dadcf

josepharhar mentioned this pull request Jul 8, 2021

hidden=until-found and auto-expanding <details> WICG/scroll-to-text-fragment#173

Closed

domenic force-pushed the main branch from 0947339 to 1b5099f Compare July 27, 2021 17:07

emilio reviewed Jul 28, 2021

View reviewed changes

source Show resolved Hide resolved

Add flat tree

295912a

josepharhar mentioned this pull request Aug 5, 2021

Support testing find-in-page web-platform-tests/wpt#29915

Open

Editorial tweaks

4054db9

domenic force-pushed the details branch from 559cc66 to 4054db9 Compare August 5, 2021 18:29

past mentioned this pull request Aug 5, 2021

Upcoming HTML standard issue triage meeting on 8/5/2021 #6884

Closed

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 merged commit 0050553 into whatwg:main Aug 6, 2021

This was referenced Sep 16, 2021

Auto-expanding details elements w3ctag/design-reviews#677

Closed

Auto-expanding details elements mozilla/standards-positions#578

Closed

westonruter mentioned this pull request Oct 13, 2021

Add Accordion block example using bento-accordion swissspidy/gutenberg-bento#10

Open

josepharhar mentioned this pull request Dec 8, 2021

[cssom-view] Proposal: add Element.isVisible[toDocument] to detect if element is visible in the document. w3c/csswg-drafts#6850

Closed

josepharhar mentioned this pull request Jan 7, 2022

Add hidden=until-found HTML attribute and beforematch event #7475

Merged

3 tasks

stevehartdata mentioned this pull request Mar 2, 2022

Odd in-page search behavior with recent Chromium and Chrome haskell/haddock#1451

Closed

westonruter mentioned this pull request Mar 3, 2022

New block: Accordion block WordPress/gutenberg#21584

Open

lpw25 mentioned this pull request Mar 11, 2022

Stop using <details> ocaml/odoc#92

Closed

joelostblom mentioned this pull request Dec 8, 2022

Improve encoding section vega/altair#2735

Merged

josepharhar mentioned this pull request Mar 6, 2023

Should popover use the flat tree #8970

Open

gibson042 mentioned this pull request Oct 24, 2023

Stage 1 proposals should be included in the README tc39/proposals#489

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

josepharhar commented Mar 8, 2021 •

edited by pr-preview bot

Loading

domenic left a comment

domenic left a comment

domenic left a comment

annevk left a comment

annevk May 10, 2021

josepharhar May 11, 2021

annevk May 10, 2021

domenic May 10, 2021 •

edited

Loading

josepharhar May 11, 2021

zcorpan Jun 29, 2021

josepharhar Jun 30, 2021

annevk May 10, 2021

josepharhar May 11, 2021

josepharhar commented May 11, 2021 •

edited

Loading

annevk commented May 17, 2021

emilio commented May 17, 2021

josepharhar commented May 17, 2021 •

edited

Loading

zcorpan May 18, 2021

josepharhar May 19, 2021

zcorpan commented May 18, 2021

zcorpan commented May 18, 2021

josepharhar commented Jul 21, 2021

emilio left a comment

josepharhar commented Jul 28, 2021

emilio commented Aug 4, 2021

josepharhar commented Aug 4, 2021

josepharhar commented Aug 5, 2021

domenic commented Aug 5, 2021

josepharhar commented Aug 5, 2021

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

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

Conversation

josepharhar commented Mar 8, 2021 • edited by pr-preview bot Loading

domenic left a comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josepharhar commented May 11, 2021 • edited Loading

annevk commented May 17, 2021

emilio commented May 17, 2021

josepharhar commented May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcorpan commented May 18, 2021

zcorpan commented May 18, 2021

josepharhar commented Jul 21, 2021

emilio left a comment

Choose a reason for hiding this comment

josepharhar commented Jul 28, 2021

emilio commented Aug 4, 2021

josepharhar commented Aug 4, 2021

josepharhar commented Aug 5, 2021

domenic commented Aug 5, 2021

josepharhar commented Aug 5, 2021

josepharhar commented Mar 8, 2021 •

edited by pr-preview bot

Loading

domenic May 10, 2021 •

edited

Loading

josepharhar commented May 11, 2021 •

edited

Loading

josepharhar commented May 17, 2021 •

edited

Loading