472326 - html:input of type "file" no longer rendered to screen readers, effective December 30, 2008

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Marco Zehe (:MarcoZ)]

See testcase in next comment. An input with type "file" is no longer rendered to screen readers, nor is the "Browse..." button. Regression occurred between December 29 and December 30, 2008 nightly builds. Checkins in this time range are:
http://hg.mozilla.org/mozilla-central/shortlog/472b245a8b2e

Nothing a11y, so some other change caused this regression.

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: Testcase

1. Open attached testcase.
2. Using AccProbe, look at the hierarchy for this file and compare the outputs of a current Firefox 3.2a1pre nightly and 3.1beta2:

3.2a1pre:
- Document
- - Form
- - - Label
- - - text
- - - Paragraph*
- - - - Text
- - - - Pushbutton
- - Form

3.1beta2:
- Document
- - Form
- - - Label
- - - text
- - - textframe*
- - - - Text
- - - - Pushbutton
- - Form

* The difference is the textframe accessible in 3.1 and the paragraph accessible in 3.2a1pre.

Comment 2

[Marco Zehe (:MarcoZ)]

Looking at above regression window, I see only Bug 471356 that has to do with this kind of frames. Dbaron, Roc, any ideas?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds likely.

Comment 4

[Ginn Chen]

So the bug is screen reader related?

It seems Orca doesn't have a problem with it.

Comment 5

[Marco Zehe (:MarcoZ)]

(In reply to comment #4)
> So the bug is screen reader related?

Yes. JAWS 10 no longer sees the "Browse..." button and the adjacent read-only textbox starting with the December 30 build.

Comment 6

[Aaron Leventhal]

We have a special case for input type="file" in the a11y code here:
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibleTreeWalker.cpp#257
It's the only thing where we walk frames instead of content or anonymous content.

Maybe moving that code would just make it work now? Does input type="file" now use anon content?

Comment 7

[Aaron Leventhal]

No, it doesn't look like that's the change which was made. Perhaps there are just different frames there now. If that's true, someone will have to look at why  eSiblingsWalkFrames doesn't find it anymore.

Comment 8

[Ginn Chen]

Aaron, I didn't catch you.

I think the frames are walked, but it gets a different ROLE for the parent. No?

Comment 9

[Aaron Leventhal]

Ginn, you could be right. I didn't walk with a debugger. I was guessing.

Comment 10

[alexander :surkov (:asurkov)]

we could prefer to fix bug 345195, since the fix will change file input control then this bug may become not actual.

Comment 11

[Aaron Leventhal]

Surkov, how will bug 345195 fix it? We still need to expose the file input the same way in the a11y object hierarchy.

Comment 12

[Marco Zehe (:MarcoZ)]

This patch apparently also landed on Shiretoko, I also get this problem now in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090107 Shiretoko/3.1b3pre. This breaks the market-leading screen reader and makes file upload controls totally inaccessible.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> Surkov, how will bug 345195 fix it? We still need to expose the file input the
> same way in the a11y object hierarchy.

Sure it may be another problem. Just iirc one way is to replace html:input from file control on text. That means we should stir file control implementation - some a11y related stuffs may be changed as well.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> This patch apparently also landed on Shiretoko, I also get this problem now in
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090107
> Shiretoko/3.1b3pre. This breaks the market-leading screen reader and makes file
> upload controls totally inaccessible.

Marco, bug number please

Comment 15

[Marco Zehe (:MarcoZ)]

Surkov: Bug 471356, as also stated in comment #2.

Comment 16

[alexander :surkov (:asurkov)]

Here's the line http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#162. Previously we had areaFrame and therefore we returned ROLE_TEXT_CONTAINER role, but since blockFrame and areaFrame were merged then we use ROLE_PARAGRAPH now. One solution is we shouldn't use tree walker for file input control and create accessible tree inside file input control accessible class.

Comment 17

[alexander :surkov (:asurkov)]

Attachment: testcase2

MarcoZ, is the button is rendered in this example?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #16)

> One solution is we shouldn't use tree walker for file input control and create
> accessible tree inside file input control accessible class.

More precisely - create file input control accessible class inherited from hypertext accessible with ROLE_TEXT_CONTAINER role and make nsFileControlFrame to create this accessible.

Comment 19

[Marco Zehe (:MarcoZ)]

(In reply to comment #17)
> MarcoZ, is the button is rendered in this example?

Yes it is.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> (In reply to comment #17)
> > MarcoZ, is the button is rendered in this example?
> 
> Yes it is.

Marco, what's difference between file input control (testcase you attached) and my testcase, they should have the same tree, no?

Comment 21

[Marco Zehe (:MarcoZ)]

Alex, I assume because JAWS uses a mix of IAccessible2 and iSimpleDom* techniques, that they get confused: They see a type of file somewhere, but don't get the right tree.

Comment 22

[alexander :surkov (:asurkov)]

Attachment: testcase3

Comment 23

[alexander :surkov (:asurkov)]

Marco, what about testcase3?

Comment 24

[Marco Zehe (:MarcoZ)]

I see the normal fiel upload, but not the one made up from the XBL binding.

I now also know what causes the FileUpload to disappear in JAWS's view: It's the label association in my testcase. If I remove the label in my testcase, the fiel upload magically appears, if I put the label back, the file upload vanishes again.

Comment 25

[Aaron Leventhal]

It is probably the change of ROLE_TEXTFRAME to ROLE_PARAGRAPH

The role is decided here, based on whether it's a block frame or not.
http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#161

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> It is probably the change of ROLE_TEXTFRAME to ROLE_PARAGRAPH
> 
> The role is decided here, based on whether it's a block frame or not.
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#161

Right, but it doesn't explain why ROLE_PARAGRAPH doesn't work because testcase2 works correctly.

Comment 27

[Aaron Leventhal]

> Right, but it doesn't explain why ROLE_PARAGRAPH doesn't work because testcase2
> works correctly.
Well, we could try labeling the paragraph in test case 2, and see, because the broken test case has a label and test case 2 doesn't. It could be that JAWS doesn't like to see a label for a paragraph, and just gives up if it sees that. 

Or, maybe they do look at ISimpleDOMNode. However, I think the easiest thing to do now is just check to see if changing the role fixes the problem.

The ROLE_TEXTFRAME is only used in nsHyperTextAccessible (at the line linked to above) as a catch all for any general element. Our first try is to use ROLE_PARAGRAPH if it's display: block, since after all it has a blank line above and below it, so PARAGRAPH is a better, more specific guess.

From the code it appears that a file input has display: block, which doesn't make a lot of sense to me. After all, it can have other text on the same line. Anyway, that is a decision that must have occurred in bug 471356.

We could add another check to the condition when deciding if it's a PARAGRAPH, for example:
    if (frame && frame->GetType() == nsAccessibilityAtoms::blockFrame &&
        frame->GetContent()->Tag() != nsAccessibilityAtoms::input) {
      *aRole = nsIAccessibleRole::ROLE_PARAGRAPH;
    }

That checks to see if it's an input. I'm not sure what the test here should be, but that might fix it with a small change.(In reply to comment #26)

Comment 28

[Marco Zehe (:MarcoZ)]

Attachment: Make sure file upload textboxes are always exposed as textFrame accessibles.

I made the test a bit more complex to make sure we don't do this for *any* input, just for file uploads.

Comment 29

[alexander :surkov (:asurkov)]

it's not good to have this check for every hypertext object. possibly I would prefer to follow comment #18

Comment 30

[Marco Zehe (:MarcoZ)]

Alex, feel free to take this. I am not sure I fully understand comment #18. But this definitely fixes it, so the approach is correct to expose these as ROLE_TEXT_CONTAINER.

Comment 31

[Aaron Leventhal]

Comment on attachment 356959 [details] [diff] [review]
Make sure file upload textboxes are always exposed as textFrame accessibles.

We don't need th check the input type. No other type of input wil ever reach this part of the code.

Comment 32

[Aaron Leventhal]

Marco, you have the right fix. Just check only Tag() == file as I suggested. That's just an int check. It would be overkill to do the extra checks or create another class.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)
> Marco, you have the right fix. Just check only Tag() == file as I suggested.
> That's just an int check. It would be overkill to do the extra checks or create
> another class.

Another class can help to make code easier. Otherwise if I don't know file input control uses nsFileControlFrame than I won't ever know what accessible is created for this element. New class with name "nsHTMLFileControlAccessible" will help to understand code.

Also I should say I'm not happy with approach let's fix this and let's forget it until I understand what's exactly wrong. It sounds JAWS has the rule "labeled accessible of paragraph role can't have accessible children" but even it is true then I don't understand where it is coming from. As well the question "what's wrong with testcase 3" is open still. That's possibly another issue but I can't be 100% sure in this.

Comment 34

[Marco Zehe (:MarcoZ)]

Attachment: Patch2, with Mochitest

Aaron's simple approach, and Mochitest added.

Comment 35

[alexander :surkov (:asurkov)]

So after some testing I realize JAWS rule is "labeled paragraph accessible can't have children", I don't understand completely where this rule comes from. Any ideas? And I still don't understand why testcase3 doesn't work as well (because that html:p doesn't have a label).

Comment 36

[Marco Zehe (:MarcoZ)]

(In reply to comment #35)
> So after some testing I realize JAWS rule is "labeled paragraph accessible
> can't have children", I don't understand completely where this rule comes from.
> Any ideas?

AFAIK, a paragraph is just a container for other elements normally. In other HTML scenarios, we don't put the paragraph contents as accessible name for the paragraph accessible.

> And I still don't understand why testcase3 doesn't work as well
> (because that html:p doesn't have a label).

This may be a fluke with XBL bindings in HTML files, as we also have for the "Subscribe to this page" feature, where JAWS doesn't see the combobox. There is a related bug to that, and I assume once that's fixed, and Freedom fixes their part, the testcase 3 scenario would work as well.

However, we do have a simple fix now. I suggest to take all other discussions out of this bug and create separate ones if necessary.

Comment 37

[Aaron Leventhal]

Please add some kind of comment at checkin. I don't see the need for more than this patch with a comment, personally.

Comment 38

[Marco Zehe (:MarcoZ)]

Attachment: Patch 2.1

Added comment.
Renamed test file to be more generic to nsHyperTextAccessible roles, will add tests for the other roles in later bug.
Removed name test.

Comment 39

[alexander :surkov (:asurkov)]

Comment on attachment 357118 [details] [diff] [review]
Patch 2.1

r=me, just please give another id to paragraph, the word "para" is reserved for something else in my mind :) for example, simply "p" or anything.

Comment 40

[Aaron Leventhal]

Already blocking 1.9.1, no need for 1.9.2

Comment 41

[alexander :surkov (:asurkov)]

(In reply to comment #33)
> (In reply to comment #32)
> > Marco, you have the right fix. Just check only Tag() == file as I suggested.
> > That's just an int check. It would be overkill to do the extra checks or create
> > another class.
> 
> Another class can help to make code easier. Otherwise if I don't know file
> input control uses nsFileControlFrame than I won't ever know what accessible is
> created for this element. New class with name "nsHTMLFileControlAccessible"
> will help to understand code.

I filed bug 473733 for this.

Comment 42

[alexander :surkov (:asurkov)]

For testcase3 problem I filed bug 473737.

Comment 43

[Marco Zehe (:MarcoZ)]

Pushed in changeset:
http://hg.mozilla.org/mozilla-central/rev/a4ad1c1d61dd

Comment 44

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090116 Minefield/3.2a1pre

Comment 45

[Marco Zehe (:MarcoZ)]

Attachment: Patch to go into mozilla-1.9.1

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 357478 [details] [diff] [review]
Patch to go into mozilla-1.9.1

Since this bug is a 1.9.1 blocker, you don't need approval for the patch.

Comment 47

[Marco Zehe (:MarcoZ)]

Pushed to mozilla-1.9.1 in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fd73ce30e1b8

Comment 48

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre