Skip to content

Conversation

@dylanb
Copy link
Contributor

@dylanb dylanb commented Oct 26, 2013

...t changes to allow support for all ATs. Fixes #9610. Tooltip announcements don't work in all browsers and AT combinations.

@dylanb
Copy link
Contributor Author

dylanb commented Oct 27, 2013

@scottgonzalez - not sure if you noticed that I changed the pull request due to merge issues

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will correct

@jzaefferer
Copy link
Member

Apart from the line comments the code looks good. @tjvantoll could you test this again?

@tjvantoll
Copy link
Member

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 content option, literally "function callback setTimeout".

@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.

@dylanb
Copy link
Contributor Author

dylanb commented Oct 31, 2013

@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?

@tjvantoll
Copy link
Member

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.

@dylanb
Copy link
Contributor Author

dylanb commented Oct 31, 2013

comments fixed

Copy link
Member

Choose a reason for hiding this comment

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

This should be chained with the rest of the code using .appendTo().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@scottgonzalez
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

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

No need to query the document. Use this.document[ 0 ].body.

@dylanb
Copy link
Contributor Author

dylanb commented Dec 8, 2013

I will take a look at these changes shortly

@dylanb
Copy link
Contributor Author

dylanb commented Dec 8, 2013

@scottgonzalez committed changes for the requested changes

@scottgonzalez
Copy link
Member

@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?

@dylanb
Copy link
Contributor Author

dylanb commented Dec 19, 2013

I apologize, I thought I had already made that change. It has now been committed.

Copy link
Member

Choose a reason for hiding this comment

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

.html() always returns a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

@scottgonzalez
Copy link
Member

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.
@dylanb
Copy link
Contributor Author

dylanb commented Dec 19, 2013

ok, changes committed

@scottgonzalez
Copy link
Member

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?

@dylanb
Copy link
Contributor Author

dylanb commented Dec 19, 2013

I will take a look at the autocomplete widget

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants