847233 - Fix a11y with <input type='file'> changes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 1

[alexander :surkov (:asurkov)]

Comment on attachment 720484 [details] [diff] [review]
Patch

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

I don't think we really need to propogate states to readonly text inside input@type=file, since it's not focusable and interactive.

Marco, Jamie, agree?

::: accessible/src/html/HTMLFormControlAccessible.cpp
@@ +556,2 @@
>        nsEventShell::FireEvent(childEvent);
>      }

I'd say to skip events firing on label accessible

::: accessible/tests/mochitest/events/test_statechange.html
@@ +77,5 @@
>      {
>        this.fileControlNode = getNode(aID);
>        this.fileControl = getAccessible(this.fileControlNode);
> +      this.browseButton = this.fileControl.firstChild;
> +      this.textEntry = this.fileControl.lastChild;

textEntry is an input, makes sense to rename to 'text' probably

::: accessible/tests/mochitest/tree/test_filectrl.html
@@ +26,3 @@
>            },
>            {
> +            role: ROLE_LABEL

pls, add

children: [
  {
    role: TEXT_LEAF
  }
];

::: layout/forms/nsFileControlFrame.cpp
@@ +180,1 @@
>    aElements.MaybeAppendElement(mTextContent);

curious, why this change is necessary?

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to alexander :surkov from comment #1)
> ::: layout/forms/nsFileControlFrame.cpp
> @@ +180,1 @@
> >    aElements.MaybeAppendElement(mTextContent);
> 
> curious, why this change is necessary?

The Accessible Tree seems to be set based on that and I was adding the text before the button while the button is actually shown *before* the text so I had an accessible tree being inverted (ie. test_filectrl.html was failing).

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Mounir Lamouri (:mounir) from comment #2)
> (In reply to alexander :surkov from comment #1)
> > ::: layout/forms/nsFileControlFrame.cpp
> > @@ +180,1 @@
> > >    aElements.MaybeAppendElement(mTextContent);
> > 
> > curious, why this change is necessary?
> 
> The Accessible Tree seems to be set based on that and I was adding the text
> before the button while the button is actually shown *before* the text so I
> had an accessible tree being inverted (ie. test_filectrl.html was failing).

oh, I see, native anonymous content array should correspond to visual order.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 720484 [details] [diff] [review]
Patch

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

This looks pretty good. Alexander do we still needinfo on Jamie?

::: accessible/src/html/HTMLFormControlAccessible.cpp
@@ +556,2 @@
>        nsEventShell::FireEvent(childEvent);
>      }

Agreed no need to fire for label AFAIK.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #4)

> This looks pretty good. Alexander do we still needinfo on Jamie?

I need someone to confirm the statement about states propogation from comment #1. Do you do that?

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander :surkov from comment #5)
> (In reply to David Bolter [:davidb] from comment #4)
> 
> > This looks pretty good. Alexander do we still needinfo on Jamie?
> 
> I need someone to confirm the statement about states propogation from
> comment #1. Do you do that?

I can try. Can you elaborate the statement? What part of the patch is it referring to?

Comment 7

[alexander :surkov (:asurkov)]

The statement was that the states like busy, invalid, etc shouldn't be propagated from input@type=file to its child text accessible. It's referring to XULLabelAccessible::State().

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 720484 [details] [diff] [review]
Patch

cancelling until comments are addressed

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 720484 [details] [diff] [review]
Patch

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

Sorry for the delay. Let's get a try build to Marco.

::: accessible/src/xul/XULElementAccessibles.cpp
@@ +99,5 @@
> +  // Inherit states from input@type="file" suitable for the <xul:label>.
> +  if (mParent && mParent->IsHTMLFileInput()) {
> +    uint64_t parentState = mParent->State();
> +    state |= parentState & (states::UNAVAILABLE | states::BUSY |
> +             states::REQUIRED | states::HASPOPUP | states::INVALID);

I think you can remove this block and you're probably good to go.

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to David Bolter [:davidb] from comment #9)
> Comment on attachment 720484 [details] [diff] [review]
> Patch
> 
> Review of attachment 720484 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay. Let's get a try build to Marco.
> 
> ::: accessible/src/xul/XULElementAccessibles.cpp
> @@ +99,5 @@
> > +  // Inherit states from input@type="file" suitable for the <xul:label>.
> > +  if (mParent && mParent->IsHTMLFileInput()) {
> > +    uint64_t parentState = mParent->State();
> > +    state |= parentState & (states::UNAVAILABLE | states::BUSY |
> > +             states::REQUIRED | states::HASPOPUP | states::INVALID);
> 
> I think you can remove this block and you're probably good to go.

Tests would fail without that. Should I fix the tests then?

Comment 11

[Mounir Lamouri (:mounir)]

So, do you guys want me to not propagate the state and not send any events and fix the tests accordingly? (tests will be failing for sure, this patch is the minimum code to make tests pass)

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Which tests fail if you don't propagate state or fire events for the label?

Personally I think I'd be okay with you landing this so that you can move on and we can do the followup. The states and event firing for the label simply seems unnecessary to me but should be harmless.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Mounir Lamouri (:mounir) from comment #11)
> So, do you guys want me to not propagate the state and not send any events
> and fix the tests accordingly? (tests will be failing for sure, this patch
> is the minimum code to make tests pass)

yes

Comment 14

[James Teh [:Jamie]]

Is input still required from me? I was never clear on what input I was supposed to provide. :)

Comment 15

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #14)
> Is input still required from me? I was never clear on what input I was
> supposed to provide. :)

Jamie, we used to propagate states to entry inside input@type=file for example:
<input type="file" aria-required="true" made child entry to have required state too. Since the entry was replaced to not editable/not focusable hypertext then I think we don't need to propagate states anymore. Do you agree?

Comment 16

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #15)
> Jamie, we used to propagate states to entry inside input@type=file for
> example:
> <input type="file" aria-required="true" made child entry to have required
> state too. Since the entry was replaced to not editable/not focusable
> hypertext then I think we don't need to propagate states anymore.
The Browse button is still focusable, though, and I assume states are no longer propagated to that? If not, the problem is that the user may never be informed of the states; e.g. required. This is especially true because the container looks fairly generic.

Comment 17

[alexander :surkov (:asurkov)]

no, no, the button is unchanged

Comment 18

[James Teh [:Jamie]]

If the button has the states, I probably wouldn't worry about the file name label.

Comment 19

[alexander :surkov (:asurkov)]

ok, thank you

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Patch

I removed the event and the state changes and fixes the tests to reflect those changes.

Comment 21

[Mounir Lamouri (:mounir)]

Marco, could you try a build from https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-bd669566f394/ and let me know if <input type='file'> is correct?

Comment 22

[Marco Zehe (:MarcoZ)]

This build works well! The file name is visible below the browse button in the NVDA virtual buffer.

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 729605 [details] [diff] [review]
Patch

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

::: accessible/tests/mochitest/events/test_statechange.html
@@ +77,5 @@
>      {
>        this.fileControlNode = getNode(aID);
>        this.fileControl = getAccessible(this.fileControlNode);
> +      this.browseButton = this.fileControl.firstChild;
> +      // We do not check for state change events on the label on purpose.

perhaps: No state change events on the label

::: accessible/tests/mochitest/states/test_aria.html
@@ +136,4 @@
>        testStates(fileBrowseButton,
>                   STATE_BUSY | STATE_UNAVAILABLE | STATE_REQUIRED | STATE_HASPOPUP | STATE_INVALID);
> +      var fileTextField = getAccessible("fileinput").lastChild;
> +      testStates(fileTextField, 0);

giving 0 states you test nothing. I don't think it makes sense to test absence of states on the label so you can just remove it.

::: accessible/tests/mochitest/states/test_inputs.html
@@ +71,3 @@
>      testStates(fileBrowseButton, STATE_UNAVAILABLE | STATE_REQUIRED);
> +    var fileTextField = getAccessible("file").lastChild;
> +    testStates(fileTextField, 0);

same

Comment 24

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed73c79342be

Comment 25

[Mounir Lamouri (:mounir)]

Thank you guys for your help with this :)

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Mounir Lamouri (:mounir) from comment #25)
> Thank you guys for your help with this :)

thanks to you

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed73c79342be