clean up nsAccessNodeWrap::MakeAccessNode()

NEW
Unassigned

Status

()

Core
Disability Access APIs
6 years ago
4 months ago

People

(Reporter: tbsaunde, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
rm a QI to nsIContent, and do a little cleanup while there.
(Reporter)

Comment 1

6 years ago
Created attachment 619365 [details] [diff] [review]
patch
Attachment #619365 - Flags: review?(surkov.alexander)

Comment 2

6 years ago
Comment on attachment 619365 [details] [diff] [review]
patch

Review of attachment 619365 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +426,4 @@
>  
>      newNode->Init();
>      iNode = static_cast<ISimpleDOMNode*>(newNode);
>      iNode->AddRef();

you need to fix indentation since you move this block out of 'else if' construction

aNode->AsElement() should make us crash in case of inaccessbile document or text node since they aren't inherited from Element class.
Attachment #619365 - Flags: review?(surkov.alexander)
(Reporter)

Comment 3

6 years ago
> ::: accessible/src/msaa/nsAccessNodeWrap.cpp
> @@ +426,4 @@
> >  
> >      newNode->Init();
> >      iNode = static_cast<ISimpleDOMNode*>(newNode);
> >      iNode->AddRef();
> 
> you need to fix indentation since you move this block out of 'else if'
> construction
> 
> aNode->AsElement() should make us crash in case of inaccessbile document or
> text node since they aren't inherited from Element class.

yeah, I guess the assertion we had there before wasn't really correct.  However we still shouldn't crash because ISimpleDOMNode methods should handle case of defunct content and I think most do.

Comment 4

6 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> yeah, I guess the assertion we had there before wasn't really correct.

what's wrong with it?
(Reporter)

Comment 5

6 years ago
(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > yeah, I guess the assertion we had there before wasn't really correct.
> 
> what's wrong with it?

I thought in some cases we intentionally don't create accessible documents for dom docs? for example temporary ones or svg?

Comment 6

6 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> I thought in some cases we intentionally don't create accessible documents
> for dom docs? for example temporary ones or svg?

yes, we don't create them in some cases but it doesn't prevent an AT to access to the content by ISimpleDOMNode.
(Reporter)

Comment 7

6 years ago
(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > I thought in some cases we intentionally don't create accessible documents
> > for dom docs? for example temporary ones or svg?
> 
> yes, we don't create them in some cases but it doesn't prevent an AT to
> access to the content by ISimpleDOMNode.

yes, and that would mean that the assert wasn't quiet right wouldn't it since the assert would fire and we'd not give t an ISimpleDOMNode?  I'd think technical in this case of inaccessible document we should return object that is a ISimpleDOMDOcument as well as a ISimpleDOMNode, but that might take work and no at seems to care.
(Reporter)

Comment 8

6 years ago
Created attachment 619877 [details] [diff] [review]
patch v2
Attachment #619877 - Flags: review?(surkov.alexander)

Comment 9

6 years ago
Comment on attachment 619877 [details] [diff] [review]
patch v2

Review of attachment 619877 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +424,3 @@
>  
> +  if (!aNode->IsElement())
> +    return nsnull;

you ignore text nodes

Comment 10

6 years ago
Comment on attachment 619877 [details] [diff] [review]
patch v2

canceling request until comments are addressed
Attachment #619877 - Flags: review?(surkov.alexander)

Comment 11

5 years ago
It will be fixed by bug 767756.
Assignee: tbsaunde+mozbugs → nobody
You need to log in before you can comment on or make changes to this bug.