Closed
Bug 739198
Opened 13 years ago
Closed 13 years ago
stop GetAccService()->GetAccessible usage in AccEvent::GetAccessibleForNode
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: surkov, Assigned: capella)
References
Details
Attachments
(1 file)
1.07 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•13 years ago
|
Comment 1•13 years ago
|
||
(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?
Reporter | ||
Comment 2•13 years ago
|
||
(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•13 years ago
|
||
> > 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.
Reporter | ||
Comment 4•13 years ago
|
||
(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•13 years ago
|
||
(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
Reporter | ||
Comment 6•13 years ago
|
||
(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•13 years ago
|
||
(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.
Reporter | ||
Comment 8•13 years ago
|
||
Ok, removing good-first-bug status for now. I'll take a look closer and file blocking bugs.
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++]
Comment 9•13 years ago
|
||
(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
Assignee | ||
Comment 10•13 years ago
|
||
Initial try, asking Surkov for feedback? since he's been stealing them lately :)
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #624508 -
Flags: feedback?(surkov.alexander)
Reporter | ||
Comment 11•13 years ago
|
||
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.
Attachment #624508 -
Flags: feedback?(surkov.alexander) → feedback+
Assignee | ||
Comment 12•13 years ago
|
||
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?
Reporter | ||
Comment 13•13 years ago
|
||
(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).
Assignee | ||
Comment 14•13 years ago
|
||
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
Attachment #624508 -
Flags: review?(trev.saunders)
Reporter | ||
Comment 15•13 years ago
|
||
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).
Attachment #624508 -
Flags: review?(trev.saunders)
Attachment #624508 -
Flags: review+
Attachment #624508 -
Flags: feedback+
Assignee | ||
Comment 16•13 years ago
|
||
Assignee | ||
Comment 17•13 years ago
|
||
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
Assignee | ||
Comment 18•13 years ago
|
||
Target Milestone: --- → mozilla15
Comment 19•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•