699017 - aria-required attribute on file input not read by JAWS

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Damian]

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)]

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 (:asurkov)]

Attachment: patch

Comment 3

[Marco Zehe (:MarcoZ)]

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 (:asurkov)]

(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)]

"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 (:asurkov)]

(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] (NeedInfo me for attention)]

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 (:asurkov)]

Attachment: patch2

Comment 9

[Marco Zehe (:MarcoZ)]

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)]

(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)]

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)]

Another question, what do we want to do about state change events here?

Comment 13

[alexander :surkov (:asurkov)]

(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 (:asurkov)]

(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)]

(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 (:asurkov)]

Attachment: patch3

+ fire events

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 577212 [details] [diff] [review]
patch3

additional review from Robert for layout part.

Comment 18

[Marco Zehe (:MarcoZ)]

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 (:asurkov)]

Robert, the layout part is trivial, just making fileinput frame to create an accessible of different type.

Comment 20

[Trevor Saunders (:tbsaunde)]

Comment on attachment 577212 [details] [diff] [review]
patch3

r=me with  comment per irc

Comment 21

[alexander :surkov (:asurkov)]

inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/247c27bf0888

Comment 22

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/247c27bf0888