767755 - Re-implement IsPrimaryForNode() using an Accessible::mFlags bit

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mark Capella [:capella]]

Create a new AccessibleType enum ...

Comment 1

[Diego Ortiz]

I want to star working to this bug. What must i do?

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to Diego Ortiz from comment #1)
> I want to star working to this bug. What must i do?

1. add eIsPrimaryForNode flag to StateFlags enum accessible/src/generic/Accessible.h line 744 (Alex feel free to biekeshed about the name if you like)
2. add method to Accessible IsPrimaryForNode() similar to IsDefunct() see Accessible.h line 690
3. replace the in the constructor for Accessible you should set the eIsPrimaryForNode bit in mFlags to true.
4. remove nsAccessNode::IsPrimaryForNode see Accessible/src/base/nsAccessNode.cpp
5. remove overloads of the nsAccessNode::IsPrimaryForNode() virtual method, in each class with such a overload change the constructor to handle setting the eIsPrimaryForNode bit in mFlags.

Comment 3

[Diego Ortiz]

I dont know where replace the constructor the point 3

Comment 4

[Xi Yang]

(In reply to Diego Ortiz from comment #3)
> I dont know where replace the constructor the point 3

http://mxr.mozilla.org/mozilla-central/ident?i=Accessible use this to find it.

Comment 5

[SADINENI RAVI CHANDRA]

hii i want to work on this project. can u please tell me what should i do..

Comment 6

[rajesh kumar]

(In reply to SADINENI RAVI CHANDRA from comment #5)
> hii i want to work on this project. can u please tell me what should i do..

you should leave this for me to solve...

Comment 7

[Josh Matthews [:jdm]]

Sadineni, if you still wish to work on this bug, just leave a comment saying so. You should follow the steps from comment 2.

Comment 8

[SADINENI RAVI CHANDRA]

ya  i am intersgted in continuing with this project but can u please illustrate what exactly i am supposed to do...

Comment 9

[alexander :surkov (:asurkov)]

(In reply to rajesh kumar from comment #6)
> (In reply to SADINENI RAVI CHANDRA from comment #5)
> > hii i want to work on this project. can u please tell me what should i do..
> 
> you should leave this for me to solve...

Did you start working on it already?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to SADINENI RAVI CHANDRA from comment #8)
> ya  i am intersgted in continuing with this project but can u please
> illustrate what exactly i am supposed to do...

comment #2 explains steps to fix. In case if any item isn't clear enough then please don't hesitate to ask. But before working on this it makes sense to figure out the assignee. If Rajesh has something already then we can leave this bug to him and find something else for you.

Comment 11

[SADINENI RAVI CHANDRA]

(In reply to alexander :surkov from comment #10)
> (In reply to SADINENI RAVI CHANDRA from comment #8)
> > ya  i am intersgted in continuing with this project but can u please
> > illustrate what exactly i am supposed to do...
> 
> comment #2 explains steps to fix. In case if any item isn't clear enough
> then please don't hesitate to ask. But before working on this it makes sense
> to figure out the assignee. If Rajesh has something already then we can
> leave this bug to him and find something else for you.
rajesh is working on an other bug i presume . so shall i go a head with this bug ?? and can you please assign it to me..

Comment 12

[SADINENI RAVI CHANDRA]

where can i get the documentation ... as i have no idea what i am doing  ??

Comment 13

[alexander :surkov (:asurkov)]

(In reply to SADINENI RAVI CHANDRA from comment #11)

> rajesh is working on an other bug i presume . so shall i go a head with this
> bug ?? and can you please assign it to me..

I don't see objection why you couldn't go with this bug. But let's wait what Rajesh says.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to SADINENI RAVI CHANDRA from comment #12)
> where can i get the documentation ... as i have no idea what i am doing  ??

I assume you meant how to get sources and build fireofx, here is it  https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions?redirectlocale=en-US&redirectslug=Build_Documentation

Comment 15

[SADINENI RAVI CHANDRA]

(In reply to alexander :surkov from comment #14)
> (In reply to SADINENI RAVI CHANDRA from comment #12)
> > where can i get the documentation ... as i have no idea what i am doing  ??
> 
> I assume you meant how to get sources and build fireofx, here is it 
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Build_Instructions?redirectlocale=en-US&redirectslug=Build_Documentation
i have cloned the git code base. and the doubt that i have is whay are we reimplementing isprimarynode(). what exactly does it do ??

Comment 16

[SADINENI RAVI CHANDRA]

i have cloned mozzilla-central is that fine ??

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to SADINENI RAVI CHANDRA from comment #16)
> i have cloned mozzilla-central is that fine ??

I recommend cloning and working against inbound:
hg clone http://hg.mozilla.org/integration/mozilla-inbound/ inbound-src

Comment 18

[SADINENI RAVI CHANDRA]

(In reply to David Bolter [:davidb] from comment #17)
> (In reply to SADINENI RAVI CHANDRA from comment #16)
> > i have cloned mozzilla-central is that fine ??
> 
> I recommend cloning and working against inbound:
> hg clone http://hg.mozilla.org/integration/mozilla-inbound/ inbound-src

i was not able to download it... it gets hanged after displaying the message  "adding-changesets"

Comment 19

[SADINENI RAVI CHANDRA]

(In reply to alexander :surkov from comment #13)
can u please assign the bug to me .. i started working on it .....

Comment 20

[alexander :surkov (:asurkov)]

done

Comment 21

[SADINENI RAVI CHANDRA]

(In reply to alexander :surkov from comment #20)
> can u please explain the third step once again....

Comment 22

[SADINENI RAVI CHANDRA]

hii surkov... in  step 4 should i include Accessible.h in AccessNode.h ??

Comment 23

[alexander :surkov (:asurkov)]

(In reply to SADINENI RAVI CHANDRA from comment #22)
> hii surkov... in  step 4 should i include Accessible.h in AccessNode.h ??

it seems no, you should remove IsPrimaryForNode() method from nsAccessNode class.

Comment 24

[SADINENI RAVI CHANDRA]

but it should return true right ?? so shouldn't  i set the flag to true...

Comment 25

[SADINENI RAVI CHANDRA]

in nsAccessNode.cpp

Comment 26

[SADINENI RAVI CHANDRA]

the actual question is . since mFlags is not in the scope of nsAccessNode.. shld i declare a variable "mFlags" and "eIsPrimaryForNode"... or shld i include Accesible.h where mFlags is defined ??

Comment 27

[alexander :surkov (:asurkov)]

nsAccessNode shouldn't have IsPrimaryForNode(), mFlags or eIsPrimaryForNode, just remove that method.

Comment 28

[SADINENI RAVI CHANDRA]

hii surkov... i think i have fixed the bug.. i ran make  again .. and no errors ..and i was able to run firefox...  now how to test the code ??

Comment 29

[alexander :surkov (:asurkov)]

(In reply to SADINENI RAVI CHANDRA from comment #28)
> hii surkov... i think i have fixed the bug.. i ran make  again .. and no
> errors ..and i was able to run firefox...  now how to test the code ??

cd yourobjdirc/_tests/testing/mochitest
python runtests.py --a11y --autorun

if tests are green then you should be in a good shape.

Please attach the patch and ask mentor for review.

Comment 30

[SADINENI RAVI CHANDRA]

hi surkov.. i got some errors while testing
i downloaded the code again into a seperate folder
i have build the newly downloaded code
i have tried to test it using the same command
but it failed in 1 case though i havent made any changes to the code
can u pls guide me what shld i do now...

Comment 31

[SADINENI RAVI CHANDRA]

it shows an AttributeError: NonType object has no attribute 'find'

Comment 32

[alexander :surkov (:asurkov)]

no idea, sorry, it makes sense to ask on #developers

Comment 33

[Eric Wong (:ekw)]

Attachment: patch

I'd like to work on this bug.  Please assign to me.
Please let me know if I'm on the right track with this patch.

Comment 34

[Trevor Saunders (:tbsaunde)]

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

> Accessible::Accessible(nsIContent* aContent, DocAccessible* aDoc) :
>   nsAccessNodeWrap(aContent, aDoc),
>-  mParent(nullptr), mIndexInParent(-1), mFlags(eChildrenUninitialized),
>+  mParent(nullptr), mIndexInParent(-1), mFlags(eChildrenUninitialized | eIsPrimaryForNode),

nit, over 80 chars

>+   * Return true if the accessible is primary for node.

maybe, "return true if this accessible is the primary one for its node."

> HTMLImageMapAccessible::
>   HTMLImageMapAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>   ImageAccessibleWrap(aContent, aDoc)
> {
>   mFlags |= eImageMapAccessible;
>+  mFlags &= ~eIsPrimaryForNode;
> }
>-bool
>-HTMLAreaAccessible::IsPrimaryForNode() const
>-{
>-  // Make HTML area DOM element not accessible. HTML image map accessible
>-  // manages its tree itself.
>-  return false;
>-}

that's not correct, it used to be HTMLAreaAccessibles where not primary for node, but the HTMLImageMapAccessible was, but you swap that.  you should add to the constructor for HTMLAreaAccessible not HTMLImageMapAccessible.

> HTMLLIAccessible::
>   HTMLLIAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>   HyperTextAccessibleWrap(aContent, aDoc), mBullet(nullptr)
> {
>   mFlags |= eHTMLListItemAccessible;
>+  mFlags &= ~eIsPrimaryForNode;
> 
>-bool
>-HTMLListBulletAccessible::IsPrimaryForNode() const
>-{
>-  return false;
>-}

same problem here.

> HTMLSelectListAccessible::
>   HTMLSelectListAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>   AccessibleWrap(aContent, aDoc)
> {
>   mFlags |= eListControlAccessible;
>+  mFlags &= ~eIsPrimaryForNode;
> }
>-bool
>-HTMLComboboxListAccessible::IsPrimaryForNode() const
>-{
>-  return false;
>-}

same here

> XULTreeAccessible::
>   XULTreeAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>   AccessibleWrap(aContent, aDoc)
> {
>   mFlags |= eXULTreeAccessible;
>+  mFlags &= ~eIsPrimaryForNode;
>-bool
>-XULTreeItemAccessibleBase::IsPrimaryForNode() const
>-{
>-  return false;
>-}
and here

> XULTreeGridAccessible::
>   XULTreeGridAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>   XULTreeAccessible(aContent, aDoc), xpcAccessibleTable(this)
> {
>+  mFlags &= ~eIsPrimaryForNode;
> }
>-bool
>-XULTreeGridCellAccessible::IsPrimaryForNode() const
>-{
>-  return false;
>-}

also here

other than those the approach is good.

Comment 35

[Eric Wong (:ekw)]

Attachment: patch

Fixed errors in previous patch.

Comment 36

[Mark Capella [:capella]]

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

I'm not a reviewer for this group ... moving to surkov ...

Comment 37

[alexander :surkov (:asurkov)]

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

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

r=me. I'm fine to get the patch after nits are fixed but I think I'd prefer the opposite constant to eIsPrimaryForNode so that you set it up instead unsetting it. For example
mFlags |= eSharedNode or eLeasedNode

Let's get Trevor's opinion and then we can either file a follow up bug or if you don't mind then you can do another round here. thank you for the patch.

::: accessible/src/generic/Accessible.cpp
@@ +158,5 @@
>  }
>  
>  Accessible::Accessible(nsIContent* aContent, DocAccessible* aDoc) :
>    nsAccessNodeWrap(aContent, aDoc),
> +  mParent(nullptr), mIndexInParent(-1), 

nit: space at the end of line

::: accessible/src/generic/Accessible.h
@@ +692,5 @@
> +  /**
> +   * Return true if this accessible is the primary one for its node.
> +   */
> +  bool IsPrimaryForNode() const { return mFlags & eIsPrimaryForNode; }
> +  

nit: spaces at new line

@@ +743,5 @@
>     */
>    enum StateFlags {
>      eIsDefunct = 1 << 2, // accessible is defunct
> +    eIsNotInDocument = 1 << 3, // accessible is not in document
> +    eIsPrimaryForNode = 1 << 4

nit: add a comment please

::: accessible/src/html/HTMLListAccessible.cpp
@@ +142,5 @@
>  // HTMLListBulletAccessible
>  ////////////////////////////////////////////////////////////////////////////////
> +HTMLListBulletAccessible::
> +  HTMLListBulletAccessible(nsIContent* aContent, DocAccessible* aDoc) :
> +  LeafAccessible(aContent, aDoc) 

nit: space at the end of line

@@ +145,5 @@
> +  HTMLListBulletAccessible(nsIContent* aContent, DocAccessible* aDoc) :
> +  LeafAccessible(aContent, aDoc) 
> +{
> +  mFlags &= ~eIsPrimaryForNode;
> +}

I'm not sure about uninlining it. Not big deal but we might want to file a follow up bug to inline all constructors. Trev, what do you think?

Comment 38

[Eric Wong (:ekw)]

Attachment: patch

Fix nits from review. 

Wouldn't mind doing another round here to do the "opposite" constant if that's the decision.

Comment 39

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #37)
> Comment on attachment 657781 [details] [diff] [review]
> patch
> 
> Review of attachment 657781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. I'm fine to get the patch after nits are fixed but I think I'd prefer
> the opposite constant to eIsPrimaryForNode so that you set it up instead
> unsetting it. For example
> mFlags |= eSharedNode or eLeasedNode
> 
> Let's get Trevor's opinion and then we can either file a follow up bug or if
> you don't mind then you can do another round here. thank you for the patch.

yeah, I was thinking about that too, it sounds like a good idea.

> @@ +145,5 @@
> > +  HTMLListBulletAccessible(nsIContent* aContent, DocAccessible* aDoc) :
> > +  LeafAccessible(aContent, aDoc) 
> > +{
> > +  mFlags &= ~eIsPrimaryForNode;
> > +}
> 
> I'm not sure about uninlining it. Not big deal but we might want to file a
> follow up bug to inline all constructors. Trev, what do you think?

I'm not sure about all, but I suspect inlining a lot of them would be a good idea, but it might be a good idea to check the generated code before doing this.

Comment 40

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #39)

> > mFlags |= eSharedNode or eLeasedNode
> > 
> > Let's get Trevor's opinion and then we can either file a follow up bug or if
> > you don't mind then you can do another round here. thank you for the patch.
> 
> yeah, I was thinking about that too, it sounds like a good idea.

What constant name do you like more?

> I'm not sure about all, but I suspect inlining a lot of them would be a good
> idea, but it might be a good idea to check the generated code before doing
> this.

can you take a look when you get a minute and file a bug if it's reasonable.

Comment 41

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #40)
> (In reply to Trevor Saunders (:tbsaunde) from comment #39)
> 
> > > mFlags |= eSharedNode or eLeasedNode
> > > 
> > > Let's get Trevor's opinion and then we can either file a follow up bug or if
> > > you don't mind then you can do another round here. thank you for the patch.
> > 
> > yeah, I was thinking about that too, it sounds like a good idea.
> 
> What constant name do you like more?

eSharedNode I guess

Comment 42

[Eric Wong (:ekw)]

The other StateFlags enums are eIsDefunct and eIsNotInDocument.  
Should I make this new one eIsSharedNode?

Comment 43

[Eric Wong (:ekw)]

So I think the reason it was done this way instead of the "opposite" way is because with this way, only 6 classes need to unset the flag.  The opposite way requires setting the flag in 16+ classes and I can't be sure I caught all of them yet.  Is there an easy way to find all children/grand-children/great-grand-children/etc of the Accessible class?

Comment 44

[alexander :surkov (:asurkov)]

(In reply to Eric Wong (:ekw) from comment #42)
> The other StateFlags enums are eIsDefunct and eIsNotInDocument.  
> Should I make this new one eIsSharedNode?

I think yes, you should add new eSharedNode constant

(In reply to Eric Wong (:ekw) from comment #43)
> So I think the reason it was done this way instead of the "opposite" way is
> because with this way, only 6 classes need to unset the flag.  The opposite
> way requires setting the flag in 16+ classes and I can't be sure I caught
> all of them yet.  Is there an easy way to find all
> children/grand-children/great-grand-children/etc of the Accessible class?

You don't need that. Say you don't set eSharedNode on Accessible class and thus
IsPrimaryNode() { returm !(mFlags & eSharedNode); } returns true on it.
Those 6 classes have mFlags |= eSharedNode in constsructor and thus
IsPrimaryNode() { returm !(mFlags & eSharedNode); } returns false on them.

Comment 45

[Eric Wong (:ekw)]

Attachment: patch

Thanks for the clarification.  This patch implements the eSharedNode constant.

Comment 46

[alexander :surkov (:asurkov)]

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

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

r=me, thanks!

::: accessible/src/base/nsAccessNode.h
@@ -101,5 @@
> -   *
> -   * Accessible hierarchy may be complex for single DOM node, in this case
> -   * these accessibles share the same DOM node. The primary accessible "owns"
> -   * that DOM node in terms it gets stored in the accessible to node map.
> -   */

it'd be nice to keep this comment for Accessible::IsPrimaryForNode

::: accessible/src/generic/Accessible.h
@@ +743,5 @@
>     */
>    enum StateFlags {
>      eIsDefunct = 1 << 2, // accessible is defunct
> +    eIsNotInDocument = 1 << 3, // accessible is not in document
> +    eSharedNode = 1 << 4 // accessible is shared node

maybe: accessible shares DOM node from another accessible

::: accessible/src/generic/ApplicationAccessible.cpp
@@ +26,5 @@
>  ApplicationAccessible::ApplicationAccessible() :
>    AccessibleWrap(nullptr, nullptr)
>  {
>    mFlags |= eApplicationAccessible;
> +  mFlags |= eSharedNode;

you can do mFlags |= eApplicationAccessible | eSharedNode but it's not big deal since app accessible doesn't really has a node and keeping it as eSharedNode is workaround of some crash I think.

::: accessible/src/html/HTMLImageMapAccessible.cpp
@@ -189,5 @@
> -bool
> -HTMLAreaAccessible::IsPrimaryForNode() const
> -{
> -  // Make HTML area DOM element not accessible. HTML image map accessible
> -  // manages its tree itself.

please keep this comment

Comment 47

[Eric Wong (:ekw)]

Attachment: patch

Incorporated comments from latest review.

"python runtests.py --a11y --autorun" --> tests are green

Comment 48

[alexander :surkov (:asurkov)]

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

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

in general you don't need to rerequest review for nits

Comment 49

[Andrew Quartey [:drexler]]

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=22133598589f

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0fcb69b26e26