Last Comment Bug 699017 - aria-required attribute on file input not read by JAWS
: aria-required attribute on file input not read by JAWS
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: aria jaws
  Show dependency treegraph
 
Reported: 2011-11-02 04:32 PDT by Damian
Modified: 2011-12-01 05:19 PST (History)
9 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.67 KB, patch)
2011-11-17 06:26 PST, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (6.50 KB, patch)
2011-11-17 07:59 PST, alexander :surkov
tbsaunde+mozbugs: review+
mzehe: review+
Details | Diff | Splinter Review
patch3 (24.76 KB, patch)
2011-11-28 03:07 PST, alexander :surkov
mzehe: review+
tbsaunde+mozbugs: review+
roc: review+
Details | Diff | Splinter Review

Description Damian 2011-11-02 04:32:42 PDT
User Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.23) Gecko/20110920 Firefox/3.6.23
Build ID: 20110920075126

Steps to reproduce:

When applying aria-required="true" on a file input, JAWS does not read the required attribute. 
E.g.
<input type="file" aria-required="true"></input>


Actual results:

The aria-required attribute is ignored when tabbing onto the File browse button


Expected results:

JAWS should have detected and read that the input field is required.
Comment 1 Marco Zehe (:MarcoZ) on PTO until August 15 2011-11-17 05:37:21 PST
Also confirmed with NVDA. The text frame parent of the read-only textbox and the button does get the required state, but the child elements don't.
Comment 2 alexander :surkov 2011-11-17 06:26:18 PST
Created attachment 575163 [details] [diff] [review]
patch
Comment 3 Marco Zehe (:MarcoZ) on PTO until August 15 2011-11-17 06:36:25 PST
Comment on attachment 575163 [details] [diff] [review]
patch

Question, do you also want to check if the button gets these states? After all that's the one that gets focus, not the read-only textfield.
Comment 4 alexander :surkov 2011-11-17 06:47:41 PST
(In reply to Marco Zehe (:MarcoZ) from comment #3)
> Comment on attachment 575163 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Question, do you also want to check if the button gets these states? After
> all that's the one that gets focus, not the read-only textfield.

I'm not sure about button what would required or invalid states mean on it?
Comment 5 Marco Zehe (:MarcoZ) on PTO until August 15 2011-11-17 06:53:34 PST
"required" would indicate to the user that they absolutely have to select a file here. When tabbing, not when navigating with the virtual cursor, this information would not be spoken otherwise. So if we expose "required" or the other universal states on the button, we're OK, but I think we should cover that in the test.
Comment 6 alexander :surkov 2011-11-17 06:55:50 PST
(In reply to alexander surkov from comment #4)
> (In reply to Marco Zehe (:MarcoZ) from comment #3)
> > Comment on attachment 575163 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch
> > 
> > Question, do you also want to check if the button gets these states? After
> > all that's the one that gets focus, not the read-only textfield.
> 
> I'm not sure about button what would required or invalid states mean on it?

but yes, if the button is what user sees and it's treated as file control then yes, you're right
Comment 7 David Bolter [:davidb] 2011-11-17 07:13:45 PST
Comment on attachment 575163 [details] [diff] [review]
patch

Review of attachment 575163 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/nsAccessible.cpp
@@ +1600,5 @@
>  void
>  nsAccessible::ApplyARIAState(PRUint64* aState)
>  {
>    // Test for universal states first
> +  *aState |= nsARIAMap::UniversalStatesFor(mContent);

Happy to see this logic move to nsARIAMap. Nice.
Comment 8 alexander :surkov 2011-11-17 07:59:45 PST
Created attachment 575191 [details] [diff] [review]
patch2
Comment 9 Marco Zehe (:MarcoZ) on PTO until August 15 2011-11-17 08:17:46 PST
Comment on attachment 575191 [details] [diff] [review]
patch2

r=me, but please add a link to this bug to
>diff --git a/accessible/tests/mochitest/states/test_aria.html 

Thanks!
Comment 10 Trevor Saunders (:tbsaunde) 2011-11-20 02:57:47 PST
(In reply to Marco Zehe (:MarcoZ) from comment #5)
> "required" would indicate to the user that they absolutely have to select a
> file here. When tabbing, not when navigating with the virtual cursor, this
> information would not be spoken otherwise. So if we expose "required" or the
> other universal states on the button, we're OK, but I think we should cover
> that in the test.

There's a part of me that says that the At should look at the whole widget to decide if it is required, and so they shouldn't need us to expose these states on the button itself so long as its parent has them.  On the other hand all the accessibles are conceptually the same "widget" so if one of them is required it sort of makes sense that  all the accessibles have the required state.
Comment 11 Trevor Saunders (:tbsaunde) 2011-11-20 03:04:49 PST
Comment on attachment 575191 [details] [diff] [review]
patch2

>+    while (nsStateMapEntry::MapToStates(aContent, &state, gWAIUnivStateMap[index]))
>+      ++ index;

++ index is weird I'd prefer index++ but don't really mind ++index.

>+  // Inherit ARIA states from input@type="file" suitable for the text field.
>+  if (mContent->IsInNativeAnonymousSubtree()) {
>+    nsIContent* parentContent = mContent->GetParent();
>+    if (parentContent && parentContent->IsHTML() &&
>+        parentContent->Tag() == nsGkAtoms::input) {
>+      *aState |= nsARIAMap::UniversalStatesFor(parentContent);
>+      return;
>+    }
>+  }

to state the obvious I'm not a big fan of copying code, but don't have a better idea for this.

>       // disabled, too. See bug 429285.
>       testAriaDisabledTree("group");
> 
>+      // universal ARIA properties inherited from file input control
>+      var fileTextField = getAccessible("fileinput").firstChild;
>+      testStates(fileTextField,
>+                 STATE_BUSY | STATE_UNAVAILABLE | STATE_REQUIRED | STATE_HASPOPUP | STATE_INVALID);
>+      var fileBrowseButton = getAccessible("fileinput").lastChild;
>+      testStates(fileBrowseButton,
>+                 STATE_BUSY | STATE_UNAVAILABLE | STATE_REQUIRED | STATE_HASPOPUP | STATE_INVALID);
>+
>       // offscreen test
>       testStates("aria_offscreen_textbox", STATE_OFFSCREEN);
> 
>       //
>       // This section tests aria roles on links/anchors for underlying
>       // nsHTMLLinkAccessible creation. (see closed bug 494807)
>       //
> 
>@@ -210,17 +218,25 @@
>     <div tabindex="0" role="listbox" aria-activedescendant="item1">
>       <div role="option" id="item1">Item 1</div>
>       <div role="option" id="item2">Item 2</div>
>       <div role="option" id="item3">Item 3</div>
>       <div role="option" id="item4">Item 4</div>
>     </div>
>     <div role="slider" tabindex="0">A slider</div>
>   </div>
>-  
>+
>+  <!-- universal ARIA properties should be inherited by text field of file input -->
>+  <input type="file" id="fileinput"
>+         aria-busy="true"
>+         aria-disabled="true"
>+         aria-required="true"
>+         aria-haspopup="true"
>+         aria-invalid="true">
>+
>   <div id="offscreen_log" role="log" class="offscreen">
>     <div id="aria_offscreen_textbox" role="textbox" aria-readonly="true">This text should be offscreen</div>
>   </div>
> 
>   <a id="aria_menuitem_link" role="menuitem" href="foo">menuitem</a>
>   <a id="aria_button_link" role="button" href="foo">button</a>
>   <a id="aria_checkbox_link" role="checkbox" href="foo">checkbox</a>
>
Comment 12 Trevor Saunders (:tbsaunde) 2011-11-20 04:57:57 PST
Another question, what do we want to do about state change events here?
Comment 13 alexander :surkov 2011-11-21 06:42:01 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> (In reply to Marco Zehe (:MarcoZ) from comment #5)
> > "required" would indicate to the user that they absolutely have to select a
> > file here. When tabbing, not when navigating with the virtual cursor, this
> > information would not be spoken otherwise. So if we expose "required" or the
> > other universal states on the button, we're OK, but I think we should cover
> > that in the test.
> 
> There's a part of me that says that the At should look at the whole widget
> to decide if it is required, and so they shouldn't need us to expose these
> states on the button itself so long as its parent has them.  On the other
> hand all the accessibles are conceptually the same "widget" so if one of
> them is required it sort of makes sense that  all the accessibles have the
> required state.

that's right but this case is special since no standard way for AT to say this is a widget, they could deal with upload button only.

(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> Another question, what do we want to do about state change events here?

good point, I'm tempting to say we should fire events for every underlying control in this case.
Comment 14 alexander :surkov 2011-11-21 06:58:06 PST
(In reply to alexander surkov from comment #13)

> (In reply to Trevor Saunders (:tbsaunde) from comment #12)
> > Another question, what do we want to do about state change events here?
> 
> good point, I'm tempting to say we should fire events for every underlying
> control in this case.

create class for file input control and make it fire event on descendants when it handles statechange event sounds ok?
Comment 15 Trevor Saunders (:tbsaunde) 2011-11-23 22:20:42 PST
(In reply to alexander surkov from comment #14)
> (In reply to alexander surkov from comment #13)
> 
> > (In reply to Trevor Saunders (:tbsaunde) from comment #12)
> > > Another question, what do we want to do about state change events here?
> > 
> > good point, I'm tempting to say we should fire events for every underlying
> > control in this case.
> 
> create class for file input control and make it fire event on descendants
> when it handles statechange event sounds ok?

I have no immediate objection to that.
Comment 16 alexander :surkov 2011-11-28 03:07:05 PST
Created attachment 577212 [details] [diff] [review]
patch3

+ fire events
Comment 17 alexander :surkov 2011-11-28 03:07:54 PST
Comment on attachment 577212 [details] [diff] [review]
patch3

additional review from Robert for layout part.
Comment 18 Marco Zehe (:MarcoZ) on PTO until August 15 2011-11-28 06:27:56 PST
Comment on attachment 577212 [details] [diff] [review]
patch3

r=me with these nits:
>+      this.invoke = function stateChangeOnFileInput_inovke()

Not important for functionality, but please correct spelling of "_invoke".

Also, there's still uncommented debug statement just below this invoker in the same file.
Comment 19 alexander :surkov 2011-11-29 21:15:13 PST
Robert, the layout part is trivial, just making fileinput frame to create an accessible of different type.
Comment 20 Trevor Saunders (:tbsaunde) 2011-11-29 22:38:27 PST
Comment on attachment 577212 [details] [diff] [review]
patch3

r=me with  comment per irc
Comment 21 alexander :surkov 2011-11-30 04:37:48 PST
inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/247c27bf0888
Comment 22 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-01 04:00:05 PST
https://hg.mozilla.org/mozilla-central/rev/247c27bf0888

Note You need to log in before you can comment on or make changes to this bug.