Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#16307 closed defect (fixed)

[PATCH][CCLA] dijit.TooltipDialog: invalid aria role and missing aria attributes

Reported by: mikeb Owned by: mikeb
Priority: undecided Milestone: 1.8.4
Component: Dijit Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

Dijit.TooltipDialog has the role of dialog. However, I think that role=alertdialog is more appropriate because "Unlike alert, alertdialog can receive a response from the user", which is what TooltipDialog? is for (form elements.)

Aside from changing the role, we also need to have aria-labelledby attribute. This gets set from _HasDropDown on either the dropDown.containerNode (for FiltereingSelect and ComboBox) or dropDown.domNode (for Select, TabContainer, DropDownButton, DateTextBox, and TooltipDialog.) We have to have a few checks to not overwrite the current aria-labelledby (if there is one, like on DateTimeTextBox) and not to apply it if there is a role=presentation.

This should be enough to correctly set aria-labelledby on any dijit that is used as the dropDown, since dijits set up their aria roles and properties in either the containerNode or domNode. Since there is no "role" attachPoint for all of the dijits I think we need this conditional instead of just being able to set aria-labelledby for all dijits (like we did on #15885 with _popupStateNode).

As part of this change I needed to move the role=listbox of the Select up one level to the domNode. This doesn't affect anything with Select since aria-owns gets set to the domNode anyway.

Attachments (6)

TooltipDialog_aria_labelledby.patch (7.8 KB) - added by mikeb 7 years ago.
change role of TooltipDialog? to alertdialog, set aria-labelledby on dropDown dijit in _hasDropDown, please proxy commit for michael billau CCLA on file with IBM
TooltipDialog_aria_labelledby-2.patch (22.5 KB) - added by mikeb 7 years ago.
Change role of TooltipDialog to alertdialog, add aria-labellledby to the popup of _hasDropDown, add automated test cases for all the dijits that use _hasDropDown, please proxy commit for Michael Billau, CCLA on file with IBM
TooltipDialog_aria_labelledby-3.patch (23.3 KB) - added by bill 7 years ago.
Updated patch, removing strange changes in spacing, etc. Can't checkin though until regression from #15907 fixed.
TooltipDialog_aria_labelledby-4.patch (23.4 KB) - added by bill 7 years ago.
TooltipDialog_aria_labelledby-5.patch (23.2 KB) - added by mikeb 7 years ago.
Updated patch file to include fix to form/robot/Select.html
16307-backport.patch (30.0 KB) - added by mikeb 7 years ago.
Backport TooltipDialog? accessibility fixes to 1.8, please proxy commit for Michael Billau

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by mikeb

change role of TooltipDialog? to alertdialog, set aria-labelledby on dropDown dijit in _hasDropDown, please proxy commit for michael billau CCLA on file with IBM

comment:1 Changed 7 years ago by bill

Description: modified (diff)
Owner: changed from bill to mikeb
Status: newpending

Hmm, why do you have to sometimes set aria-labelledby on the popup's containerNode, but sometimes on the domNode? Why not always on the domNode? And why only check if aria-labelledby is already set in one of those cases?

The other thing is that you need to add automated tests for this functionality; you can't just add code to _HasDropDown without any (automatic) verification. It may be sufficient to add new checks to the existing tests.

comment:2 Changed 7 years ago by mikeb

Status: pendingnew

Hi Bill,

After looking at this for a little while longer, I'm not really sure why I even had the first if clause. We should always be setting aria-labelledby on the popup's domNode unless there is already an existing labelledby (if the popup is a Calendar, for instance.) I must have been working off a bad branch.

I did have to make one change to the role of another dijit: needed to move the role of the colorPalette up to the domNode instead of the table. I created #16353 for that.

I added a bunch of automated test cases. I'm pretty sure I hit all of the dijits that use _hasDropDown. I verified that the test cases all correctly report a failure when there is one.

Changed 7 years ago by mikeb

Change role of TooltipDialog to alertdialog, add aria-labellledby to the popup of _hasDropDown, add automated test cases for all the dijits that use _hasDropDown, please proxy commit for Michael Billau, CCLA on file with IBM

comment:3 Changed 7 years ago by bill

Milestone: tbd1.9

Thanks for all the tests.

About the code, if a dropdown were ever moved from one anchor to another, the aria-labelledby would continue to point to the first anchor, because of the && !this.dropDown.domNode.hasAttribute("aria-labelledby") condition, and because the aria-labelledby set by the first anchor never gets erased. Hopefully that will never happen in practice though.

Changed 7 years ago by bill

Updated patch, removing strange changes in spacing, etc. Can't checkin though until regression from #15907 fixed.

comment:4 Changed 7 years ago by bill

Also, you can't use hasAttribute() because it isn't supported in IE6, IE7, and quirks mode. I'll update it to just use getAttribute().

comment:5 Changed 7 years ago by bill

Status: newpending

Mike - After this patch (presumably the change to Select.js), I'm getting a failure in the "two clicks to select" test from form/robot/Select.html, on the line:

doh.is("listbox", s1_menu.domNode.firstChild.getAttribute('role'), 'menu role');

where the value is "presentation" instead of "listbox". Aren't you getting that failure too? And if so, how to fix it?

Changed 7 years ago by bill

comment:6 Changed 7 years ago by mikeb

Status: pendingnew

Hi Bill. I was also failing that test case but fixed it locally; apparently the fix didn't make it into the patch. I was also having troubles applying patch #4, the diff editor detected there were differences but wasn't displaying them, so maybe my environment is screwed up. Anyway I fixed the test case by looking at dropDown.domNode instead of dropDown.domNode.firstChild, since we moved the role up to the domNode. I generated the new patch on top of yours.

Changed 7 years ago by mikeb

Updated patch file to include fix to form/robot/Select.html

comment:7 Changed 7 years ago by bill

Ah OK that's simple, thanks.

comment:8 Changed 7 years ago by bill

Resolution: fixed
Status: newclosed

In [30053]:

Aria related fixes from Mike Billau (IBM, CCLA):

  • set aria-labelledby for popups
  • move listbox role of Select widget up to its root node
  • change TooltipDialog to be role alertdialog rather than dialog, and move that role up to its root node
  • automated tests for aria roles and states
  • other miscellaneous test updates


Fixes #16307 !strict.

comment:9 Changed 7 years ago by mikeb

Hello. Would it be possible to backport this to 1.8 branch? I created a patch and tested it. Some of the automated test files needed to be changed since we cannot backport #16242, #16244, or [30088]

  • tests/form/_autoCompleter.html - needed to comment out two test lines that check for a wrapper node to have role="region", since we can't backport [29904] (this is a big accessibility but, it really stinks we can't get it into 1.8)
  • tests/form/test_DateTextBox.html - This page has some additional tests that did not get backported to 1.8, so I added in the test in the best place. However, for some reason the test is not failing after the call to button.openDropDown() - it always passes. (It fails correctly on the nightly build.)
  • tests/layout/robot/TabContainer_a11y.html - needed to comment out two tests that check for role=tabpanel and aria-labelledby, from changeset [30088] that we can't backport.

Changed 7 years ago by mikeb

Attachment: 16307-backport.patch added

Backport TooltipDialog? accessibility fixes to 1.8, please proxy commit for Michael Billau

comment:10 Changed 7 years ago by bill

Hmm, so this ticket is about many widgets, not just TooltipDialog. The summary is misleading.

That 16307-backport.patch is confusing to me:

  • The change to _HasDropDown.js seems fine although there are spurious spacing changes.
  • _PaletteMixin's change to remove the summary attribute was already done in [30315].
  • The other change to _PaletteMixin in your patch was not in [30315].
  • Why is there a change to ColorPalette? That's not in [30053] either.

Version 0, edited 7 years ago by bill (next)

comment:11 Changed 7 years ago by Douglas Hays

In [30316]:

Refs #16307. Backport [30053] to 1.8 except for tests/TooltipDialog.html and tests/form/test_DateTextBox.html which require additional changesets which are not needed in 1.8.

comment:12 Changed 7 years ago by Douglas Hays

Milestone: 1.91.8.4

comment:13 Changed 7 years ago by bill

In [30482]:

Since the StackContainer wrapper changes from [29904] were (intentionally) not backported to 1.8, this part of the test changes from [30053] cannot be backported either, refs #16307, fixes #16631.

comment:14 Changed 4 years ago by Bill Keese <bill@…>

In c35b9f6461f4ab18d04cab5b744af964df1da5e3/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.