Merged
Conversation
radium-v
changed the title
radium-v
force-pushed
the
users/jokreitl/use-floatingui-for-tooltip
branch
from
c7c1671 to
a0c8649
Compare
radium-v
changed the base branch from
users/jokreitl/use-floatingui-for-select
to
master
radium-v
requested review from
EisenbergEffect,
chrisdholt,
janechu and
nicholasrice
as code owners
radium-v
force-pushed
the
users/jokreitl/use-floatingui-for-tooltip
branch
from
a0c8649 to
7573023
Compare
📊 Tachometer Benchmark ResultsSummaryclickTrigger10x
create10k
createDelete5x
runFile1k
update10th
usedJSHeapSize
Resultsobservable-runFile1k
runFile1k
usedJSHeapSize
render-create10k
create10k
usedJSHeapSize
render-createDelete5x
createDelete5x
usedJSHeapSize
render-update10th
update10th
usedJSHeapSize
repeat-basic-splice-itemCount=1000&deleteCount=20&addCount=20
clickTrigger10x
usedJSHeapSize
repeat-nested-push-itemCount=100&addCount=20
clickTrigger10x
usedJSHeapSize
repeat-nested-reverse-itemCount=100
clickTrigger10x
usedJSHeapSize
repeat-nested-shift-itemCount=100
clickTrigger10x
usedJSHeapSize
repeat-nested-unshift-itemCount=100&addCount=20
clickTrigger10x
usedJSHeapSize
|
radium-v
force-pushed
the
users/jokreitl/use-floatingui-for-tooltip
branch
3 times, most recently
from
836da95 to
b047c0a
Compare
EisenbergEffect
approved these changes
Nov 4, 2022
Contributor
EisenbergEffect
left a comment
EisenbergEffect
left a comment
There was a problem hiding this comment.
Looks much simpler. I agree, I'm not sure about patching id in this way...but if it's working cross-browser and there are no a11y issues then it's probably ok. I've seen much crazier things done.
radium-v
force-pushed
the
users/jokreitl/use-floatingui-for-tooltip
branch
from
b047c0a to
5b20318
Compare
Contributor
|
chrisdholt
reviewed
Nov 23, 2022
Comment on lines
+35
to
+37
| <fast-button id="${x => x.anchor}"> | ||
| ${x => x.buttonContent ?? "Hover or focus me"} | ||
| </fast-button> |
Member
There was a problem hiding this comment.
This is a good example as I think it illustrates an issue we currently have with cross-root ARIA for Button and similar components which delegate focus. While the tooltip shows up, it's not announced, which means that (to my knowledge) aria-describedby isn't working since the two id's are in separate DOM's.
Member
There was a problem hiding this comment.
TLDR, we need to review if we either can figure out a way to manage this issue or if we should shift how we approach components which currently delegate focus.
radium-v
force-pushed
the
users/jokreitl/use-floatingui-for-tooltip
branch
from
5b20318 to
64e288c
Compare
Member
Thinking about 2 above more, I think we likely should avoid forcing non-focusable elements to be focusable - in many cases, if the tooltip is applied to something like an image, or other non-interactive element, the tabindex would create an issue. Further, focus isn't required to have tooltips read out, it's simply a common pattern to add them to interactive elements. In the case an element should be focusable and it's not, I think it's probably best to leave that to authors. |
chrisdholt
approved these changes
Dec 9, 2022
radium-v
force-pushed
the
users/jokreitl/use-floatingui-for-tooltip
branch
from
64e288c to
6a3820c
Compare
Contributor
@chrisdholt you mind sharing more on why "focus isn't required to have tooltips read out"? can tooltips still get picked up by ATs then? |
janechu
pushed a commit
that referenced
this pull request
Jun 10, 2024
* add nullableBooleanConverter for attributes * remove tooltip options from dom-shim spec * rewrite tooltip to use floating-ui * Change files
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
📖 Description
Tooltipcomponent to use Floating UI.aria-describedbysupport. Once the anchor element is resolved, thearia-describedbyattribute will be added to it to match the tooltip'sid. If the anchor changes, theidwill be removed from the old anchor'saria-describedbyattribute and added to the new anchor.positionattribute withplacementto match the naming convention used by Floating UI.showattribute to control the visibility of the tooltip.delayattribute as it's no longer needed - tooltip transitions can be controlled via CSS.nullalbleBooleanConverterto@microsoft/fast-elementfor attributes that aretrue,false, orundefined🎫 Issues
👩💻 Reviewer Notes
The
idproperty is now decorated with the@observabledecorator, to allow the component to update thearia-describedbyattribute accordingly. Since this is a built-in property, I don't know if or how that may be treated in comparison to a custom property.📑 Test Plan
All tests for the tooltip component should pass as expected.
✅ Checklist
General
$ yarn changeComponent-specific