Closed Bug 541618 Opened 14 years ago Closed 14 years ago

nsINode instead of nsIDOMNode should be used by nsAccessNode

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(1 file, 5 obsolete files)

      No description provided.
Attached patch wip (obsolete) — Splinter Review
it still doesn't work well
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
(In reply to comment #1)
> Created an attachment (id=430260) [details]
> wip
> 
> it still doesn't work well

What's happening? Do some of our tests fail?
The problem is with document accessible which are created too early (and therefore mContent is null). I'm working to reorganize document loading handling to get this fixed. Once I do this I get back to this patch.
I heard nsINode based walking is real fast now (inlining or something)... so this fix should be a good win when it gets sorted out.
Yes, it is.
Making bug 566103 a dependency based on comment 3.
Depends on: 566103
Attached patch wip2 (obsolete) — Splinter Review
Attachment #430260 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
let's go

please be careful, mochitests caught three regressions.
Attachment #449276 - Attachment is obsolete: true
Attachment #449436 - Flags: review?(marco.zehe)
Attachment #449436 - Flags: review?(bolterbugz)
Comment on attachment 449436 [details] [diff] [review]
patch

The try-server build crashes when used with NVDA immediately upon startup. A crash stack, without symbols, is here:
http://crash-stats.mozilla.com/report/index/3bd288c6-c954-4985-81f4-ba9e42100608
Attachment #449436 - Flags: review?(marco.zehe)
Blocks: 570946
Attached patch patch (obsolete) — Splinter Review
try server build is coming
Attachment #449436 - Attachment is obsolete: true
Attachment #450101 - Flags: review?(marco.zehe)
Attachment #450101 - Flags: review?(bolterbugz)
Attachment #449436 - Flags: review?(bolterbugz)
Comment on attachment 450101 [details] [diff] [review]
patch


>-nsAccEvent::GetAccessibleByNode()

While you are here, feel free to change this to GetAccessibleForNode if you want.


I notice many method comments still refer to DOM nodes, although they take a nsINode... I guess that's ok.

> nsIStringBundle *nsAccessNode::gStringBundle = 0;
> nsIStringBundle *nsAccessNode::gKeyStringBundle = 0;
>-nsIDOMNode *nsAccessNode::gLastFocusedNode = 0;
>+nsINode *nsAccessNode::gLastFocusedNode = nsnull;

Might as well change the other globals to nsnull no?

> NS_IMETHODIMP
> nsAccessNode::GetFirstChildNode(nsIAccessNode **aAccessNode)
> {
>   NS_ENSURE_ARG_POINTER(aAccessNode);
>   *aAccessNode = nsnull;
>-  NS_ENSURE_TRUE(mDOMNode, NS_ERROR_NULL_POINTER);
> 
>-  nsCOMPtr<nsIDOMNode> domNode;
>-  mDOMNode->GetFirstChild(getter_AddRefs(domNode));
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
> 
>-  return domNode ? MakeAccessNode(domNode, aAccessNode) : NS_OK;
>+  nsIContent* childNode = GetNode()->GetChildAt(0);
>+  if (childNode)
>+    NS_IF_ADDREF(*aAccessNode = MakeAccessNode(childNode));
>+

Hmmm it isn't clear to me how we got away without addref'ing here before?
Probably GetPreviousSiblingNode and GetNextSiblingNode could be combined into one method with an extra arg.
Can CreateOuterDocAccessible just take a content node now?
Comment on attachment 450101 [details] [diff] [review]
patch

> nsresult
> nsAccessibilityService::CreateHTMLObjectFrameAccessible(nsObjectFrame *aFrame,

>-  nsCOMPtr<nsIDOMDocument> domDoc;
>-  nsCOMPtr<nsIDOMHTMLObjectElement> obj(do_QueryInterface(node));
>-  if (obj)
>+  nsCOMPtr<nsIDOMHTMLObjectElement> obj(do_QueryInterface(content));
>+  if (obj) {
>+    nsCOMPtr<nsIDOMDocument> domDoc;
>     obj->GetContentDocument(getter_AddRefs(domDoc));
>-  else
>-    domDoc = do_QueryInterface(node);
>-  if (domDoc)
>-    return CreateOuterDocAccessible(node, aAccessible);
>+    if (domDoc) {
>+      nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(content));
>+      return CreateOuterDocAccessible(DOMNode, aAccessible);
>+    }
>+  }

This isn't quite the same behaviour. Are we confident that the case where QI nsIDOMHTMLObjectElement fails but QI nsIDOMDocument succeeds is bogus?
(In reply to comment #13)
> (From update of attachment 450101 [details] [diff] [review])
> 
> >-nsAccEvent::GetAccessibleByNode()
> 
> While you are here, feel free to change this to GetAccessibleForNode if you
> want.

Ok.

> 
> I notice many method comments still refer to DOM nodes, although they take a
> nsINode... I guess that's ok.

they are still dom nodes, so this is fine

> >-nsIDOMNode *nsAccessNode::gLastFocusedNode = 0;
> >+nsINode *nsAccessNode::gLastFocusedNode = nsnull;
> 
> Might as well change the other globals to nsnull no?

Ah, I thought but decided to not touch that's not directly related
 
> >-  return domNode ? MakeAccessNode(domNode, aAccessNode) : NS_OK;
> >+  nsIContent* childNode = GetNode()->GetChildAt(0);
> >+  if (childNode)
> >+    NS_IF_ADDREF(*aAccessNode = MakeAccessNode(childNode))
> 
> Hmmm it isn't clear to me how we got away without addref'ing here before?

It was addrefed by MakeAccessNode

(In reply to comment #14)
> Probably GetPreviousSiblingNode and GetNextSiblingNode could be combined into
> one method with an extra arg.

I'll remove them at all :) Later.

(In reply to comment #15)
> Can CreateOuterDocAccessible just take a content node now?

I didn't touched nsIAccessibilityService. We should deal with it separately since it's used outside and therefore we need sr+ for this.

(In reply to comment #16)
> (From update of attachment 450101 [details] [diff] [review])
> > nsresult
> > nsAccessibilityService::CreateHTMLObjectFrameAccessible(nsObjectFrame *aFrame,
> 
> This isn't quite the same behaviour. Are we confident that the case where QI
> nsIDOMHTMLObjectElement fails but QI nsIDOMDocument succeeds is bogus?

HTML object isn't DOM document (http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLObjectElement.cpp#54). There is nothing helpful in the bug where this check was introduced. So I removed it.
Comment on attachment 450101 [details] [diff] [review]
patch

>+//class nsObjectFrame;
>+//class nsIDocShell;
>+//class nsIPresShell;
>+//class nsIContent;
>+//struct nsRoleMapEntry;

Do you want to keep these?

>+  /**
>+   * The same like getAccessibleFor method except it returns accessible only if
>+   * it is attached, i.e. accessible is certified to be a descendent of the root
>+   * accessible.

"The same as get..."

>+  nsAccessible *GetAttachedAccessibleFor(nsINode *aNode);
>+
>+  /**
>+   * Return an DOM node that is relevant to attached accesible check. This

'accessible check'

>+nsAccessible::GetFocusedChild(nsIAccessible **aFocusedChild) 
> { 

>+    focusedChild = GetAccService()->GetAccessible(gLastFocusedNode);
>     if (focusedChild) {
>-      nsCOMPtr<nsIAccessible> focusedParentAccessible;
>-      focusedChild->GetParent(getter_AddRefs(focusedParentAccessible));
>-      if (focusedParentAccessible != this) {
>+      if (focusedChild->GetParent() != this)

Can you make it:
if (focusedChildr && (focusedChild->GetParent() != this)
Comment on attachment 450101 [details] [diff] [review]
patch

> nsApplicationAccessible::GetStateInternal(PRUint32 *aState,
>                                           PRUint32 *aExtraState)
> {
>   *aState = 0;
>+
>+  if (IsDefunct()) {
>+    if (aExtraState)
>+      *aExtraState = nsIAccessibleStates::EXT_STATE_DEFUNCT;
>+
>+    return NS_OK_DEFUNCT_OBJECT;
>+  }
>+

Heh, sure, why not.
I am experiencing one particular crash. I sent Surkov mail, since it requires a specific login to reproduce. The crash, for the record, is here:
http://crash-stats.mozilla.com/report/index/bp-8daa9026-61c3-4b3b-afbc-93c7b2100609
Comment on attachment 450101 [details] [diff] [review]
patch

>       nsEventShell::FireEvent(aEvent);
> 
>       // Post event processing
>-      if (eventType == nsIAccessibleEvent::EVENT_HIDE) {
>+      if (eventType == nsIAccessibleEvent::EVENT_HIDE && node) {

Why did you add the node check?

>         // Shutdown nsIAccessNode's or nsIAccessibles for any DOM nodes in
>         // this subtree.
>-        nsCOMPtr<nsIDOMNode> hidingNode;
>-        aEvent->GetDOMNode(getter_AddRefs(hidingNode));
>-        if (hidingNode) {
>-          RefreshNodes(hidingNode); // Will this bite us with asynch events
>-        }
>+        // XXX: Will this bite us with asynch events.
>+        RefreshNodes(node);

Is that comment a question? If so use a '?' or leave off '.'
Comment on attachment 450101 [details] [diff] [review]
patch

>+  /**
>+   * Traverse through DOM tree and shutdowns accessible objects.
>+   */
>+  void RefreshNodes(nsINode *aStartNode);

nit: "and shutdown"

>+nsTextEquivUtils::GetNameFromSubtree(nsAccessible *aAccessible,
>                                      nsAString& aName)
> {
>   aName.Truncate();
> 
>   if (gInitiatorAcc)
>     return NS_OK;
> 
>   gInitiatorAcc = aAccessible;
> 
>   PRUint32 role = nsAccUtils::Role(aAccessible);
>   PRUint32 nameRule = gRoleToNameRulesMap[role];
> 
>   if (nameRule == eFromSubtree) {

>+    //XXXwhy?

Why what?

>+    if (aAccessible->IsContent()) {
Not sure the nsHTMLAreaAccessible::GetBounds cosmetics are needed here.
OK I finished my first pass. I want to think a bit more.
Oh, but lots to be happy about with this patch! Thank you.
> (In reply to comment #14)
> > Probably GetPreviousSiblingNode and GetNextSiblingNode could be combined into
> > one method with an extra arg.
> 
> I'll remove them at all :) Later.

That's good thanks.
(In reply to comment #21)
> (From update of attachment 450101 [details] [diff] [review])
> >       nsEventShell::FireEvent(aEvent);
> > 
> >       // Post event processing
> >-      if (eventType == nsIAccessibleEvent::EVENT_HIDE) {
> >+      if (eventType == nsIAccessibleEvent::EVENT_HIDE && node) {
> 
> Why did you add the node check?

I moved it here from "if" below.

> >         // Shutdown nsIAccessNode's or nsIAccessibles for any DOM nodes in
> >         // this subtree.
> >-        nsCOMPtr<nsIDOMNode> hidingNode;
> >-        aEvent->GetDOMNode(getter_AddRefs(hidingNode));
> >-        if (hidingNode) {
> >-          RefreshNodes(hidingNode); // Will this bite us with asynch events


> >-        }
> >+        // XXX: Will this bite us with asynch events.
> >+        RefreshNodes(node);
> 
> Is that comment a question? If so use a '?' or leave off '.'

comment, from the past but still valid
(In reply to comment #22)

> >+    //XXXwhy?
> 
> Why what?

Forgot to replace it on something more useful :)
(In reply to comment #23)
> Not sure the nsHTMLAreaAccessible::GetBounds cosmetics are needed here.

yeah, but let's keep them since they were introduced already ;)
Attached patch patch3 (obsolete) — Splinter Review
crash should be fixed, the problem was the frame for HTML body (content of document accessible) is null. I've started to use root frame for a document (so we can calculate styles on document accessible successfully).
Attachment #450101 - Attachment is obsolete: true
Attachment #450294 - Flags: review?(marco.zehe)
Attachment #450294 - Flags: review?(bolterbugz)
Attachment #450101 - Flags: review?(marco.zehe)
Attachment #450101 - Flags: review?(bolterbugz)
This newest try-server build no longer crashes, and NVDA and WE both work fine.

However, JAWS is not able to read any of the contents of the google.de web site.
STR:
1. Go to www.google.de, or maybe www.google.com will also work.
2. JAWS automatically recognizes that the focus goes to the textbox and turns on Forms Mode (high-pitched plopp sound).
3. Press Escape to turn forms mode off (low pitched plopp sound).
4. Try to read anything with the virtual cursor.

Result: Only the title is visible, all of the body contents is missing.

Regular nightly is OK.

Other sites like www.blindcooltech.com work fine with JAWS as well.
Comment on attachment 450294 [details] [diff] [review]
patch3

Cancelling review (see comment #32).
Attachment #450294 - Flags: review?(marco.zehe)
Marco, please provide step by step instructions. I don't know JAWS really good.
1. Go to www.google.de, or maybe www.google.com will also work.
2. JAWS automatically recognizes that the focus goes to the textbox and turns
on Forms Mode (high-pitched plopp sound).
3. Press Escape to turn forms mode off (low pitched plopp sound).
4. Try to read anything with the virtual cursor. Just try to use up and down arrow to read anything other than the title. It won't work. Only 1 line in the virtual cursor, and that's the title of the page.
Additional pages that come up like this with JAWS 11 are *any* Bugzilla bug pages (for example for this bug or bug 571184. I also only see the page title in the virtual buffer for such a bug page.
Probably I need to try to revert every change of ISimpleDOMNode implementation
(In reply to comment #37)
> Probably I need to try to revert every change of ISimpleDOMNode implementation

Unfortunate.
You know, we've called it IID_SimpleDOMDeprecated for a while...
(In reply to comment #39)
> You know, we've called it IID_SimpleDOMDeprecated for a while...

and then it was resurrected and proposed as a part of IA2. Things are permanently changed.
Has it been officially adopted by IA2?
(In reply to comment #41)
> Has it been officially adopted by IA2?

No.
Attached patch patch4Splinter Review
must be fixed
Attachment #450294 - Attachment is obsolete: true
Attachment #450372 - Flags: review?(marco.zehe)
Attachment #450372 - Flags: review?(bolterbugz)
Attachment #450294 - Flags: review?(bolterbugz)
Comment on attachment 450372 [details] [diff] [review]
patch4

r=me if Marco confirms the fix. Thanks!!!!
Attachment #450372 - Flags: review?(bolterbugz) → review+
Comment on attachment 450372 [details] [diff] [review]
patch4

r=me, thanks! This one works with JAWS, too!
Attachment #450372 - Flags: review?(marco.zehe) → review+
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/117fe7a234cc
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment on attachment 450372 [details] [diff] [review]
patch4

>+  PRBool thisLineWasReviewedByDavid = PR_FALSE;

Hey!
(In reply to comment #50)
> (From update of attachment 450372 [details] [diff] [review])
> >+  PRBool thisLineWasReviewedByDavid = PR_FALSE;
> 
> Hey!

(In reply to comment #51)
> Cheeky :)

:)
(In reply to comment #52)
> (In reply to comment #50)
> > (From update of attachment 450372 [details] [diff] [review] [details])
> > >+  PRBool thisLineWasReviewedByDavid = PR_FALSE;
> > 
> > Hey!
> 
> (In reply to comment #51)
> > Cheeky :)

I honestly thought this is for years now, I mean after landing, and I couldn't realize you find it so quickly :)
Comment on attachment 450372 [details] [diff] [review]
patch4

> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(nsDocAccessible)
>   NS_INTERFACE_MAP_STATIC_AMBIGUOUS(nsDocAccessible)
>   NS_INTERFACE_MAP_ENTRY(nsIAccessibleDocument)
>   NS_INTERFACE_MAP_ENTRY(nsIDocumentObserver)
>   NS_INTERFACE_MAP_ENTRY(nsIMutationObserver)
>   NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
>   NS_INTERFACE_MAP_ENTRY(nsIObserver)
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAccessibleDocument)
>-NS_INTERFACE_MAP_END_INHERITING(nsHyperTextAccessible)
>+    foundInterface = 0;
>+
>+  nsresult status;
>+  if (!foundInterface) {
>+    // HTML document accessible must inherit from nsHyperTextAccessible to get
>+    // support text interfaces. XUL document accessible doesn't need this.
>+    // However at some point we may push <body> to implement the interfaces and
>+    // return nsDocAccessible to inherit from nsAccessibleWrap.
>+
>+    nsCOMPtr<nsIDOMXULDocument> xulDoc(do_QueryInterface(mDocument));
Unfortunately cycle collection can call QueryInterface and you're not allowed to change reference counts of cycle collected objects during cycle collection. This was exposed by bug 580096. I'll file a followup bug.
Blocks: 613174
Depends on: 638326
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: