Closed Bug 699017 Opened 9 years ago Closed 9 years ago

aria-required attribute on file input not read by JAWS

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: damian.chojna, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: jaws
Component: General → Disability Access APIs
Product: Firefox → Core
QA Contact: general → accessibility-apis
Version: 3.6 Branch → Trunk
Blocks: aria
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #575163 - Flags: review?(trev.saunders)
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.
(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?
"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.
(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
Attachment #575163 - Flags: review?(trev.saunders)
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.
Attached patch patch2 (obsolete) — Splinter Review
Attachment #575163 - Attachment is obsolete: true
Attachment #575191 - Flags: review?(trev.saunders)
Attachment #575191 - Flags: review?(marco.zehe)
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!
Attachment #575191 - Flags: review?(marco.zehe) → review+
(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 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>
>
Another question, what do we want to do about state change events here?
(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.
(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?
(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.
Attachment #575191 - Flags: review?(trev.saunders) → review+
Attached patch patch3Splinter Review
+ fire events
Attachment #575191 - Attachment is obsolete: true
Attachment #577212 - Flags: review?(trev.saunders)
Attachment #577212 - Flags: review?(marco.zehe)
Comment on attachment 577212 [details] [diff] [review]
patch3

additional review from Robert for layout part.
Attachment #577212 - Flags: review?(roc)
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.
Attachment #577212 - Flags: review?(marco.zehe) → review+
Robert, the layout part is trivial, just making fileinput frame to create an accessible of different type.
Comment on attachment 577212 [details] [diff] [review]
patch3

r=me with  comment per irc
Attachment #577212 - Flags: review?(trev.saunders) → review+
https://hg.mozilla.org/mozilla-central/rev/247c27bf0888
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Flags: in-litmus+
Flags: in-litmus+ → in-testsuite+
You need to log in before you can comment on or make changes to this bug.