739198 - stop GetAccService()->GetAccessible usage in AccEvent::GetAccessibleForNode

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

We need to do things explicitly, i.e. need to switch to nsDocAccessile::GetAccessible. Events initialized by nsINode (instead of nsAccessible instance) aren't target of non primary presshell documents. So we can do:

nsDocAccessible* document = GetAccService()->GetDocAccessible(mNode->OwnerDoc());
if (document) {
  document->GetAccessible(mNode);
}

alternative is we can keep nsDocAccessible* instance at AccEvent and require it when AccEvent is created like
AccEvent(nsDocAccessible* aDocument, nsINode* aNode)

but not sure it's worth to do since the first approach should work. Trevor?

Comment 1

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #0)
> We need to do things explicitly, i.e. need to switch to
> nsDocAccessile::GetAccessible. Events initialized by nsINode (instead of
> nsAccessible instance) aren't target of non primary presshell documents. So
> we can do:
> 
> nsDocAccessible* document =
> GetAccService()->GetDocAccessible(mNode->OwnerDoc());
> if (document) {
>   document->GetAccessible(mNode);
> }

I'd be fine with that although I'd probably just reuse GetDocAccessible() on AccEvent.

> alternative is we can keep nsDocAccessible* instance at AccEvent and require
> it when AccEvent is created like
> AccEvent(nsDocAccessible* aDocument, nsINode* aNode)
> 
> but not sure it's worth to do since the first approach should work. Trevor?

well, if you were going to do that you could just get the accessible from the document and pass that to the AccEvent constructor no?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> (In reply to alexander :surkov from comment #0)
> > We need to do things explicitly, i.e. need to switch to
> > nsDocAccessile::GetAccessible. Events initialized by nsINode (instead of
> > nsAccessible instance) aren't target of non primary presshell documents. So
> > we can do:
> > 
> > nsDocAccessible* document =
> > GetAccService()->GetDocAccessible(mNode->OwnerDoc());
> > if (document) {
> >   document->GetAccessible(mNode);
> > }
> 
> I'd be fine with that although I'd probably just reuse GetDocAccessible() on
> AccEvent.

agree

> > alternative is we can keep nsDocAccessible* instance at AccEvent and require
> > it when AccEvent is created like
> > AccEvent(nsDocAccessible* aDocument, nsINode* aNode)
> > 
> > but not sure it's worth to do since the first approach should work. Trevor?
> 
> well, if you were going to do that you could just get the accessible from
> the document and pass that to the AccEvent constructor no?

nah, we have a logic where accessible might be not constructed yet

Comment 3

[Trevor Saunders (:tbsaunde)]

> > alternative is we can keep nsDocAccessible* instance at AccEvent and require
> > it when AccEvent is created like
> > AccEvent(nsDocAccessible* aDocument, nsINode* aNode)
> > 
> > but not sure it's worth to do since the first approach should work. Trevor?
> 
> well, if you were going to do that you could just get the accessible from
> the document and pass that to the AccEvent constructor no?

I haven't looked at all the places we create events yet, just a bunch, but I'll bet we could get a lot closer to events only operating on accessibles while we're here without a whole lot of work.  I'd suggest converting a chunk of related events at a time and fixing this bug in multiple small patches.  Once we get all the low hanging fruit of events that could easily be created with an accessible we can consider what to do with any others if we have to.(In reply to alexander :surkov from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> > (In reply to alexander :surkov from comment #0)
> > > We need to do things explicitly, i.e. need to switch to
> > > nsDocAccessile::GetAccessible. Events initialized by nsINode (instead of
> > > nsAccessible instance) aren't target of non primary presshell documents. So
> > > we can do:
> > > 
> > > nsDocAccessible* document =
> > > GetAccService()->GetDocAccessible(mNode->OwnerDoc());
> > > if (document) {
> > >   document->GetAccessible(mNode);
> > > }
> > 
> > I'd be fine with that although I'd probably just reuse GetDocAccessible() on
> > AccEvent.
> 
> agree
> 
> > > alternative is we can keep nsDocAccessible* instance at AccEvent and require
> > > it when AccEvent is created like
> > > AccEvent(nsDocAccessible* aDocument, nsINode* aNode)
> > > 
> > > but not sure it's worth to do since the first approach should work. Trevor?
> > 
> > well, if you were going to do that you could just get the accessible from
> > the document and pass that to the AccEvent constructor no?
> 
> nah, we have a logic where accessible might be not constructed yet

yeah, well, then we'll end up with crazy results when coalescing events gets the accessible for the event or uses AccEvent->Accessible directly.  I suppose we could keep track of which event types and coalescing rules allows for this case and which don't, but I'm pretty sure I'm not smart enough to get that correct.

THere are also some cases where the accessible must be constructed because we pass mDocument or mContent to the constructor.  I suppose the accessible could get  recreated, but the current behavior for that case is already not predictable.

Comment 4

[alexander :surkov (:asurkov)]

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

> I haven't looked at all the places we create events yet, just a bunch, but
> I'll bet we could get a lot closer to events only operating on accessibles
> while we're here without a whole lot of work.  I'd suggest converting a
> chunk of related events at a time and fixing this bug in multiple small
> patches.  Once we get all the low hanging fruit of events that could easily
> be created with an accessible we can consider what to do with any others if
> we have to.

I must admit that I used to think this way, I cannot say I changed my mind entirely though but now I think of this as a nice "shortcut" of notification processing after accessible is created. That's why I'm not in hurry to get rid that code. Code unification is wanted but having light-weight other paths is tempting.

> yeah, well, then we'll end up with crazy results when coalescing events gets
> the accessible for the event or uses AccEvent->Accessible directly.

that's a downside of other tempting paths :)

>  I
> suppose we could keep track of which event types and coalescing rules allows
> for this case and which don't, but I'm pretty sure I'm not smart enough to
> get that correct.

I think we could keep this approach working for state change events which are coalesced between each other by target.

> THere are also some cases where the accessible must be constructed because
> we pass mDocument or mContent to the constructor.

I don't follow this.

> I suppose the accessible
> could get  recreated, but the current behavior for that case is already not
> predictable.

must be in connection with previous sentence so can't comment it now

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> 
> > I haven't looked at all the places we create events yet, just a bunch, but
> > I'll bet we could get a lot closer to events only operating on accessibles
> > while we're here without a whole lot of work.  I'd suggest converting a
> > chunk of related events at a time and fixing this bug in multiple small
> > patches.  Once we get all the low hanging fruit of events that could easily
> > be created with an accessible we can consider what to do with any others if
> > we have to.
> 
> I must admit that I used to think this way, I cannot say I changed my mind
> entirely though but now I think of this as a nice "shortcut" of notification
> processing after accessible is created. That's why I'm not in hurry to get
> rid that code. Code unification is wanted but having light-weight other
> paths is tempting.

I guess, on the other hand it seems like these other  paths can lead to slightly crazy things like a state change event on an accessible to a start that it had when it ws created because of the timing.   or wasted effort like fireing an event on content that it turns out we will never create an accessible for.

> >  I
> > suppose we could keep track of which event types and coalescing rules allows
> > for this case and which don't, but I'm pretty sure I'm not smart enough to
> > get that correct.
> 
> I think we could keep this approach working for state change events which
> are coalesced between each other by target.

you mean only look at the accessible when coalescing if its a state change event?  afaik most of the cases we could create an event without having an accessible yet are for state changes so  this seems a little strange.

> > THere are also some cases where the accessible must be constructed because
> > we pass mDocument or mContent to the constructor.
> 
> I don't follow this.

if we create an event with mDocument as the target then clearly the target will be the doc accessible for that document, which we know exists because we are the doc accessible for that document.  I'm not sure how this would work in a world of multiple doc accessibles for one dom document,  though.

Other examples of this are passing accessible->GetNode() or mContent as the node, unless accessible or this respectively is a secondary accessible for that content, but I really suspect that isn't the case.

> > I suppose the accessible
> > could get  recreated, but the current behavior for that case is already not
> > predictable.
> 
> must be in connection with previous sentence so can't comment it now

Comment 6

[alexander :surkov (:asurkov)]

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

> I guess, on the other hand it seems like these other  paths can lead to
> slightly crazy things like a state change event on an accessible to a start
> that it had when it ws created because of the timing.   or wasted effort
> like fireing an event on content that it turns out we will never create an
> accessible for.

ok, good point. Do you think we should fix this bug as a set of bugs removing the firing of accessible events with DOM node target?

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > I guess, on the other hand it seems like these other  paths can lead to
> > slightly crazy things like a state change event on an accessible to a start
> > that it had when it ws created because of the timing.   or wasted effort
> > like fireing an event on content that it turns out we will never create an
> > accessible for.
> 
> ok, good point. Do you think we should fix this bug as a set of bugs
> removing the firing of accessible events with DOM node target?

Well, I'd certainly start by getting rid of all the easy cases, if that does all of them great.

Comment 8

[alexander :surkov (:asurkov)]

Ok, removing good-first-bug status for now. I'll take a look closer and file blocking bugs.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> Ok, removing good-first-bug status for now. I'll take a look closer and file
> blocking bugs.

sounds good

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v1)

Initial try, asking Surkov for feedback? since he's been stealing them lately :)

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 624508 [details] [diff] [review]
Patch (v1)

I wanted to get rid mNode from AccEvent at all but I didn't get close to this enough. So the patch goes with bug description and it doesn't make sense to stop it.

Comment 12

[Mark Capella [:capella]]

Do you want to review+ this one then and let me push it forward? (I have a few small patches I'll push together).

I saw that this was the last blocker on bug 725572 ... you could update that to describe whats left overall?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #12)
> Do you want to review+ this one then and let me push it forward? (I have a
> few small patches I'll push together).

please ask extra review from Trevor (iirc he had some thoughts on this bug).

> I saw that this was the last blocker on bug 725572 ... you could update that
> to describe whats left overall?

I'll update it when this one is fixed (or you can do that).

Comment 14

[Mark Capella [:capella]]

Comment on attachment 624508 [details] [diff] [review]
Patch (v1)

Ok, asking Trev for review+ on this portion of the blocker bug only.

Note that there is still extra work to finish within the umbrella bug 725572 to complete the "train of thought" -- mark

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 624508 [details] [diff] [review]
Patch (v1)

changing feedback on r+ (let's deal in follow up if Trevor has concerns, anyway this is temporary approach that should go away in nearest future so we can be not nitpicky here).

Comment 16

[Mark Capella [:capella]]

Push to try https://tbpl.mozilla.org/?tree=Try&rev=3591e146146d

Comment 17

[Mark Capella [:capella]]

Due to a TRY failure related to https://bugzilla.mozilla.org/show_bug.cgi?id=712205 i had to re-push to TRY:

https://tbpl.mozilla.org/?tree=Try&rev=a94caf8233cd

Comment 18

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f120010dd2bf

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f120010dd2bf