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

Doing find-in-page for text in GitHub PR/issue pages doesn’t cause expected auto-expansion of closed “details” elements that have matches #2592

Closed
sideshowbarker opened this issue Mar 21, 2024 · 2 comments · Fixed by #2611 · May be fixed by #2624

Comments

@sideshowbarker
Copy link

sideshowbarker commented Mar 21, 2024

Describe the bug
Unexpectedly, doing find-in-page in current Chrome for a given string in GitHub PR/issue pages that contain comments with details elements doesn’t cause auto-expansion of any closed details elements that have matches for the given string.

To Reproduce
Steps to reproduce the behavior:

  1. Go to Add popover=hint whatwg/html#9778 in current Chrome.
  2. Do a find-in-page for the string “web service” — which occurs multiple times in the contents of the More details element in the issue description.
  3. Notice that no match is found.
  4. Open the More details element in the issue description (by clicking the disclosure triangle).
  5. Do a find-in-page for the string “web service” with that More details element open.
  6. Notice that multiple matches are found .

Expected behavior
With current Chrome, when doing find-in-page in GitHub PR/issue descriptions for some particular string , it’s expected that any closed details elements whose contents have matches for that string will auto-expand to reveal the matches.

That’s the expected behavior because it’s what’s required in the current HTML spec:

When find-in-page begins searching for matches, all details elements in the page which do not have their open attribute set should have the skipped contents of their second slot become accessible, without modifying the open attribute, in order to make find-in-page able to search through it.

…and that’s what current Chrome supports — and what Safari will also have support for in the near future.

Desktop (please complete the following information):

  • OS: macOS
  • Browser: Chrome
  • Version: 123

Additional context
The cause appears to be this:

css/src/base/base.scss

Lines 95 to 106 in 8673252

details {
summary {
cursor: pointer;
}
&:not([open]) {
// Set details content hidden by default for browsers that don't do this
> *:not(summary) {
display: none !important;
}
}
}

That is, specifically, the styling that sets display: none !important on details contents.

I can understand why that was added at the time (~6 years ago) — but the details element is now well-supported in browsers, so that styling no longer seems useful.

But anyway, that styling as-is now is preventing users from being able to benefit from the details-auto-expand feature when searching for strings in details elements in GitHub PRs and issues — which seems not great, since people (and bots) are using details in GitHub issue/PR comments quite a lot.

Note also: work is in progress on adding the details-auto-expand feature to Safari in the very near future. So we’ll soon have support in two major browsers to let users find text in closed details contents in GitHub PRs and issues — but it’ll only work if the primer styling for contents of closed details is updated to not have display: none !important.

@siddharthkp
Copy link
Contributor

siddharthkp commented Mar 22, 2024

@sideshowbarker Thanks for taking the time to open an issue! ❤

@langermank, Does this look safe to delete? I don't think we support any browsers that don't automatically handle this anymore (https://caniuse.com/mdn-html_elements_details_open)

&:not([open]) { 
   // Set details content hidden by default for browsers that don't do this 
   > *:not(summary) { 
     display: none !important; 
   } 
 } 

@sideshowbarker
Copy link
Author

@keithamus It seems that the fix in #2611 wasn’t sufficient for making the details elements auto-expand for find-in-page matches. See the repro steps above in #2592 (comment)

So I’ve opened #2624 with a patch that goes further to completely removed the display: none styling.

sideshowbarker added a commit to sideshowbarker/css that referenced this issue May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants