Fix tooltips used on targets with delegated focus #4606

bearfriend · 2024-04-16T20:03:23Z

Currently, if a tooltip targets an element with delegated focus (i.e. basically everything: d2l-button, d2l-link, d2l-input-text, etc.), that target will never be properly described by the tooltip because once it delegates focus internally, the newly focused element is not described by the tooltip.

Non-interactable target warnings are currently disabled on the constructor after a single warning, so most tooltips are never communicating this issue.

Once that restriction is removed, all focus-delegated targets begin to warn because they are not in interactiveElements and do not have a role as the delegate should handle both of these.

Detect elements using FocusMixin, use their delegate as the tooltip target (which also prevents the warning)
Since delegates are always across shadow DOM boundaries, set an aria-label/description directly on the target
Manage label, description, labelledby, and describedby appropriately
Move the old "I don't know, maybe?" warning to a definitely-worded error
Disable logging the error only once
Prevent disconnected tooltips from hiding the activeTooltip
Prevents firing d2l-tooltip-show on tooltips that were never shown before being hidden by another tooltip
Read tag-list-item truncated text as a label, instead of duplicating it as a description

GAUD-6266: Investigate "d2l-tooltip" non-accessible warnings

github-actions · 2024-04-16T20:04:30Z

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL	https://live.d2l.dev/prs/BrightspaceUI/core/pr-4606/

Note

The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

bearfriend · 2024-04-16T20:09:40Z

components/tooltip/tooltip.js

@@ -96,7 +94,7 @@ const computeTooltipShift = (centerDelta, spaceLeft, spaceRight) => {

 /**
 * A component used to display additional information when users focus or hover on a point of interest.
- * @slot - Default content placed inside of the tooltip
+ * @slot - Default content placed inside of the tooltip. This content is also used as the description for the target element, and may override existing descriptors.


It will override if the target focus is delegated

bearfriend · 2024-04-16T20:26:05Z

components/tooltip/tooltip.js

+		if (this._target && this.id) {
+			elemIdListRemove(this._target, 'aria-labelledby', this.id);
+			elemIdListRemove(this._target, 'aria-describedby', this.id);
+			if (this.#targetDelegated) {
+				this._target.removeAttribute('aria-label');
+				this._target.removeAttribute('aria-description');
+			}
+		}


If we already have a target, we're likely updating it, so clean up the existing one.

bearfriend · 2024-04-16T20:29:10Z

components/tooltip/tooltip.js

+				if (this.forType === 'label') {
+					this.removeAttribute('aria-labelledby');
+					this._target.ariaLabel = this.shadowRoot.querySelector('slot').assignedNodes().map(n => n.textContent).join('\n');
+				} else if (!this.announced || isInteractive) {
+					this.removeAttribute('aria-describedby');
+					this._target.ariaDescription = this.shadowRoot.querySelector('slot').assignedNodes().map(n => n.textContent).join('\n');
+				}


When the target has been delegated, we can't use the ...by properties, which take precedence, so remove them.

bearfriend · 2024-04-16T20:31:46Z

components/tooltip/tooltip.js

+				if (this.forType === 'label') {
+					this._target.removeAttribute('aria-label');
+					elemIdListAdd(this._target, 'aria-labelledby', this.id);
+				} else if (!this.announced || isInteractive) {
+					this._target.removeAttribute('aria-description');
+					elemIdListAdd(this._target, 'aria-describedby', this.id);
+				}


Removing the label/description isn't strictly necessary here, but they won't be used since the ...by property we're setting takes precedence, and cleaning them up seems good.

So if I'm reading this correctly, there's no way for the downstream code (e.g. d2l-button) to leverage aria-labelledby/aria-decribedby to set a label/description themselves on their <button> and then have a consumer point a tooltip at them... since that would fall into the targetDelegated case which would then remove their label/description?

The only way it could work would be if <d2l-button> itself rendered the tooltip, in which case they're in the same scope and therefore not delegated so the tooltip id would get added to the list.

Right, I couldn't find a convenient way to get the computed label/description, besides by just getting the element(s) via the id and grabbing the text content, which seems like it's just as well to handle once in the tooltip code rather than duplicating that in every delegated-focus component.

Obviously we will someday be able to use the ElementInternals/AOM stuff, but until then this seems better than having nothing.

The only way it could work would be if itself rendered the tooltip, in which case they're in the same scope and therefore not delegated so the tooltip id would get added to the list.

Yes, that's exactly right. I think there could be some much more complicated solutions, but I'm not sure it's worth the effort.

Cool, makes sense! I thought of an alternate approach last night... so as a thought experiment:

What if the components themselves opted into this using something like a TooltipTargetMixin? That mixin could come with some reactive state properties like tooltipValue and tooltipType, and the tooltip code would set those whenever the tooltip value changed or it was shown/hidden.

That would then put the responsibility on the target to figure out what to actually do with the value and what element to put it on -- using aria-label/description, maybe an offscreen element hooked up with aria-labelledby if there are multiple labels, etc.

The advantage to this approach is that it empowers the target element to "do what's best" and hooks into Lit's lifecycle to do the rendering. Tooltip could even throw if the target didn't implement the mixin. It's obviously more work though for each one to opt in.

My worry with the more "automatic" approach is that consumers can accidentally mess up a core component's accessibility by unknowingly completely overriding its label. But maybe that's what we want anyway... if you're pointing a tooltip at a button or whatever, you probably want its value to become the label. 🤷

This overlaps with the comments I just made.
https://github.com/BrightspaceUI/core/pull/4606/files#r1571060598
https://github.com/BrightspaceUI/core/pull/4606/files#r1571072507

Dave, if I follow you correctly, that would address my concern about modifying the DOM outside of the target custom element's render method. That's where my mind was going too, but you put a bit more thought into the sol'n. 😄

Yeah, that was my intent.

Yeah, I agree with all of this. I'll investigate a little more, and try to get some numbers on how many places would need to be updated.

bearfriend · 2024-04-16T20:33:46Z

components/tag-list/tag-list-item-mixin.js

@@ -264,7 +264,7 @@ export const TagListItemMixin = superclass => class extends LocalizeCoreElement(
 		} else if (options.hasTruncationTooltip || hasDescription) {
 			const tooltipHeader = hasDescription ? html`<div class="d2l-heading-4">${tagContent}</div>` : tagContent;
 			tooltip = html`
-				<d2l-tooltip class="vdiff-target" for="${this._id}" ?show-truncated-only="${!hasDescription}">
+				<d2l-tooltip class="vdiff-target" for="${this._id}" ?show-truncated-only="${!hasDescription}" for-type="${hasDescription ? 'descriptor' : 'label'}">


Prevents reading out the truncated contents twice.

…ls + target warning

components/tooltip/tooltip.js

bearfriend · 2024-04-17T16:54:07Z

components/tooltip/tooltip.js

@@ -892,19 +909,22 @@ class Tooltip extends RtlMixin(LitElement) {
 	}

 	async _showingChanged(newValue, dispatch) {
+		if (!this.isConnected) return;


I'm not sure why this only started being necessary now, but input-date-range has two layers of validation tooltips, and the upper layer (which is suppressed if there are child validation errors), was showing itself and hiding the child error before then removing itself from the DOM to allow the child tooltip to show, but that one had already been hidden, so we ended up with none. This simply bails if the tooltip that called it is no longer connected to the DOM.

bearfriend · 2024-04-17T16:57:20Z

components/tooltip/tooltip.js

 			await this.updateComplete;
-			await this.updatePosition();
-			if (dispatch) {
+
+			if (this._showing && dispatch) {
+				await this.updatePosition();
 				this.dispatchEvent(new CustomEvent(


If, after the update, another tooltip has become active (and hidden this one), don't fire the d2l-tooltip-show event since it never actually showed. Also, don't bother updating the position since it can and should be recalculated on the next show

dbatiste · 2024-04-18T16:30:16Z

components/tooltip/tooltip.js

+
+			if (this.#targetDelegated) {
+				if (this.forType === 'label') {
+					this._target.removeAttribute('aria-labelledby');


Is it safe to remove aria-labelledby and aria-describedby from the target? i.e. what if the target custom element has set this attribute... this code would be wiping it out. I've always felt like modifying the target's DOM was sketchy here, violating the principle that the custom element's DOM should strictly be a function of its render method and reactive properties.

dbatiste · 2024-04-18T16:37:31Z

components/tooltip/tooltip.js

+				if (this.forType === 'label') {
+					this._target.removeAttribute('aria-labelledby');
+					this._target.ariaLabel = this.shadowRoot.querySelector('slot').assignedNodes().map(n => n.textContent).join('\n');
+				} else if (!this.announced || isInteractive) {


With your changes, do we still need the announced property/attribute? It was intended to be used in these cases where the tooltip could not be wired up due to the id ref / dom scope limitation. But your solution appears to handle that in a better way.

There was this one place in d2l-input-text where we intentionally used announced (... announce()), because we didn't want the validation message buried in VO's menu. We'll want to make sure the validation message is not buried - it should be announced upon focus with no additional user interaction.

bearfriend requested a review from a team as a code owner April 16, 2024 20:03

github-actions bot mentioned this pull request Apr 16, 2024

Updating vdiff goldens for PR 4606 #4607

Closed

bearfriend commented Apr 16, 2024

View reviewed changes

bearfriend added 3 commits April 16, 2024 19:43

Fix tooltips used on targets with delegated focus + descriptions/labe…

c7a44af

…ls + target warning

Fix truncating

d6feb7b

Cleanup

1938db8

bearfriend force-pushed the dgleckler/tooltip-a11y-warning branch from 106da3d to 1938db8 Compare April 17, 2024 00:17

bearfriend commented Apr 17, 2024

View reviewed changes

components/tooltip/tooltip.js Outdated Show resolved Hide resolved

bearfriend added 2 commits April 16, 2024 21:48

Wait for nextFrame

19d6d87

Fix input-date-range tooltip precedence

5193929

bearfriend commented Apr 17, 2024

View reviewed changes

dbatiste reviewed Apr 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tooltips used on targets with delegated focus #4606

Fix tooltips used on targets with delegated focus #4606

bearfriend commented Apr 16, 2024 •

edited by jira bot

github-actions bot commented Apr 16, 2024

bearfriend Apr 16, 2024

bearfriend Apr 16, 2024

bearfriend Apr 16, 2024

This comment was marked as resolved.

bearfriend Apr 16, 2024

dlockhart Apr 17, 2024 •

edited

bearfriend Apr 17, 2024 •

edited

dlockhart Apr 18, 2024

dbatiste Apr 18, 2024

dlockhart Apr 18, 2024

bearfriend Apr 18, 2024

bearfriend Apr 16, 2024

bearfriend Apr 17, 2024

bearfriend Apr 17, 2024 •

edited

dbatiste Apr 18, 2024

dbatiste Apr 18, 2024

Fix tooltips used on targets with delegated focus #4606

Are you sure you want to change the base?

Fix tooltips used on targets with delegated focus #4606

Conversation

bearfriend commented Apr 16, 2024 • edited by jira bot

github-actions bot commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

dlockhart Apr 17, 2024 • edited

Choose a reason for hiding this comment

bearfriend Apr 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bearfriend Apr 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bearfriend commented Apr 16, 2024 •

edited by jira bot

dlockhart Apr 17, 2024 •

edited

bearfriend Apr 17, 2024 •

edited

bearfriend Apr 17, 2024 •

edited