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

Use absolute positioning of dfn panels and highlight ref links #2689

Merged
merged 17 commits into from
Oct 12, 2023

Conversation

dlaliberte
Copy link
Collaborator

@dlaliberte dlaliberte commented Oct 10, 2023

This PR is for some UI fixes and affordances regarding dfn panels and references links.

  • dfn panel is always raised to the root level, changed to absolute positioning next to the dfn when clicking on a dfn. This avoids any truncation when the dfn is contained in a scrolling container. It also scrolls with the page.
  • Clicking on links in dfn panel don't scroll the page if the destination of the link is already in the view.
  • Destinations are always highlighted for 3 seconds.
  • When the dfn panel is "activated" and moved to the bottom left corner, only the horizontal animation works for some reason. But this is perhaps enough.
  • I experimented with smooth scrolling, but it is still rather janky and not very helpful.

@dlaliberte dlaliberte requested a review from tabatkins October 10, 2023 22:52
@dlaliberte
Copy link
Collaborator Author

dlaliberte commented Oct 11, 2023

Tabbing now works as follows:

  • When a panel is shown, the tabIndex of the dfn is set to 100 (no difference with 1) and all anchor and button tags in the panel are set to increments above that.
  • Tab and shift-tab from the dfn to the panel (and within the panel) and all back work as expected.
  • Before and after the dfn and its panel, everything has undefined tabIndex, so tabbing skips to the window level or "Jump to Table of Contents" .
    • I suspect the only way to avoid this is to globally coordinate all tabIndexes.
    • Leaving the dfn tabIndex at 0 and incrementing the panel elements to 1 and above does not work.
    • Starting at 1 works the same as 100.
  • Pinning the panel preserves all this behavior.
  • Closing a panel sets the dfn tabIndex back to undefined.

@dlaliberte
Copy link
Collaborator Author

I'm surprised to learn that many pages try to hide the focus ring around links and buttons. This is a useful user affordance that is important to preserve in some cases.

In pinned dfn panels, to make click and keyboard behavior consistent, independent of whether the link destination needs to be scrolled to, the focus should remain on the link in the panel. So the focus should also remain visible either way.

@dlaliberte dlaliberte changed the title Use fixed positioning of dfn panels always and highlight ref links Use absolute positioning of dfn panels always and highlight ref links Oct 11, 2023
@dlaliberte dlaliberte changed the title Use absolute positioning of dfn panels always and highlight ref links Use absolute positioning of dfn panels and highlight ref links Oct 12, 2023
@tabatkins tabatkins merged commit 0e6afc0 into main Oct 12, 2023
@tabatkins tabatkins deleted the dlaliberte-dfnpanels-10-04 branch October 12, 2023 20:21
@tabatkins
Copy link
Collaborator

Excellent! I've just done a few edits:

  • I selected some better-looking colors (I did this before you committed your latest color patch; if you feel you liked the colors you chose better, feel free to change them)
  • I moved the positioning code to a separate function so I could invoke it in the resize event rather than just pinning it.
  • Fixed the positioning when the panel would overflow; it now just gets nudged to the left if it would overflow to the right (and avoids nudging too far into the unscrollable zone).

And then just a touch of refactoring while I was doing the above; in particular, rather than constantly passing around the panel and the dfn, I just store the dfn on the panel when it's generated so I can grab it when needed. (I needed this since the resize event wouldn't have both elements in hand.)

@tabatkins
Copy link
Collaborator

When the dfn panel is "activated" and moved to the bottom left corner, only the horizontal animation works for some reason. But this is perhaps enough.

Oh yeah, this happens because the abspos code sets the element's 'top' (leaving 'bottom' as auto) while the fixpos code sets the 'bottom' (leaving 'top' as auto). So neither 'top' nor 'bottom' is transitionable. We could fix this by swapping to setting 'bottom' in the activation function before we switch to fixed, but I think the animation as it currently exists is enough to draw the eye.

Copy link
Collaborator Author

@dlaliberte dlaliberte left a comment

Choose a reason for hiding this comment

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

This will need to be fixed.

@tabatkins
Copy link
Collaborator

Yup, fixed in trunk now.

@dlaliberte dlaliberte mentioned this pull request Oct 13, 2023
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

Successfully merging this pull request may close these issues.

2 participants