386235 - the <link> tag for stylesheets can cause text in document frame to be inaccessible.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Attachment: test case

Steps to reproduce:

1. View the attached test case in Firefox
2. Use Accerciser's Interface Viewer to examine the text for the document_frame

Expected results:  The body text would be displayed
Actual results:   There is no text associated with the document_frame

3. Edit the attached test case making any change to the <link> tag for the style sheet (e.g. insert a character within the href="", add a title, delete the entire thing, whatever)
4. Repeat steps 1-2

Expected results: The body text would be displayed
Actual results: The body text is displayed

Other comments:  For an actual case, use Accerciser to examine items for sale on craigslist.  The document frame has no text associated with it.

Comment 1

[Mike Pedersen]

I wonder, should the sevarity or priority on this one be bumped up as it is loss of functionality?  It could almost be called data loss as well because the screen reader user can't at all get to this information.  

Comment 2

[Ginn Chen]

similar problem of the EditableText interface, QI to Text interface fails when we create atkobject for DocAccessible.
But when the page is finally loaded, QI to Text interface is OK.
We need to recreate atkobject when interfaces changed.

Comment 3

[Ginn Chen]

We can ShutdownATKObject when nsDocAccessibleWrap::InvalidateChildren is called.
It seems it's too often, we should only recreate atkobject on interface changes.

Or maybe we can make atkobject of nsDocAccessibleWrap always have AtkHypertext and AtkText interface.

Comment 4

[Ginn Chen]

[19:40]  <aaronlev> a hypertext with only text children does not support accessiblehypertext
[19:41]  <aaronlev> but it could get other children inserted

So not only nsDocAccessible has this problem, right?

Comment 5

[Aaron Leventhal]

Right, any nsHyperTextAccessible can have this problem.

In fact, we should look at all of the "Smart QI" implementation where we implement our own custom QueryInterface() method.

Comment 6

[Ginn Chen]

Attachment: patch

Always give text and hypertext interface to atkobject of nsHyperTextAccessible.

I'll leave editable interface issue to another bug.
It's more complicated

Comment 7

[alexander :surkov (:asurkov)]

I think it's not right to expose all possible interfaces. Our feature is list of supported interfaces may be changed. I think we should watch if interface is changed then recreate atk object. Otherwise will AT be confused if we say object supports interfaces that it doens't support actually? Also the problem is wider than nsHyperTextAccessible issue because, for example, we won't expose nsIAccessibleValue methods if corresponded ARIA attributes setted dynamically.

Comment 8

[Ginn Chen]

Well, nsIAccessibleValue is another interface we should take care.

From the discussion with Aaron and Willie, I think it's good to not recreate atkobject for text and hypertext interfaces.

See Bug 371394 #c47 #c48 #c52

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 273943 [details] [diff] [review]
patch

(In reply to comment #8)
> From the discussion with Aaron and Willie, I think it's good to not recreate
> atkobject for text and hypertext interfaces.
> 
> See Bug 371394 #c47 #c48 #c52
> 

Ok. Though as I said ARIA brings dynamic into accessible objects and if we don't want to expose almost all sham interfaces on accessible objects then we should recreate objects (possibly atk only). In that case I can't see a good reason why we should do an exclusion for hypertext accessible.

I'm not sure in approach. but patch code looks correct, so r=me.

Comment 10

[Aaron Leventhal]

If we're going to do this, I'd rather just change nsHypertextAccessible::QueryInterface() so that it always exposes text/hypertext/editabletext interfaces.

* GetText can be used to see if the item actually has text
* The EDITABLE state can be used to determine if it is editable or not
* getLinks() can be used to see if there are embedded objects

Or, do what Surkov suggests and recreate objects when interface changes. We'd want to do that for IA2 as well, because of rules of COM (the interfaces supported during the lifetime of an object must not change). They might have that rule for marshalling optimizations, but I'm not sure. There's probably an MSDN article about it.

What was Willie's reason for not wanting to recreate the object? Is that a general issue or specific to text and hypertext interface support?

Comment 11

[Ginn Chen]

Will, can you answer Aaron's question?

And is Aaron's proposal good for editable text interface?

Comment 12

[Ginn Chen]

Attachment: patch v2

Aaron, is this what you proposed?

Comment 13

[Aaron Leventhal]

Ginn, if we go this way, we should remove the nsHypertextAccessible::QueryInterface() and replace it with macros NS_IMPL_ISUPPORTS etc.

What do the changes to accessible/src/atk do?

Comment 14

[Ginn Chen]

(In reply to comment #13)
> What do the changes to accessible/src/atk do?
> 

don't want to check link num in nsAccessibleWrap::CreateMaiInterfaces
no need to recreate atkobject in nsDocAccessibleWrap::SetEditor
remove NS_ASSERTION, because it is supposed to fail
change bad variable naming in nsMaiInterfaceHypertext.cpp
add a missing null check in nsMaiInterfaceText.cpp

Comment 15

[Aaron Leventhal]

So, this approach could be ok.. But I wonder why Will doesn't want to destroy and recreate the object when the interfaces change?

Does it save perf if something doesn't QI to HyperText? that way you don't need to check an extra cross process call for the number of links.

Similar for text. The QI to the Text interface fails, so wouldn't Orca save an extra cross-process call to getText to find out there isn't any?

Comment 16

[Ginn Chen]

Attachment: patch v2.5

change to marco NS_IMPL_QUERY_

Comment 17

[Willie Walker]

Orca cares about the world in more than just the snapshot sense -- we like to know where the user came from and where they went to over time.  This means keeping some consistency in the objects.  Trashing and recreating the accessible peer for the same object over and over can make this difficult.

Comment 18

[Willie Walker]

> If we're going to do this, I'd rather just change
> nsHypertextAccessible::QueryInterface() so that it always exposes
> text/hypertext/editabletext interfaces.

This seems reasonable to me.  Thanks!

Comment 19

[Aaron Leventhal]

Ginn, can you make sure of the following?

1. Check any place we QI to nsIAccessibleText/HyperText/EditableText and make sure we're not using that as a test to see what the thing is?
2. Check all nsIAccessibleText method calls and make sure they do something sane when there are no children to support it?
3. Check all nsIAccessibleHyperText calls and make sure they do the right thing, e.g. return 0 links etc.
4. Check all nsIAccessibleEditableText calls and make sure they don't allow editing when the object is not truly editable. For example, don't allow paste/cut/delete operations.

Comment 20

[Aaron Leventhal]

5. Make sure we fire state change for EDITABLE when contenteditable changes.

Comment 21

[Aaron Leventhal]

6. Make sure nsIAccessibleHyperText is *not* supported for plain text widgets like nsHTMLTextfieldAccessible (also watch for XUL and XForms inputs). You might have to explicitly impl QueryInterface for it and return NS_ERROR_NO_INTERFACE for nsIAccessibleHyperText.

Comment 23

[Ginn Chen]

Attachment: patch v3

Changes:
Get xulDoc check back in nsHyperTextAccessible QI.
Also check ROLE for nsIAccessibleHyperText.

atk/AccessibleWrap.cpp
In getChildCountCB and refChildCB
QI to RefPtr<nsHyperTextAccessible> and call IsHyperText() to check if we're real hypertext

state change event for EDITABLE,
we did fire this event in nsDocAccessible::CheckForEditor()
of course it's not enough, but I'd like to leave this issue for separate bug.
There's a lot to do to support contenteditable.

Comment 24

[Aaron Leventhal]

For making sure that text fields don't QI to hypertext, another way would be to override QueryInterface on nsHTMLTextfieldAccessible, nsXULTextfieldAccessible and whatever the XForms one is. But, I'm okay with this approach.

One thing is, though, because of bug 372131 it looks like some text leaf nodes will implement these interfaces and they shouldn't. We can fix that the right way in bug 372131, but for Firefox 3 you might just want to check to see if it's ROLE_TEXT_LEAF in the QI, and not support any of these interfaces in that case. I'm not sure, because I haven't looked closely.

Comment 25

[Aaron Leventhal]

Why does getChildCountCB() need to change? If it's an nsIAccessibleHyperText with 0 links (embedded objects) it should report 0 children anyway.

Similarly, I'm not sure refChildCB() needs to change either. If it's an nsIAccessibleHyperText with 0 links then that does look like it will return nsnull.

My comments about what to check 1-6 were more things to look at -- I wasn't sure.

Can't we actually remove the method IsHyperText() now?

Comment 26

[Aaron Leventhal]

Comment on attachment 274744 [details] [diff] [review]
patch v3

Clearing request until questions about IsHyperText() answered.

Comment 27

[Ginn Chen]

Attachment: patch v4

I found "the problem" of getChildCountCB() and refChildCB() while doing check 1).
I didn't realize actually they also work well with the new QI.
I changed it back in this version.

I added the check of ROLE_TEXT_LEAF to QI.
For Fx3, I don't think we should override QI for nsHTMLTextfieldAccessible, nsXULTextfieldAccessible and Xforms text field.
Maybe we will solve it in another way in Fx4.

Yes, IsHyperText() can be removed. It is removed in patch v4.

Comment 28

[Aaron Leventhal]

Comment on attachment 275086 [details] [diff] [review]
patch v4

Is there no way to let the ATK caller know that the editable text calls like cutText have failed (when the object is not editable)?

It looks like there is an indentation error in getRunAttributesCB

I also think that QI to nsIAccessibleEditableText or nsIAccessibleText should not happen for ROLE_TEXT_LEAF.

Comment 29

[Ginn Chen]

Attachment: patch updated to HEAD and addressed comments

Comment 30

[Ginn Chen]

(In reply to comment #28)
> (From update of attachment 275086 [details] [diff] [review])
> Is there no way to let the ATK caller know that the editable text calls like
> cutText have failed (when the object is not editable)?

Correct, no way.