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

rustdoc: add interaction delays for tooltip popovers #111892

Merged
merged 3 commits into from
Jun 1, 2023

Conversation

notriddle
Copy link
Contributor

Preview:

Designing a good hover microinteraction is a matter of guessing user intent from what are, literally, vague gestures. In this case, guessing if hovering in our out of the tooltip base is intentional or not.

To figure this out, a few different techniques are used:

  • When the mouse pointer enters a tooltip anchor point, its hitbox is grown on the bottom, where the popover is/will appear. This was already there before this commit: search "hover tunnel" in rustdoc.css for the implementation.

  • This commit adds a delay when the mouse pointer enters the base anchor, in case the mouse pointer was just passing through and the user didn't want to open it.

  • This commit also adds a delay when the mouse pointer exits the tooltip's base anchor or its popover, before hiding it.

  • A fade-out animation is layered onto the pointer exit delay to immediately inform the user that they successfully dismissed the popover, while still providing a way for them to cancel it if it was a mistake and they still wanted to interact with it.

  • No animation is used for revealing it, because we don't want people to try to interact with an element while it's in the middle of fading in: either they're allowed to interact with it while it's fading in, meaning it can't serve as mistake- proofing for opening the popover, or they can't, but they might try and be frustrated.

See also:

Designing a good hover microinteraction is a matter of guessing
user intent from what are, literally, vague gestures. In this case,
guessing if hovering in our out of the tooltip base is intentional
or not.

To figure this out, a few different techniques are used:

* When the mouse pointer enters a tooltip anchor point, its hitbox
  is grown on the bottom, where the popover is/will appear. This was
  already there before this commit: search "hover tunnel" in
  rustdoc.css for the implementation.

* This commit adds a delay when the mouse pointer enters the base
  anchor, in case the mouse pointer was just passing through and the
  user didn't want to open it.

* This commit also adds a delay when the mouse pointer exits the
  tooltip's base anchor or its popover, before hiding it.

* A fade-out animation is layered onto the pointer exit delay to
  immediately inform the user that they successfully dismissed the
  popover, while still providing a way for them to cancel it if
  it was a mistake and they still wanted to interact with it.

* No animation is used for revealing it, because we don't want
  people to try to interact with an element while it's in the
  middle of fading in: either they're allowed to interact with
  it while it's fading in, meaning it can't serve as mistake-
  proofing for opening the popover, or they can't, but they
  might try and be frustrated.

See also:

* https://www.nngroup.com/articles/timing-exposing-content/
* https://www.nngroup.com/articles/tooltip-guidelines/
* https://bjk5.com/post/44698559168/breaking-down-amazons-mega-dropdown
@rustbot
Copy link
Collaborator

rustbot commented May 23, 2023

r? @GuillaumeGomez

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 23, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 23, 2023

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@notriddle
Copy link
Contributor Author

@Manishearth #111856 (comment)

Looking at the preview, yeah, that looks better. From the guidelines we may want to introduce some instant non-popup hover interaction to the ℹ️, similar to what we have going on with the panic interface, so that people know that an interaction has "registered". Perhaps give it a more muted color that becomes darker, or just make the existing color darker, or give it a shadow, or something.

That seems like a good idea. It'll take a bit to upload to the preview, but I've added a patch so it'll change the (i) to the link color when you mouse over it.

@Manishearth
Copy link
Member

Yeah that looks great! I'll have a look at the code later but I'm happy from that perspective.

@Manishearth
Copy link
Member

Code lgtm. Would want @jsha to be the final arbiter on this

@jsha
Copy link
Contributor

jsha commented May 24, 2023

I haven't read the code in detail but the concept is great, and playing with the demo it feels good too. I didn't know about this concept before and I feel like I learned a lot from reading that Nielsen Norman Group article. Thanks for sharing it!

@GuillaumeGomez
Copy link
Member

Code and feature both look great to me as well, nice work! Waiting for @jsha to approve it.

Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this! A few small bits of feedback below.

src/librustdoc/html/static/js/main.js Outdated Show resolved Hide resolved
Comment on lines +803 to +812
if (e.getAttribute("title") !== null) {
e.setAttribute("data-title", e.getAttribute("title"));
e.removeAttribute("title");
}
if (e.getAttribute("data-title") !== null) {
const titleContent = document.createElement("div");
titleContent.className = "content";
titleContent.appendChild(document.createTextNode(e.getAttribute("data-title")));
wrapper.appendChild(titleContent);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the purpose of this code. It looks like it replaces any title attribute with a data-title attribute containing the same value. Why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to prevent double tooltips.

I've added a comment addressing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, I see now. Presumably that fixes the behavior illustrated below where both the hover tooltip and the browser's title overlay can show at once?

image

(only a problem for code examples; the notable traits tooltip hover target doesn't have a title)

I think it would be better to solve this by omitting the title attribute from the code example hover target. According to https://inclusive-components.design/tooltips-toggletips/, "Don't rely on title attributes. They are not keyboard accessible and are not supported in many screen reader setups."

That leaves us the problem of how to announce this information ("this example panics") to screen readers. Perhaps we could use one of these invisible content tricks? https://webaim.org/techniques/css/invisiblecontent/. In other words, we could include the text "this example panics" in an offscreen element, in the appropriate DOM position to be read by a screen reader. And for hover we could move that offscreen element into the visually correct place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, I see now. Presumably that fixes the behavior illustrated below where both the hover tooltip and the browser's title overlay can show at once?

Yes, exactly.

Perhaps we could use one of these invisible content tricks?

I'm not sure this makes sense.

The part that makes this harder than it would normally be is the way popovers and containment interact. #102576 and #104129 required the popover to actually be outside the docblock in the DOM, and positioned visually using JavaScript1.

The other problem is that this is a toggletip. The article you linked to does not recommend making screen readers announce the contents of toggletips unless the user clicks them:

Because a screen reader user would have access to the information before pressing the button, so pressing it would appear not to do anything. Technically, they have access to the information, making the control "accessible" — but the control simply wouldn't make sense. In other words, it's a user experience issue more than a strict accessibility one, but it's important.

That leaves us the problem of how to announce this information ("this example panics") to screen readers.

Either hide the button from screen readers entirely and replace it with text, or make sure that screen readers announce the popover when it's focused?

Footnotes

  1. To make popover tooltips keyboard-accessible, clicking on the button moves keyboard focus to the popover, and when focus leaves the popover it's moved back to the button again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, yeah, it's complicated. At any rate, this isn't a new problem introduced by this PR, and indeed you've fixed an existing bug, so let's go ahead as-is and we can talk separately about more improvements / simplifications to the handling of "this example panics."

}
};
}

function setTooltipHoverTimeout(element, show) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a JSDoc comment to this and clearTooltipHoverTimeout? In particular I'm interested seeing that show is documented to indicate that true means "show the tooltip" and false means "hide the tooltip".

Alternately, maybe it would be clearer if this took a callback, either showTooltip or hideTooltip? That would make it obvious at the callsite what's happening. That would require refactoring hideTooltip slightly so that it can take an element as its only parameter (which it might ignore). That in turn would probably require providing a hideTooltipAndFocusBase to cover the hideTooltip(true) behavior.

Broadly speaking the intuition I'm working from is that boolean parameters often wind up confusing because the meaning is not obvious at the call site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Boolean flags are rightfully considered harmful. I've added a comment to document what it's doing.

I don't know how a callback would work, since it's not just calling the function with a timeout. It also has guard conditionals at the start so that it skips the timeout if the tooltip is already in the desired state, which means it needs to actually look at the current state and compare it with the bool flag.

@rust-log-analyzer

This comment has been minimized.

@jsha
Copy link
Contributor

jsha commented Jun 1, 2023

Looks good!

@bors r=me,GuillaumeGomez,Manishearth rollup

@bors
Copy link
Contributor

bors commented Jun 1, 2023

📌 Commit d7d497a has been approved by me,GuillaumeGomez,Manishearth

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 1, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 1, 2023
…p, r=me,GuillaumeGomez,Manishearth

rustdoc: add interaction delays for tooltip popovers

Preview:

* [notable traits](http://notriddle.com/rustdoc-demo-html-3/delay-tooltip/testing/struct.Vec.html#method.iter)
* [panicking code block](http://notriddle.com/rustdoc-demo-html-3/delay-tooltip/testing/struct.Vec.html#indexing)

Designing a good hover microinteraction is a matter of guessing user intent from what are, literally, vague gestures. In this case, guessing if hovering in our out of the tooltip base is intentional or not.

To figure this out, a few different techniques are used:

* When the mouse pointer enters a tooltip anchor point, its hitbox is grown on the bottom, where the popover is/will appear. This was already there before this commit: search "hover tunnel" in rustdoc.css for the implementation.

* This commit adds a delay when the mouse pointer enters the base anchor, in case the mouse pointer was just passing through and the user didn't want to open it.

* This commit also adds a delay when the mouse pointer exits the tooltip's base anchor or its popover, before hiding it.

* A fade-out animation is layered onto the pointer exit delay to immediately inform the user that they successfully dismissed the popover, while still providing a way for them to cancel it if it was a mistake and they still wanted to interact with it.

* No animation is used for revealing it, because we don't want people to try to interact with an element while it's in the middle of fading in: either they're allowed to interact with it while it's fading in, meaning it can't serve as mistake- proofing for opening the popover, or they can't, but they might try and be frustrated.

See also:

* https://www.nngroup.com/articles/timing-exposing-content/
* https://www.nngroup.com/articles/tooltip-guidelines/
* https://bjk5.com/post/44698559168/breaking-down-amazons-mega-dropdown
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
Rollup of 7 pull requests

Successful merges:

 - rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
 - rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
 - rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
 - rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
 - rust-lang#112024 (Don't suggest break through nested items)
 - rust-lang#112128 (Don't compute inlining status of mono items in advance.)
 - rust-lang#112141 (remove reference to Into in ? operator core/std docs, fix rust-lang#111655)

Failed merges:

 - rust-lang#112071 (Group rfcs tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 453fc03 into rust-lang:master Jun 1, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 1, 2023
@notriddle notriddle deleted the notriddle/timeout-tooltip branch June 1, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants