Closed
Bug 541618
Opened 15 years ago
Closed 14 years ago
nsINode instead of nsIDOMNode should be used by nsAccessNode
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 2 open bugs)
Details
(Keywords: access)
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
davidb
:
review+
MarcoZ
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•15 years ago
|
||
it still doesn't work well
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Comment 2•14 years ago
|
||
(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?
Assignee | ||
Comment 3•14 years ago
|
||
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.
Comment 4•14 years ago
|
||
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.
Assignee | ||
Comment 5•14 years ago
|
||
Yes, it is.
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #430260 -
Attachment is obsolete: true
Assignee | ||
Comment 8•14 years ago
|
||
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)
Assignee | ||
Comment 9•14 years ago
|
||
Comment 10•14 years ago
|
||
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)
Assignee | ||
Comment 11•14 years ago
|
||
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)
Assignee | ||
Comment 12•14 years ago
|
||
Comment 13•14 years ago
|
||
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?
Comment 14•14 years ago
|
||
Probably GetPreviousSiblingNode and GetNextSiblingNode could be combined into one method with an extra arg.
Comment 15•14 years ago
|
||
Can CreateOuterDocAccessible just take a content node now?
Comment 16•14 years ago
|
||
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?
Assignee | ||
Comment 17•14 years ago
|
||
(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 18•14 years ago
|
||
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 19•14 years ago
|
||
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.
Comment 20•14 years ago
|
||
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 21•14 years ago
|
||
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 22•14 years ago
|
||
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()) {
Comment 23•14 years ago
|
||
Not sure the nsHTMLAreaAccessible::GetBounds cosmetics are needed here.
Comment 24•14 years ago
|
||
OK I finished my first pass. I want to think a bit more.
Comment 25•14 years ago
|
||
Oh, but lots to be happy about with this patch! Thank you.
Comment 26•14 years ago
|
||
> (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.
Assignee | ||
Comment 27•14 years ago
|
||
(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
Assignee | ||
Comment 28•14 years ago
|
||
(In reply to comment #22)
> >+ //XXXwhy?
>
> Why what?
Forgot to replace it on something more useful :)
Assignee | ||
Comment 29•14 years ago
|
||
(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 ;)
Assignee | ||
Comment 30•14 years ago
|
||
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)
Assignee | ||
Comment 31•14 years ago
|
||
Comment 32•14 years ago
|
||
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 33•14 years ago
|
||
Attachment #450294 -
Flags: review?(marco.zehe)
Assignee | ||
Comment 34•14 years ago
|
||
Marco, please provide step by step instructions. I don't know JAWS really good.
Comment 35•14 years ago
|
||
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.
Comment 36•14 years ago
|
||
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.
Assignee | ||
Comment 37•14 years ago
|
||
Probably I need to try to revert every change of ISimpleDOMNode implementation
Comment 38•14 years ago
|
||
(In reply to comment #37)
> Probably I need to try to revert every change of ISimpleDOMNode implementation
Unfortunate.
Comment 39•14 years ago
|
||
You know, we've called it IID_SimpleDOMDeprecated for a while...
Assignee | ||
Comment 40•14 years ago
|
||
(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.
Comment 41•14 years ago
|
||
Has it been officially adopted by IA2?
Assignee | ||
Comment 42•14 years ago
|
||
(In reply to comment #41)
> Has it been officially adopted by IA2?
No.
Assignee | ||
Comment 43•14 years ago
|
||
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 44•14 years ago
|
||
Comment on attachment 450372 [details] [diff] [review]
patch4
r=me if Marco confirms the fix. Thanks!!!!
Attachment #450372 -
Flags: review?(bolterbugz) → review+
Assignee | ||
Comment 45•14 years ago
|
||
Comment 46•14 years ago
|
||
Comment on attachment 450372 [details] [diff] [review]
patch4
r=me, thanks! This one works with JAWS, too!
Attachment #450372 -
Flags: review?(marco.zehe) → review+
Assignee | ||
Comment 47•14 years ago
|
||
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/117fe7a234cc
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 48•14 years ago
|
||
solaris bustage - http://hg.mozilla.org/mozilla-central/rev/431eab8cf6ab
Comment 49•14 years ago
|
||
Mac bustage fix: http://hg.mozilla.org/mozilla-central/rev/36dec4f55fa8.
Comment 50•14 years ago
|
||
Comment on attachment 450372 [details] [diff] [review]
patch4
>+ PRBool thisLineWasReviewedByDavid = PR_FALSE;
Hey!
Comment 51•14 years ago
|
||
Cheeky :)
Assignee | ||
Comment 52•14 years ago
|
||
(In reply to comment #50)
> (From update of attachment 450372 [details] [diff] [review])
> >+ PRBool thisLineWasReviewedByDavid = PR_FALSE;
>
> Hey!
(In reply to comment #51)
> Cheeky :)
:)
Assignee | ||
Comment 53•14 years ago
|
||
(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 54•14 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•