The default bug view has changed. See this FAQ.

aria-required attribute on file input not read by JAWS

RESOLVED FIXED in mozilla11

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Damian, Assigned: surkov)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 617918
(Assignee)

Updated

5 years ago
Component: General → Disability Access APIs
Product: Firefox → Core
QA Contact: general → accessibility-apis
Version: 3.6 Branch → Trunk
(Assignee)

Updated

5 years ago
Blocks: 343213
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 575163 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #575163 - Flags: review?(trev.saunders)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
(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

5 years ago
"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.
(Assignee)

Comment 6

5 years ago
(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
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 8

5 years ago
Created attachment 575191 [details] [diff] [review]
patch2
Attachment #575163 - Attachment is obsolete: true
Attachment #575191 - Flags: review?(trev.saunders)
Attachment #575191 - Flags: review?(marco.zehe)

Comment 9

5 years ago
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?
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
(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+
(Assignee)

Comment 16

5 years ago
Created attachment 577212 [details] [diff] [review]
patch3

+ fire events
Attachment #575191 - Attachment is obsolete: true
Attachment #577212 - Flags: review?(trev.saunders)
Attachment #577212 - Flags: review?(marco.zehe)
(Assignee)

Comment 17

5 years ago
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+
(Assignee)

Comment 19

5 years ago
Robert, the layout part is trivial, just making fileinput frame to create an accessible of different type.
Attachment #577212 - Flags: review?(roc) → review+
Comment on attachment 577212 [details] [diff] [review]
patch3

r=me with  comment per irc
Attachment #577212 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 21

5 years ago
inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/247c27bf0888
https://hg.mozilla.org/mozilla-central/rev/247c27bf0888
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(Assignee)

Updated

5 years ago
Flags: in-litmus+
(Assignee)

Updated

5 years ago
Flags: in-litmus+ → in-testsuite+
You need to log in before you can comment on or make changes to this bug.