542646 - Don't walk the anonymous content free for text controls on HTML web pages

Status: RESOLVED WONTFIX

(Core :: Disability Access APIs, defect)

Description

[(no longer active)]

Attachment: Patch (v1)

We discussed this with David today, and we believe that it's not needed for the a11y code to walk the anonymous content tree of text controls (<input type=text|password> and <textarea>).

The attached patch does this.  I've run the entire a11y test suite with this patch, and it seems that it's not breaking anything at least! :-)

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Actually, the inclusion of nsPresContext.h is not needed.

Comment 2

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

Comment on attachment 423865 [details] [diff] [review]
Patch (v1)

Hey thanks for taking a look at this. Did you confirm that we do walk the anonymous text controls?

In any event I'm not sure we need this patch.

Comment 3

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

(I mean anonymous text control frames)

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #0)
> Created an attachment (id=423864) [details]
> Patch (v1)
> 
> We discussed this with David today, and we believe that it's not needed for the
> a11y code to walk the anonymous content tree of text controls (<input
> type=text|password> and <textarea>).

> The attached patch does this.  I've run the entire a11y test suite with this
> patch, and it seems that it's not breaking anything at least! :-)

Then we don't have proper a11y tests. I believe nsHyperTextAccessible logic depends there are accessibles inside of text controls.

I wonder what is the initial reason of this bug? 

I think the bug is wontfix because we don't expose these accessible to AT but they are used internally. We could fix that of course but I'm not sure it's worth since it should require huge amount of time and I don't see any worth motivation.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (In reply to comment #0)
> > Created an attachment (id=423864) [details] [details]
> > Patch (v1)
> > 
> > We discussed this with David today, and we believe that it's not needed for the
> > a11y code to walk the anonymous content tree of text controls (<input
> > type=text|password> and <textarea>).
> 
> > The attached patch does this.  I've run the entire a11y test suite with this
> > patch, and it seems that it's not breaking anything at least! :-)
> 
> Then we don't have proper a11y tests.

Looking more deeply your changes don't have any affect because this code isn't used for text fields (see http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#222).

I wonder if getting an accessibles from is editor's document is equivalent to getting an accessbiles from native anonymous content in this case.

Comment 6

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

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #0)
> > > Created an attachment (id=423864) [details] [details] [details]
> > > Patch (v1)
> > > 
> > > We discussed this with David today, and we believe that it's not needed for the
> > > a11y code to walk the anonymous content tree of text controls (<input
> > > type=text|password> and <textarea>).
> > 
> > > The attached patch does this.  I've run the entire a11y test suite with this
> > > patch, and it seems that it's not breaking anything at least! :-)
> > 
> > Then we don't have proper a11y tests.
> 
> Looking more deeply your changes don't have any affect because this code isn't
> used for text fields (see
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#222).

You are right.

Ehsan is getting to know layout pretty well, and has concerns about us walking the frame children of text fields. I'm not sure there is a problem though, since we won't hold on to these frames long.

> I wonder if getting an accessibles from is editor's document is equivalent
> to getting an accessbiles from native anonymous content in this case.

Not sure of your question, but if we can use API to avoid walking the frames ourselves that is preferred.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> > I wonder if getting an accessibles from is editor's document is equivalent
> > to getting an accessbiles from native anonymous content in this case.
> 
> Not sure of your question, but if we can use API to avoid walking the frames
> ourselves that is preferred.

The question is can we walk native anonymous content instead of waking editor document? Walking frames by ourselves will be fixed in bug 534527.

Comment 8

[(no longer active)]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > > I wonder if getting an accessibles from is editor's document is equivalent
> > > to getting an accessbiles from native anonymous content in this case.
> > 
> > Not sure of your question, but if we can use API to avoid walking the frames
> > ourselves that is preferred.
> 
> The question is can we walk native anonymous content instead of waking editor
> document? Walking frames by ourselves will be fixed in bug 534527.

So, there are three types of editors, <input type=text|password>, <textarea> and contenteditable elements.  For the first two, the editor's root node is an anonymous div created under the content node.  The latter is however a bit more complex.  For contenteditable elements, the editor uses a body element which is _not_ in the anonymous content tree, so I guess the above solution wouldn't work in every case.

How about special casing the input and textarea elements there?  Would that be a good approach?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)

> How about special casing the input and textarea elements there?  Would that be
> a good approach?

I think yes. We could remove that hack from nsHyperTextAccessible for these elements and be happy with common approach that works currently for input@type="file" for example.

For contenteditable elements we create accessibles for explicit children I think.

Comment 10

[(no longer active)]

Filed bug 542824 for special casing that code.