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.

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.

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.

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.

[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?

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.

Always give text and hypertext interface to atkobject of nsHyperTextAccessible. I'll leave editable interface issue to another bug. It's more complicated

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.

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 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.

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?

Will, can you answer Aaron's question? And is Aaron's proposal good for editable text interface?

Aaron, is this what you proposed?

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?

(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

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?

change to marco NS_IMPL_QUERY_

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.

> 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!

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.

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

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.

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.

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.

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 on attachment 274744 [details] [diff] [review] patch v3 Clearing request until questions about IsHyperText() answered.

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 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.

(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.