figure out a way to deal with accessibles with no content

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks 2 bugs)

unspecified
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

7 years ago
we have three types of accessibles with no content:
1) no content at all like application accessible (nsApplicationAccessible) and native accessibles (atk/nsNativeRootAccessibleWrap and msaa/nsHTMLWin32ObjectAccessible)
2) document accessible (nsDocAccessible), mContent can be null, not now but in the future
3) not primary node accessbile (nsXULTreeItemAccessible and others), mContent is not null but all operations on mContent doesn't make any sense.

At the first glance all of them can be dealt together, i.e. introduce eHasContent state flag and make all accessibles to expose this flag if mContent doesn't make sense for it. Any public method that operates on mContent should make sure eHasContent state flag is not presented.
Assignee

Comment 1

7 years ago
we had couple crashes because of this: bug 741053 and bug 740958
Just finished bug 760345 (eIsNotInDocument state flag) ... and I've a couple questions ...

Shouldn't this new state flag be eHasNoContent? Then ...

#1 we add mFlags |= eHasNoContent; to ApplicationAccessible class, NativeRootAccessibleWrap class, and nsHTMLWin32ObjectAccessible class constructors ?

#2 comes later though I need more information about this point ...

#3 we add for classes where IsPrimaryForNode() returns false like XULTreeGridCellAccessible, XULTreeItemAccessibleBase, HTMLListBulletAccessible, HTMLComboboxListAccessible, HTMLAreaAccessible ?
Assignee

Comment 3

7 years ago
#1, #3 correct
#2 I think we have bugs on this

It seems we could go with inline
bool HasContent() { return mContent && IsPrimaryForNode(); }

instead of having extra flag.
> bool HasContent() { return mContent && IsPrimaryForNode(); }
> 
> instead of having extra flag.

Well, why don't we merge this with making IsPrimaryForNode a flag, since being the primary accessible for no node doesn't really make much sense.

Another question I have is is it allowed for an accessible to be alive (not defunct and still have a node?).

Does it ever make sense to check that the accessible has a node before calling the public method say to decrease the number of times it needs to be checked?
Assignee

Comment 5

7 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > bool HasContent() { return mContent && IsPrimaryForNode(); }
> > 
> > instead of having extra flag.
> 
> Well, why don't we merge this with making IsPrimaryForNode a flag, since
> being the primary accessible for no node doesn't really make much sense.

it seems we can have one flag for both like eHasContent (eHasNoContent as Mark suggested).

> Another question I have is is it allowed for an accessible to be alive (not
> defunct and still have a node?).

I don't get your question: not defunct and have a node since most of accessibles complies this definition.

> Does it ever make sense to check that the accessible has a node before
> calling the public method say to decrease the number of times it needs to be
> checked?

sounds unsafe since the caller should know implementation of the method.
(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > > bool HasContent() { return mContent && IsPrimaryForNode(); }
> > > 
> > > instead of having extra flag.
> > 
> > Well, why don't we merge this with making IsPrimaryForNode a flag, since
> > being the primary accessible for no node doesn't really make much sense.
> 
> it seems we can have one flag for both like eHasContent (eHasNoContent as
> Mark suggested).
> 
> > Another question I have is is it allowed for an accessible to be alive (not
> > defunct and still have a node?).
> 
> I don't get your question: not defunct and have a node since most of
> accessibles complies this definition.

I probably mean is it ok to have an accessible who is defunct, and has a node, but I guess you think not too if a single flag will do.

> > Does it ever make sense to check that the accessible has a node before
> > calling the public method say to decrease the number of times it needs to be
> > checked?
> 
> sounds unsafe since the caller should know implementation of the method.

Well, they'd just know the method requires it only be called on accessibles with a node, which is pretty similar to know it should only be called on non-defunct things.

Another question, what can methods assume not being defunct means they can do or touch?
Assignee

Comment 7

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

> I probably mean is it ok to have an accessible who is defunct, and has a
> node, but I guess you think not too if a single flag will do.

yes, defunct and has a node means something wrong

> > > Does it ever make sense to check that the accessible has a node before
> > > calling the public method say to decrease the number of times it needs to be
> > > checked?
> > 
> > sounds unsafe since the caller should know implementation of the method.
> 
> Well, they'd just know the method requires it only be called on accessibles
> with a node, which is pretty similar to know it should only be called on
> non-defunct things.

non-defunct things are possible since we are guaranteed that we don't deal with defuncts internally. This case must be more complicated. Especially for methods that has logic for accessible having a node and universal logic (like GetAttributes()).

> Another question, what can methods assume not being defunct means they can
> do or touch?

Can you rephrase it please?
> > Another question, what can methods assume not being defunct means they can
> > do or touch?
> 
> Can you rephrase it please?

so, what does it mean that an accessible isn't defunct? what becomes safe to do with a node that isn't defunct that wasn't safe to do before you knew it wasn't defunct?  What is the purpose of the defunct check?
Assignee

Comment 9

7 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > > Another question, what can methods assume not being defunct means they can
> > > do or touch?
> > 
> > Can you rephrase it please?
> 
> so, what does it mean that an accessible isn't defunct? what becomes safe to
> do with a node that isn't defunct that wasn't safe to do before you knew it
> wasn't defunct?  What is the purpose of the defunct check?

the propose of defuct is AT can keep an accessible longer than we want. Thus all external calls must be checked whether the given accessible is defunct, at least that's what we do now.

if we introduce no node acceessible into a low then no any methods working on node is not safe without a check.

non primary for node accessibles are safe for operations on nodes but they may give incorrect results.

It seems it makes sense to have a flag for non primary nodes case and inline method for no content case like I suggested above. It seems it's most flexible approach. Evey method decides what it needs to check before to proceed.
Assignee

Comment 10

7 years ago
Posted patch part1 (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #669388 - Flags: review?(trev.saunders)
Assignee

Updated

7 years ago
Attachment #669388 - Attachment is obsolete: true
Attachment #669388 - Flags: review?(trev.saunders)
Assignee

Comment 11

7 years ago
Posted patch patch (obsolete) — Splinter Review
It's not completely safe but bug 741053 and bug 740958 shouldn't appear again. Polishing can happen in follow up bugs.

summary of changes
1) add eHasNoOwnContent flag for accessible that don't have a node or share a node
2) add eNotInNodeMap flag for area accessible since it shouldn't be presented in node map but all content stuff are valid for it
3) any public method of Accessible class should be safe and correct when operates on mContent
3.1) add HasOwnContent() checks where it's reasonable
3.2) introduce DumbAccessible to avoid HasOwnContent() extra checks
Attachment #669493 - Flags: review?(trev.saunders)
Assignee

Comment 12

7 years ago
(In reply to alexander :surkov from comment #11)

> 1) add eHasNoOwnContent flag for accessible that don't have a node or share
> a node

however I could keep eSharedNode flag and add
HasOwnContent() { return mContent && !(mFlags & eSharedNode); }

and it seems be nicer
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla14
Version: unspecified → 17 Branch
Assignee

Updated

7 years ago
Status: NEW → ASSIGNED
Target Milestone: mozilla14 → ---
Version: 17 Branch → unspecified
Assignee

Comment 13

7 years ago
Posted patch patch2 (obsolete) — Splinter Review
keep eSharedNode flag
Attachment #669493 - Attachment is obsolete: true
Attachment #669493 - Flags: review?(trev.saunders)
Attachment #669559 - Flags: review?(trev.saunders)
Comment on attachment 669559 [details] [diff] [review]
patch2


>-class NativeRootAccessibleWrap : public RootAccessible
>+class NativeRootAccessibleWrap : public DumbAccessible

rename it to GtkWindowAccessible or GtkDialogAccessible?

>+#ifdef MOZ_ACCESSIBILITY_ATK
>+#include "AtkSocketAccessible.h"
>+#endif

you might want to be careful moving that include around iirc we had weird anoying dependancy type issues with it before

> Accessible::Name(nsString& aName)
> {
>   aName.Truncate();
> 
>-  GetARIAName(aName);
>+  if (!HasOwnContent())
>+    return eNameOK;

why not just have DumbAccessible override it?

>+  if (HasOwnContent() && mContent->IsElement()) {
>     nsEventStates elementState = mContent->AsElement()->State();
> 
>     if (elementState.HasState(NS_EVENT_STATE_INVALID))
>       state |= states::INVALID;
> 
>     if (elementState.HasState(NS_EVENT_STATE_REQUIRED))
>       state |= states::REQUIRED;

so, we won't do this stuff for <area> in a img map right? shouldn't we though?

>     // XXX we should look at layout for non XUL box frames, but need to decide
>     // how that interacts with ARIA.
>-    if (mContent->IsXUL() && frame->IsBoxFrame()) {
>+    if (HasOwnContent() && mContent->IsXUL() && frame->IsBoxFrame()) {

its not really clear to me that non owning accessible for content should never have the below check for horizontal / vertical states

> Accessible::SetSelected(bool aSelect)
> {
>-  if (IsDefunct())
>+  if (IsDefunct() || !HasOwnContent())
>     return NS_ERROR_FAILURE;

do we really want to throw when people try to call this on the wrong sort of accessible?  I don't think that's particularly bad, but slightly funny

>-Accessible::GetARIAName(nsAString& aName)
>+// Accessible protected
>+void
>+Accessible::ARIAName(nsAString& aName)

nit, blank line after that comment?


>+class DumbAccessible : public LeafAccessible
>+{
>+public:
>+  DumbAccessible() : LeafAccessible(nullptr, nullptr) { }
>+  virtual ~DumbAccessible() { }
>+
>+  virtual uint64_t NativeState();
>+  virtual uint64_t NativeInteractiveState() const;
>+  virtual uint64_t NativeLinkState() const;
>+  virtual bool NativelyUnavailable() const;

add MOZ_OVERRIDE MOZ_FINAL?

I still want ApplicationAccessible to inherit from DumbAccessible.  I don't want strange description, but I care less about that and whatever you want to rename DumbAccessible to than not having to override things in 2 places.
As for DumbAccessible being a LeafAccessible I think we could change it to just be a AccessibleWrap, but I don't think that's actually needed, LeafAccessible overrides accessible CacheChildren() and InvalidateChildren() but not  AppendChild / RemoveChild so iirc you can technically add children and remove them if you like, but admittedly that's kind of hacky
also, shouldn't atleast ARIATransformRole() and RelationByType get checks or over rides in DumbAccessible?  ApplyARIAState() too now that I think about it, and its probably worth a look at ApplicationAccessible to see what else it over rides
Assignee

Comment 16

7 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> >+#ifdef MOZ_ACCESSIBILITY_ATK
> >+#include "AtkSocketAccessible.h"
> >+#endif
> 
> you might want to be careful moving that include around iirc we had weird
> anoying dependancy type issues with it before

includes order might be a problem?

> > Accessible::Name(nsString& aName)
> > {
> >+  if (!HasOwnContent())
> >+    return eNameOK;
> 
> why not just have DumbAccessible override it?

DocAccessible relies on it (no content case), maybe other shared node accessibles have this dependency. Either we make sure that all !HasOwnContent() nodes overrides Name or we have a check here. The second might be simpler.

> >+  if (HasOwnContent() && mContent->IsElement()) {
> >     nsEventStates elementState = mContent->AsElement()->State();
> > 
> >     if (elementState.HasState(NS_EVENT_STATE_INVALID))
> >       state |= states::INVALID;
> > 
> >     if (elementState.HasState(NS_EVENT_STATE_REQUIRED))
> >       state |= states::REQUIRED;
> 
> so, we won't do this stuff for <area> in a img map right? shouldn't we
> though?

this code is invoked for <area>s. It doesn't seem needed though but it's not applicable I guess to a number of accessbile classes.

> >-    if (mContent->IsXUL() && frame->IsBoxFrame()) {
> >+    if (HasOwnContent() && mContent->IsXUL() && frame->IsBoxFrame()) {
> 
> its not really clear to me that non owning accessible for content should
> never have the below check for horizontal / vertical states

true but we don't have usecases for this code iirc. Or can you think of any shared node accessible having these states?

> 
> > Accessible::SetSelected(bool aSelect)
> > {
> >+  if (IsDefunct() || !HasOwnContent())
> >     return NS_ERROR_FAILURE;
> 
> do we really want to throw when people try to call this on the wrong sort of
> accessible?  I don't think that's particularly bad, but slightly funny

it should return ok, agree.

> >+class DumbAccessible : public LeafAccessible
> >+  virtual bool NativelyUnavailable() const;
> 
> add MOZ_OVERRIDE MOZ_FINAL?

ok
Assignee

Comment 17

7 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #15)
> also, shouldn't atleast ARIATransformRole() and RelationByType get checks or
> over rides in DumbAccessible?  ApplyARIAState() too now that I think about
> it, and its probably worth a look at ApplicationAccessible to see what else
> it over rides

It's a first round, I think I'll continue the work as follow up bugs. But what we have here should be ok for landing on Firefox 19.
Assignee

Comment 18

7 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #14)

> I still want ApplicationAccessible to inherit from DumbAccessible.

agree, it makes sense

>  I don't
> want strange description, but I care less about that and whatever you want
> to rename DumbAccessible to than not having to override things in 2 places.

which one suites more: dumb, dummy, stub?

> As for DumbAccessible being a LeafAccessible I think we could change it to
> just be a AccessibleWrap, but I don't think that's actually needed,

for those native accessible I'd prefer to assert if somebody tries to manage its children rather than hide an error by LeafAccessible inheritance.

> LeafAccessible overrides accessible CacheChildren() and InvalidateChildren()
> but not  AppendChild / RemoveChild so iirc you can technically add children
> and remove them if you like, but admittedly that's kind of hacky

yea, LeafAccessible wants to override those too I guess.
(In reply to alexander :surkov from comment #18)
> (In reply to Trevor Saunders (:tbsaunde) from comment #14)
> 
> > I still want ApplicationAccessible to inherit from DumbAccessible.
> 
> agree, it makes sense

so, what are you going to do there?

> >  I don't
> > want strange description, but I care less about that and whatever you want
> > to rename DumbAccessible to than not having to override things in 2 places.
> 
> which one suites more: dumb, dummy, stub?

dummy probably makes the most sense

> > As for DumbAccessible being a LeafAccessible I think we could change it to
> > just be a AccessibleWrap, but I don't think that's actually needed,
> 
> for those native accessible I'd prefer to assert if somebody tries to manage
> its children rather than hide an error by LeafAccessible inheritance.
> 
> > LeafAccessible overrides accessible CacheChildren() and InvalidateChildren()
> > but not  AppendChild / RemoveChild so iirc you can technically add children
> > and remove them if you like, but admittedly that's kind of hacky
> 
> yea, LeafAccessible wants to override those too I guess.

probably makes sense to fix that, file a bug?
(In reply to alexander :surkov from comment #16)
> (In reply to Trevor Saunders (:tbsaunde) from comment #14)
> > >+#ifdef MOZ_ACCESSIBILITY_ATK
> > >+#include "AtkSocketAccessible.h"
> > >+#endif
> > 
> > you might want to be careful moving that include around iirc we had weird
> > anoying dependancy type issues with it before
> 
> includes order might be a problem?

maybe, I don't really remember

> > > Accessible::Name(nsString& aName)
> > > {
> > >+  if (!HasOwnContent())
> > >+    return eNameOK;
> > 
> > why not just have DumbAccessible override it?
> 
> DocAccessible relies on it (no content case), maybe other shared node
> accessibles have this dependency. Either we make sure that all
> !HasOwnContent() nodes overrides Name or we have a check here. The second
> might be simpler.

maybe, on the other hand you seem to have thought it made more sense to override NativelyUnavailable and a few others, not really clear to me  how you choose or why.

> > >     nsEventStates elementState = mContent->AsElement()->State();
> > > 
> > >     if (elementState.HasState(NS_EVENT_STATE_INVALID))
> > >       state |= states::INVALID;
> > > 
> > >     if (elementState.HasState(NS_EVENT_STATE_REQUIRED))
> > >       state |= states::REQUIRED;
> > 
> > so, we won't do this stuff for <area> in a img map right? shouldn't we
> > though?
> 
> this code is invoked for <area>s. It doesn't seem needed though but it's not
> applicable I guess to a number of accessbile classes.

I don't see how that code runs, HTMLAreaAccessible has eSharedNode, so HasOwnContent() returns false, and this code is skipped.  I'd think since <area> is sort of link that these states should be exposed on area accessible if applicable.

> > >-    if (mContent->IsXUL() && frame->IsBoxFrame()) {
> > >+    if (HasOwnContent() && mContent->IsXUL() && frame->IsBoxFrame()) {
> > 
> > its not really clear to me that non owning accessible for content should
> > never have the below check for horizontal / vertical states
> 
> true but we don't have usecases for this code iirc. Or can you think of any
> shared node accessible having these states?

tree item type thing maybe? I'm not really sure if those can be boxes.
Assignee

Comment 21

7 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #20)

> > > you might want to be careful moving that include around iirc we had weird
> > > anoying dependancy type issues with it before
> > 
> > includes order might be a problem?
> 
> maybe, I don't really remember

it was ok on try server

> maybe, on the other hand you seem to have thought it made more sense to
> override NativelyUnavailable and a few others, not really clear to me  how
> you choose or why.

I don't have a strong rule here. In general it depends whether there are other overrides. I think we can sort it out eventually.

> I don't see how that code runs, HTMLAreaAccessible has eSharedNode, so

<area> accessible doesn't have eSharedNode

> > true but we don't have usecases for this code iirc. Or can you think of any
> > shared node accessible having these states?
> 
> tree item type thing maybe? I'm not really sure if those can be boxes.

treeitems don't have frames
Assignee

Comment 22

7 years ago
Posted patch patch3Splinter Review
I'm still not certain that dummy accessible suites for application accessible well.

1) it's not dummy (name conflicts)
2) it needs to override a number of methods from dummy class which are defined as MOZ_FINAL

I agree it looks close to dummy accessible since it needs many dummy methods too. I start thinking that I would keep it separate. In either case I wouldn't want to take decision in this bug.
Attachment #669559 - Attachment is obsolete: true
Attachment #669559 - Flags: review?(trev.saunders)
Attachment #670678 - Flags: review?(trev.saunders)
Comment on attachment 670678 [details] [diff] [review]
patch3

>+class GtkWindowAccessible : public DummyAccessible

final? while your here

btw seems not the right file for this stuff anymore, file a bug for me to move it and get rid of those nsAccessibility service methods

>+  GtkWindowAccessible* nativeWnd =
>+    new GtkWindowAccessible(static_cast<AtkObject*>(aAtkAccessible));
> 
>-  if (applicationAcc->AppendChild(nativeRootAcc))
>-    return nativeRootAcc;
>+  if (applicationAcc->AppendChild(nativeWnd))
>+    return nativeWnd;

ref counting setup here is kind of scary, but I don't think you change that, and it seems fine.  I mean because App accessible is only one to ever AddRef() so removing it as a child deletes the accessible.

>+namespace a11y {
>+
> class nsHTMLWin32ObjectOwnerAccessible : public AccessibleWrap

nit, mozilla::a11y::nsHTMLBlah is kind of funny

>+class nsHTMLWin32ObjectAccessible : public mozilla::a11y::DummyAccessible

you don't need mozilla::a11y do you?
Attachment #670678 - Flags: review?(trev.saunders) → review+
Assignee

Comment 24

7 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #23)
> >+class GtkWindowAccessible : public DummyAccessible
> 
> btw seems not the right file for this stuff anymore, file a bug for me to
> move it and get rid of those nsAccessibility service methods

it's gtk analogue of our root accessible, what file is appropriate for it? I dind't get about accservice methods I need to get rid.

> >+  GtkWindowAccessible* nativeWnd =
> >+    new GtkWindowAccessible(static_cast<AtkObject*>(aAtkAccessible));
> > 
> >-  if (applicationAcc->AppendChild(nativeRootAcc))
> >-    return nativeRootAcc;
> >+  if (applicationAcc->AppendChild(nativeWnd))
> >+    return nativeWnd;
> 
> ref counting setup here is kind of scary, but I don't think you change that,
> and it seems fine.  I mean because App accessible is only one to ever
> AddRef() so removing it as a child deletes the accessible.

agree, we should be ok even if AppendChild fails

> >+namespace a11y {
> >+
> > class nsHTMLWin32ObjectOwnerAccessible : public AccessibleWrap
> 
> nit, mozilla::a11y::nsHTMLBlah is kind of funny

ok, I renamed those
(In reply to alexander :surkov from comment #24)
> (In reply to Trevor Saunders (:tbsaunde) from comment #23)
> > >+class GtkWindowAccessible : public DummyAccessible
> > 
> > btw seems not the right file for this stuff anymore, file a bug for me to
> > move it and get rid of those nsAccessibility service methods
> 
> it's gtk analogue of our root accessible, what file is appropriate for it? I
> dind't get about accservice methods I need to get rid.

I mean Add / Remove nativeRootAccessible() I'll do it, I've been meaning to for a while.  I'm not really sure where it should all go, maybe move them and the event gtk event watcher hook thing in ApplicationAccessible.cpp to GtkWindows.cpp or something no definite idea yet.
Assignee

Comment 26

7 years ago
I field bug 801222
https://hg.mozilla.org/mozilla-central/rev/4ee4e3aa9ea6

Should this have a test?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Assignee

Comment 29

7 years ago
(In reply to Ryan VanderMeulen from comment #28)

> Should this have a test?

no, it's big reorg and we need to add large set of tests what must be not easy, the same time the patch is plain enough to be scary about regressions.
Assignee

Comment 30

7 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #19)

> > yea, LeafAccessible wants to override those too I guess.
> 
> probably makes sense to fix that, file a bug?

bug 801356 filed
Assignee

Updated

7 years ago
Blocks: 612830
You need to log in before you can comment on or make changes to this bug.