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

Scroll to the start node of the first text directive instead of its common ancestor #259

Open
jnjaeschke opened this issue Jul 9, 2024 · 2 comments

Comments

@jnjaeschke
Copy link

If a text directive should be scrolled to, it should be visible in the center of the view. However, scrolling the common ancestor of a range into the center of the view is not necessarily scrolling the range into view.

Example:

<html>
  <style>
    .space {
      margin-top: 200vh;
    }
  </style>
  <body>
    <p class="space">Test</p>
    <p class="space">Test2</p>
  </body>
</html>

Text Directive: :~:text=Test,Test2

An implementation that follows the spec would scroll in between the two <p>, so that neither Test nor Test2 would be visible. This is confusing for users. Therefore, my proposal is to change the spec so that the start container of the range is visible at the center of the view instead of the common ancestor.

Chrome already follows this proposal and scrolls Test into the center of the view. Safari and Gecko (as of now) follow the spec. I'm planning to change the implementation in Gecko to align with Chrome.

/cc @zcorpan

@bokand
Copy link
Collaborator

bokand commented Jul 9, 2024

Yeah, I agree that makes more sense. The intent here was that the first common ancestor is used to set the :target pseudo class since we can only set it on one element (I think?). But scrolling to it seems like a bug - we could separate the element used for :target and scroll the start container as you suggest.

@annevk
Copy link
Collaborator

annevk commented Jul 11, 2024

cc @megangardner

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 15, 2024
…ad of its common ancestor.

Spec issue: WICG/scroll-to-text-fragment#259

Differential Revision: https://phabricator.services.mozilla.com/D216147

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1906895
gecko-commit: 91c93776446c64bb2438b0fa69d0b9f198bb00a9
gecko-reviewers: emilio
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 15, 2024
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 15, 2024
…ad of its common ancestor.

Spec issue: WICG/scroll-to-text-fragment#259

Differential Revision: https://phabricator.services.mozilla.com/D216147

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1906895
gecko-commit: 91c93776446c64bb2438b0fa69d0b9f198bb00a9
gecko-reviewers: emilio
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Jul 16, 2024
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Jul 18, 2024
sadym-chromium pushed a commit to web-platform-tests/wpt that referenced this issue Jul 18, 2024
…ad of its common ancestor.

Spec issue: WICG/scroll-to-text-fragment#259

Differential Revision: https://phabricator.services.mozilla.com/D216147

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1906895
gecko-commit: 91c93776446c64bb2438b0fa69d0b9f198bb00a9
gecko-reviewers: emilio
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants