Closed Bug 472326 Opened 11 years ago Closed 11 years ago

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

Categories

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

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: MarcoZ, Assigned: MarcoZ)

Details

(Keywords: access, regression, verified1.9.1)

Attachments

(4 files, 3 obsolete files)

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.
Flags: blocking1.9.2?
Attached file 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.
Looking at above regression window, I see only Bug 471356 that has to do with this kind of frames. Dbaron, Roc, any ideas?
So the bug is screen reader related?

It seems Orca doesn't have a problem with it.
(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.
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?
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.
Aaron, I didn't catch you.

I think the frames are walked, but it gets a different ROLE for the parent. No?
Ginn, you could be right. I didn't walk with a debugger. I was guessing.
we could prefer to fix bug 345195, since the fix will change file input control then this bug may become not actual.
Surkov, how will bug 345195 fix it? We still need to expose the file input the same way in the a11y object hierarchy.
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.
Flags: blocking1.9.1?
(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.
(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
Surkov: Bug 471356, as also stated in comment #2.
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.
Attached file testcase2 (obsolete) —
MarcoZ, is the button is rendered in this example?
(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.
(In reply to comment #17)
> MarcoZ, is the button is rendered in this example?

Yes it is.
(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?
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.
Attached file testcase3
Attachment #356521 - Attachment is obsolete: true
Marco, what about testcase3?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
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.
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
(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.
> 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)
I made the test a bit more complex to make sure we don't do this for *any* input, just for file uploads.
Assignee: nobody → marco.zehe
Status: NEW → ASSIGNED
Attachment #356959 - Flags: review?(aaronleventhal)
it's not good to have this check for every hypertext object. possibly I would prefer to follow comment #18
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 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.
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.
(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.
Attached patch Patch2, with Mochitest (obsolete) — Splinter Review
Aaron's simple approach, and Mochitest added.
Attachment #356959 - Attachment is obsolete: true
Attachment #357096 - Flags: review?(surkov.alexander)
Attachment #356959 - Flags: review?(aaronleventhal)
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).
(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.
Attachment #357096 - Flags: review+
Please add some kind of comment at checkin. I don't see the need for more than this patch with a comment, personally.
Attached patch Patch 2.1Splinter Review
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.
Attachment #357118 - Flags: review?(surkov.alexander)
Attachment #357096 - Flags: review?(surkov.alexander)
Attachment #357096 - Attachment is obsolete: true
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.
Attachment #357118 - Flags: review?(surkov.alexander) → review+
Already blocking 1.9.1, no need for 1.9.2
Flags: blocking1.9.2?
(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.
For testcase3 problem I filed bug 473737.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090116 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Attachment #357478 - Flags: approval1.9.1?
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.
Attachment #357478 - Flags: approval1.9.1?
Pushed to mozilla-1.9.1 in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fd73ce30e1b8
Keywords: fixed1.9.1
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre
You need to log in before you can comment on or make changes to this bug.