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.

Sign up for GitHub

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

Jump to bottom

rustdoc: add interaction delays for tooltip popovers #111892

Merged
merged 3 commits into from
Jun 1, 2023
Merged

rustdoc: add interaction delays for tooltip popovers #111892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c1d72de
rustdoc: add interaction delays for tooltip popovers
notriddle May 23, 2023
b9302d7
rustdoc: add hover indicator for notable trait tooltip
notriddle May 24, 2023
d7d497a
rustdoc: add jsdoc comments for complex functions
notriddle May 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/librustdoc/html/static/css/rustdoc.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,6 +1179,10 @@ a.test-arrow:hover {
position: relative;
}

.code-header a.tooltip:hover {
color: var(--link-color);
}

/* placeholder thunk so that the mouse can easily travel from "(i)" to popover
the resulting "hover tunnel" is a stepped triangle, approximating
https://bjk5.com/post/44698559168/breaking-down-amazons-mega-dropdown */
Expand All @@ -1191,6 +1195,14 @@ a.tooltip:hover::after {
content: "\00a0";
}

/* This animation is layered onto the mistake-proofing delay for dismissing
a hovered tooltip, to ensure it feels responsive even with the delay.
*/
.fade-out {
opacity: 0;
transition: opacity 0.45s cubic-bezier(0, 0, 0.1, 1.0);
}

.popover.tooltip .content {
margin: 0.25em 0.5em;
}
Expand Down
148 changes: 138 additions & 10 deletions src/librustdoc/html/static/js/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@

"use strict";

// The amount of time that the cursor must remain still over a hover target before
// revealing a tooltip.
//
// https://www.nngroup.com/articles/timing-exposing-content/
window.RUSTDOC_TOOLTIP_HOVER_MS = 300;
window.RUSTDOC_TOOLTIP_HOVER_EXIT_MS = 450;

// Given a basename (e.g. "storage") and an extension (e.g. ".js"), return a URL
// for a resource under the root-path, with the resource-suffix.
function resourcePath(basename, extension) {
Expand Down Expand Up @@ -772,6 +779,13 @@ function preLoadCss(cssUrl) {
});
});

/**
* Show a tooltip immediately.
*
* @param {DOMElement} e - The tooltip's anchor point. The DOM is consulted to figure
* out what the tooltip should contain, and where it should be
* positioned.
*/
function showTooltip(e) {
const notable_ty = e.getAttribute("data-notable-ty");
if (!window.NOTABLE_TRAITS && notable_ty) {
Expand All @@ -782,20 +796,29 @@ function preLoadCss(cssUrl) {
throw new Error("showTooltip() called with notable without any notable traits!");
}
}
// Make this function idempotent. If the tooltip is already shown, avoid doing extra work
// and leave it alone.
if (window.CURRENT_TOOLTIP_ELEMENT && window.CURRENT_TOOLTIP_ELEMENT.TOOLTIP_BASE === e) {
// Make this function idempotent.
clearTooltipHoverTimeout(window.CURRENT_TOOLTIP_ELEMENT);
return;
}
window.hideAllModals(false);
const wrapper = document.createElement("div");
if (notable_ty) {
wrapper.innerHTML = "<div class=\"content\">" +
window.NOTABLE_TRAITS[notable_ty] + "</div>";
} else if (e.getAttribute("title") !== undefined) {
const titleContent = document.createElement("div");
titleContent.className = "content";
titleContent.appendChild(document.createTextNode(e.getAttribute("title")));
wrapper.appendChild(titleContent);
} else {
// Replace any `title` attribute with `data-title` to avoid double tooltips.
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);
}
Comment on lines +812 to +821
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."

}
wrapper.className = "tooltip popover";
const focusCatcher = document.createElement("div");
Expand Down Expand Up @@ -824,17 +847,77 @@ function preLoadCss(cssUrl) {
wrapper.style.visibility = "";
window.CURRENT_TOOLTIP_ELEMENT = wrapper;
window.CURRENT_TOOLTIP_ELEMENT.TOOLTIP_BASE = e;
clearTooltipHoverTimeout(window.CURRENT_TOOLTIP_ELEMENT);
wrapper.onpointerenter = function(ev) {
// If this is a synthetic touch event, ignore it. A click event will be along shortly.
if (ev.pointerType !== "mouse") {
return;
}
clearTooltipHoverTimeout(e);
};
wrapper.onpointerleave = function(ev) {
// If this is a synthetic touch event, ignore it. A click event will be along shortly.
if (ev.pointerType !== "mouse") {
return;
}
if (!e.TOOLTIP_FORCE_VISIBLE && !elemIsInParent(event.relatedTarget, e)) {
hideTooltip(true);
if (!e.TOOLTIP_FORCE_VISIBLE && !elemIsInParent(ev.relatedTarget, e)) {
// See "Tooltip pointer leave gesture" below.
setTooltipHoverTimeout(e, false);
addClass(wrapper, "fade-out");
}
};
}

/**
* Show or hide the tooltip after a timeout. If a timeout was already set before this function
* was called, that timeout gets cleared. If the tooltip is already in the requested state,
* this function will still clear any pending timeout, but otherwise do nothing.
*
* @param {DOMElement} element - The tooltip's anchor point. The DOM is consulted to figure
* out what the tooltip should contain, and where it should be
* positioned.
* @param {boolean} show - If true, the tooltip will be made visible. If false, it will
* be hidden.
*/
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.

clearTooltipHoverTimeout(element);
if (!show && !window.CURRENT_TOOLTIP_ELEMENT) {
// To "hide" an already hidden element, just cancel its timeout.
return;
}
if (show && window.CURRENT_TOOLTIP_ELEMENT) {
// To "show" an already visible element, just cancel its timeout.
return;
}
if (window.CURRENT_TOOLTIP_ELEMENT &&
window.CURRENT_TOOLTIP_ELEMENT.TOOLTIP_BASE !== element) {
// Don't do anything if another tooltip is already visible.
return;
}
element.TOOLTIP_HOVER_TIMEOUT = setTimeout(() => {
if (show) {
showTooltip(element);
} else if (!element.TOOLTIP_FORCE_VISIBLE) {
hideTooltip(false);
}
}, show ? window.RUSTDOC_TOOLTIP_HOVER_MS : window.RUSTDOC_TOOLTIP_HOVER_EXIT_MS);
}

/**
* If a show/hide timeout was set by `setTooltipHoverTimeout`, cancel it. If none exists,
* do nothing.
*
* @param {DOMElement} element - The tooltip's anchor point,
* as passed to `setTooltipHoverTimeout`.
*/
function clearTooltipHoverTimeout(element) {
if (element.TOOLTIP_HOVER_TIMEOUT !== undefined) {
removeClass(window.CURRENT_TOOLTIP_ELEMENT, "fade-out");
clearTimeout(element.TOOLTIP_HOVER_TIMEOUT);
delete element.TOOLTIP_HOVER_TIMEOUT;
}
}

function tooltipBlurHandler(event) {
if (window.CURRENT_TOOLTIP_ELEMENT &&
!elemIsInParent(document.activeElement, window.CURRENT_TOOLTIP_ELEMENT) &&
Expand All @@ -854,6 +937,12 @@ function preLoadCss(cssUrl) {
}
}

/**
* Hide the current tooltip immediately.
*
* @param {boolean} focus - If set to `true`, move keyboard focus to the tooltip anchor point.
* If set to `false`, leave keyboard focus alone.
*/
function hideTooltip(focus) {
if (window.CURRENT_TOOLTIP_ELEMENT) {
if (window.CURRENT_TOOLTIP_ELEMENT.TOOLTIP_BASE.TOOLTIP_FORCE_VISIBLE) {
Expand All @@ -864,6 +953,7 @@ function preLoadCss(cssUrl) {
}
const body = document.getElementsByTagName("body")[0];
body.removeChild(window.CURRENT_TOOLTIP_ELEMENT);
clearTooltipHoverTimeout(window.CURRENT_TOOLTIP_ELEMENT);
window.CURRENT_TOOLTIP_ELEMENT = null;
}
}
Expand All @@ -886,7 +976,14 @@ function preLoadCss(cssUrl) {
if (ev.pointerType !== "mouse") {
return;
}
showTooltip(this);
setTooltipHoverTimeout(this, true);
};
e.onpointermove = function(ev) {
// If this is a synthetic touch event, ignore it. A click event will be along shortly.
if (ev.pointerType !== "mouse") {
return;
}
setTooltipHoverTimeout(this, true);
};
e.onpointerleave = function(ev) {
// If this is a synthetic touch event, ignore it. A click event will be along shortly.
Expand All @@ -895,7 +992,38 @@ function preLoadCss(cssUrl) {
}
if (!this.TOOLTIP_FORCE_VISIBLE &&
!elemIsInParent(ev.relatedTarget, window.CURRENT_TOOLTIP_ELEMENT)) {
hideTooltip(true);
// Tooltip pointer leave gesture:
//
// 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 or 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. Search "hover tunnel" in
// rustdoc.css for the implementation.
// * There's a delay when the mouse pointer enters the popover base anchor, in
// case the mouse pointer was just passing through and the user didn't want
// to open it.
// * Similarly, a delay is added when exiting the anchor, or the popover
// itself, 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 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
setTooltipHoverTimeout(e, false);
addClass(window.CURRENT_TOOLTIP_ELEMENT, "fade-out");
}
};
});
Expand Down
2 changes: 2 additions & 0 deletions tests/rustdoc-gui/codeblock-tooltip.goml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ define-function: (
"background-color": |background|,
"border-color": |border|,
})
click: ".docblock .example-wrap.compile_fail .tooltip"

// should_panic block
assert-css: (
Expand Down Expand Up @@ -71,6 +72,7 @@ define-function: (
"background-color": |background|,
"border-color": |border|,
})
click: ".docblock .example-wrap.should_panic .tooltip"

// ignore block
assert-css: (
Expand Down
19 changes: 17 additions & 2 deletions tests/rustdoc-gui/notable-trait.goml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ assert-count: ("//*[@class='tooltip popover']", 0)
// Now check the colors.
define-function: (
"check-colors",
(theme, header_color, content_color, type_color, trait_color),
(theme, header_color, content_color, type_color, trait_color, link_color),
block {
go-to: "file://" + |DOC_PATH| + "/test_docs/struct.NotableStructWithLongName.html"
// This is needed to ensure that the text color is computed.
Expand All @@ -133,8 +133,20 @@ define-function: (
// We reload the page so the local storage settings are being used.
reload:

assert-css: (
"//*[@id='method.create_an_iterator_from_read']//*[@class='tooltip']",
{"color": |content_color|},
ALL,
)

move-cursor-to: "//*[@id='method.create_an_iterator_from_read']//*[@class='tooltip']"
assert-count: (".tooltip.popover", 1)
wait-for-count: (".tooltip.popover", 1)

assert-css: (
"//*[@id='method.create_an_iterator_from_read']//*[@class='tooltip']",
{"color": |link_color|},
ALL,
)

assert-css: (
".tooltip.popover h3",
Expand Down Expand Up @@ -163,6 +175,7 @@ call-function: (
"check-colors",
{
"theme": "ayu",
"link_color": "rgb(57, 175, 215)",
"content_color": "rgb(230, 225, 207)",
"header_color": "rgb(255, 255, 255)",
"type_color": "rgb(255, 160, 165)",
Expand All @@ -174,6 +187,7 @@ call-function: (
"check-colors",
{
"theme": "dark",
"link_color": "rgb(210, 153, 29)",
"content_color": "rgb(221, 221, 221)",
"header_color": "rgb(221, 221, 221)",
"type_color": "rgb(45, 191, 184)",
Expand All @@ -185,6 +199,7 @@ call-function: (
"check-colors",
{
"theme": "light",
"link_color": "rgb(56, 115, 173)",
"content_color": "rgb(0, 0, 0)",
"header_color": "rgb(0, 0, 0)",
"type_color": "rgb(173, 55, 138)",
Expand Down