Tooltip: Add tooltip content to an aria-live div when the tooltip conten... #1118
Conversation
|
@scottgonzalez - not sure if you noticed that I changed the pull request due to merge issues |
| @@ -244,6 +254,8 @@ $.widget( "ui.tooltip", { | |||
| tooltip = this._tooltip( target ); | |||
| this._addDescribedBy( target, tooltip.attr( "id" ) ); | |||
| tooltip.find( ".ui-tooltip-content" ).html( content ); | |||
| this.liveRegion.find( "p" ).hide(); // stop duplicate announcement on VO (OSX) | |||
There was a problem hiding this comment.
Line comments need to go above the commented line, with a line break before them. First character in the comment must be uppercase. There's another one below.
|
Apart from the line comments the code looks good. @tjvantoll could you test this again? |
|
I tested this out in VoiceOver, JAWS, and NVDA. VO and JAWS now read the dynamic content perfectly. NVDA however exhibited some odd behavior. The tooltip was not immediately read, then NVDA started reading the code for the @dylanb Did you run into this at all in NVDA? I'm using your test case verbatim from #9610. Edit: I'm not able to recreate NVDA reading strange text; it must've been me multitasking. However, NVDA still does not read the tooltips for me after this fix. |
|
@tjvantoll I did test with NVDA - I always test NVDA with FF and JAWS with IE. Other combos are not reliable - what combos were you testing? |
|
Ah ok. I was testing NVDA with IE. You're right this works fine in Firefox. After the comments are fixed this looks good to land to me. |
|
comments fixed |
| "aria-relevant": "additions" | ||
| }) | ||
| .addClass( "ui-tooltip-polite ui-helper-hidden-accessible" ); | ||
| $( "body", this.document ).append( this.liveRegion ); |
There was a problem hiding this comment.
This should be chained with the rest of the code using .appendTo().
|
Thanks @dylanb, we're getting closer. There are a few minor issues which I was going to clean up and land, but then I realized that some of the functionality is still untested. Do you want to address these issues? |
| "aria-relevant": "additions" | ||
| }) | ||
| .addClass( "ui-helper-hidden-accessible" ) | ||
| .appendTo( this.document.find( "body" ) ); |
There was a problem hiding this comment.
No need to query the document. Use this.document[ 0 ].body.
|
I will take a look at these changes shortly |
|
@scottgonzalez committed changes for the requested changes |
|
@dylanb We're still waiting on an update for the HTML/text issue. Is this still on your list or do you want someone else to finish the changes? |
|
I apologize, I thought I had already made that change. It has now been committed. |
| deepEqual( $( "#" + element.data( "ui-tooltip-id" ) ).text(), "customstring" ); | ||
| equal( liveRegion.children().last().html().toString(), "<div>cu<b>s</b>tomstring</div>" ); |
There was a problem hiding this comment.
.html() always returns a string.
|
Thanks. This looks much better. Just a few minor updates and we'll be ready to finally land this. |
…tent changes to allow support for all ATs. Fixes #9610. Tooltip announcements don't work in all browsers and AT combinations.
|
ok, changes committed |
|
Thanks for working through all of that. Do you want to review the use of live regions for autocomplete since we probably need to use the hiding/additions trick there as well? |
|
I will take a look at the autocomplete widget |
...t changes to allow support for all ATs. Fixes #9610. Tooltip announcements don't work in all browsers and AT combinations.