678454 - coalesce events by accessible tree

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

We should make events firing against accessible tree rather than DOM tree so that event should be initialized by accessibles. I realize we should discover some exceptions on this way but that should work in general.

So ideal case is
1) remove DOM node from event constructors (and do not keep the node as member)(AccEvent.h/cpp files)
2) make event coalescence by accessible targets (NotificationController.cpp)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

note: test_attrs, I changed event-from-user: true for caret event. Now we don't repack that event, just file it so from-user-input value seems to be correct (bug 540285 where that test was introduced doesn't say much about that)

Comment 2

[Trevor Saunders (:tbsaunde)]

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

> AccEvent::CaptureIsFromUserInput(EIsFromUserInput aIsFromUserInput)
> {
>-  nsINode *targetNode = GetNode();
>+  nsINode* targetNode = mAccessible->GetContent();

so, all we do with that node is get a pres shell right? so why not do mAccessible->Document()->PresShell() ?

> 
> #ifdef DEBUG
>   if (!targetNode) {
>     // XXX: remove this hack during reorganization of 506907. Meanwhile we
>     // want to get rid an assertion for application accessible events which
>     // don't have DOM node (see bug 506206).
> 
>     if (mAccessible != static_cast<nsIAccessible*>(ApplicationAcc()))

won't we hit this case when doc accessible has no  mContent too?

>+  Accessible* GetAccessible() const { return mAccessible; }

Accessible() ?

>+  DocAccessible* GetDocAccessible() const
>+    { return mAccessible ? mAccessible->Document() : nullptr; }

DocAccessible()? how can mAccessible be null?

>+  AccStateChangeEvent(Accessible* aAccessible, uint64_t aState) :
>+    AccEvent(::nsIAccessibleEvent::EVENT_STATE_CHANGE, aAccessible,
>+             eAutoDetect, eAllowDupes), mState(aState)
>+    { mIsEnabled = (mAccessible->State() & mState) != 0; }

it seems silly to call State() when we could figure out if the state is enabled much faster where the event is created.

>         HyperTextAccessible* hyperText = target->AsHyperText();
>-        int32_t caretOffset = -1;
>         if (hyperText &&
>-          NS_SUCCEEDED(hyperText->GetCaretOffset(&caretOffset))) {
>-          nsRefPtr<AccEvent> caretMoveEvent =
>-            new AccCaretMoveEvent(hyperText, caretOffset);
>+            NS_SUCCEEDED(hyperText->GetCaretOffset(&caretMoveEvent->mCaretOffset))) {

why don't we do this at the point where we create caret move events, and then coalesce away the out of date ones?

>     const uint32_t kState = (aAttribute == nsGkAtoms::aria_checked) ?
>                             states::CHECKED : states::PRESSED;

shouldn't that be uint64_t ?

>   // Fire reorder event after the document tree is constructed. Note, since
>   // this reorder event is processed by parent document then events targeted to
>   // this document may be fired prior to this reorder event. If this is
>   // a problem then consider to keep event processing per tab document.
>   if (!IsRoot()) {
>     nsRefPtr<AccReorderEvent> reorderEvent = new AccReorderEvent(Parent());

shouldn't we not use AccReorderEvent here  to avoid coalescing?

Comment 3

[alexander :surkov (:asurkov)]

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

> > AccEvent::CaptureIsFromUserInput(EIsFromUserInput aIsFromUserInput)
> > {
> >-  nsINode *targetNode = GetNode();
> >+  nsINode* targetNode = mAccessible->GetContent();
> 
> so, all we do with that node is get a pres shell right? so why not do
> mAccessible->Document()->PresShell() ?

good point

> >+  Accessible* GetAccessible() const { return mAccessible; }
> 
> Accessible() ?

there's something wrong with that approach since Name as a type and Name() as a function are in conflict and we need to use namesapce to resolve it (see nsAccessNode::RootAccessible for example). So I left that unchanged.

> >+  DocAccessible* GetDocAccessible() const
> >+    { return mAccessible ? mAccessible->Document() : nullptr; }
> 
> DocAccessible()? how can mAccessible be null?

It shouldn't be, but it could be in future I think (though I'm not certain yet how better to introduce "no target" events). I'll remove it for now.

> 
> >+  AccStateChangeEvent(Accessible* aAccessible, uint64_t aState) :
> >+    AccEvent(::nsIAccessibleEvent::EVENT_STATE_CHANGE, aAccessible,
> >+             eAutoDetect, eAllowDupes), mState(aState)
> >+    { mIsEnabled = (mAccessible->State() & mState) != 0; }
> 
> it seems silly to call State() when we could figure out if the state is
> enabled much faster where the event is created.

yes, that bug is in my todo list of not filed bugs :)

> why don't we do this at the point where we create caret move events, and
> then coalesce away the out of date ones?

it's faster, it doesn't make sense to compute the caret for coalesced events (I guess we can get many events when user moves through the text).

> 
> >     const uint32_t kState = (aAttribute == nsGkAtoms::aria_checked) ?
> >                             states::CHECKED : states::PRESSED;
> 
> shouldn't that be uint64_t ?

you don't know the answer, right? ;)

> >   // Fire reorder event after the document tree is constructed. Note, since
> >   // this reorder event is processed by parent document then events targeted to
> >   // this document may be fired prior to this reorder event. If this is
> >   // a problem then consider to keep event processing per tab document.
> >   if (!IsRoot()) {
> >     nsRefPtr<AccReorderEvent> reorderEvent = new AccReorderEvent(Parent());
> 
> shouldn't we not use AccReorderEvent here  to avoid coalescing?

I'm not sure, technically coalescence should be correct thing here but it depends on how AT use it.

Comment 4

[alexander :surkov (:asurkov)]

Jamie, we fire reorder event on iframe when it gets a document accessible. If that reorder event is coalesced by reorder event on container, would it be a problem for you?

Comment 5

[alexander :surkov (:asurkov)]

Jamie, we fire reorder event on iframe when it gets a document accessible. If that reorder event is coalesced by reorder event on container, would it be a problem for you?

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 7

[Trevor Saunders (:tbsaunde)]

> > >+  Accessible* GetAccessible() const { return mAccessible; }
> > 
> > Accessible() ?
> 
> there's something wrong with that approach since Name as a type and Name()
> as a function are in conflict and we need to use namesapce to resolve it
> (see nsAccessNode::RootAccessible for example). So I left that unchanged.

ok :(

> > >+  DocAccessible* GetDocAccessible() const
> > >+    { return mAccessible ? mAccessible->Document() : nullptr; }
> > 
> > DocAccessible()? how can mAccessible be null?
> 
> It shouldn't be, but it could be in future I think (though I'm not certain
> yet how better to introduce "no target" events). I'll remove it for now.

well, this patch relies on mAccessible not being null in a couple other places too so I don't see it hurting anything.

btw what is purpose of events without a target?

> > >+  AccStateChangeEvent(Accessible* aAccessible, uint64_t aState) :
> > >+    AccEvent(::nsIAccessibleEvent::EVENT_STATE_CHANGE, aAccessible,
> > >+             eAutoDetect, eAllowDupes), mState(aState)
> > >+    { mIsEnabled = (mAccessible->State() & mState) != 0; }
> > 
> > it seems silly to call State() when we could figure out if the state is
> > enabled much faster where the event is created.
> 
> yes, that bug is in my todo list of not filed bugs :)

ok

> > why don't we do this at the point where we create caret move events, and
> > then coalesce away the out of date ones?
> 
> it's faster, it doesn't make sense to compute the caret for coalesced events
> (I guess we can get many events when user moves through the text).

actually, it should mean we never need to compute caret offset since we already computed it for mLastCaretOffset just before we fire the event.

> > >     const uint32_t kState = (aAttribute == nsGkAtoms::aria_checked) ?
> > >                             states::CHECKED : states::PRESSED;
> > 
> > shouldn't that be uint64_t ?
> 
> you don't know the answer, right? ;)

well, I do, I just phrased the comment as a question for some reason

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > > >+  Accessible* GetAccessible() const { return mAccessible; }
> > > 
> > > Accessible() ?
> > 
> > there's something wrong with that approach since Name as a type and Name()
> > as a function are in conflict and we need to use namesapce to resolve it
> > (see nsAccessNode::RootAccessible for example). So I left that unchanged.
> 
> ok :(

I filed bug 812963 for this.

> > > DocAccessible()? how can mAccessible be null?
> > 
> > It shouldn't be, but it could be in future I think (though I'm not certain
> > yet how better to introduce "no target" events). I'll remove it for now.
> 
> well, this patch relies on mAccessible not being null in a couple other
> places too so I don't see it hurting anything.

I agree, that was just thinking aloud.

> btw what is purpose of events without a target?

no target for delayed events, for example, it makes sense for anchor jump events to make sure event is delivered fine even if document mutates.

> > > it seems silly to call State() when we could figure out if the state is
> > > enabled much faster where the event is created.
> > 
> > yes, that bug is in my todo list of not filed bugs :)

bug 812964 filed

> 
> > > why don't we do this at the point where we create caret move events, and
> > > then coalesce away the out of date ones?
> > 
> > it's faster, it doesn't make sense to compute the caret for coalesced events
> > (I guess we can get many events when user moves through the text).
> 
> actually, it should mean we never need to compute caret offset since we
> already computed it for mLastCaretOffset just before we fire the event.

good point, I'll file a bug?

> > > >     const uint32_t kState = (aAttribute == nsGkAtoms::aria_checked) ?
> > > >                             states::CHECKED : states::PRESSED;
> > > 
> > > shouldn't that be uint64_t ?
> > 
> > you don't know the answer, right? ;)
> 
> well, I do, I just phrased the comment as a question for some reason

I know, just sometimes it's funny you do that :)

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > > > >+  Accessible* GetAccessible() const { return mAccessible; }
> > > > 
> > > > Accessible() ?
> > > 
> > > there's something wrong with that approach since Name as a type and Name()
> > > as a function are in conflict and we need to use namesapce to resolve it
> > > (see nsAccessNode::RootAccessible for example). So I left that unchanged.
> > 
> > ok :(
> 
> I filed bug 812963 for this.
> 
> > > > DocAccessible()? how can mAccessible be null?
> > > 
> > > It shouldn't be, but it could be in future I think (though I'm not certain
> > > yet how better to introduce "no target" events). I'll remove it for now.
> > 
> > well, this patch relies on mAccessible not being null in a couple other
> > places too so I don't see it hurting anything.
> 
> I agree, that was just thinking aloud.
> 
> > btw what is purpose of events without a target?
> 
> no target for delayed events, for example, it makes sense for anchor jump
> events to make sure event is delivered fine even if document mutates.
> 
> > > > it seems silly to call State() when we could figure out if the state is
> > > > enabled much faster where the event is created.
> > > 
> > > yes, that bug is in my todo list of not filed bugs :)
> 
> bug 812964 filed
> 
> > 
> > > > why don't we do this at the point where we create caret move events, and
> > > > then coalesce away the out of date ones?
> > > 
> > > it's faster, it doesn't make sense to compute the caret for coalesced events
> > > (I guess we can get many events when user moves through the text).
> > 
> > actually, it should mean we never need to compute caret offset since we
> > already computed it for mLastCaretOffset just before we fire the event.
> 
> good point, I'll file a bug?

Comment 10

[Trevor Saunders (:tbsaunde)]

err, hit enter too soon

> > btw what is purpose of events without a target?
> 
> no target for delayed events, for example, it makes sense for anchor jump
> events to make sure event is delivered fine even if document mutates.

hm, who will event be delivered to?

> > > > why don't we do this at the point where we create caret move events, and
> > > > then coalesce away the out of date ones?
> > > 
> > > it's faster, it doesn't make sense to compute the caret for coalesced events
> > > (I guess we can get many events when user moves through the text).
> > 
> > actually, it should mean we never need to compute caret offset since we
> > already computed it for mLastCaretOffset just before we fire the event.
> 
> good point, I'll file a bug?

sure

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> err, hit enter too soon

I thought no words :)

> > > btw what is purpose of events without a target?
> > 
> > no target for delayed events, for example, it makes sense for anchor jump
> > events to make sure event is delivered fine even if document mutates.
> 
> hm, who will event be delivered to?

I meant we need to fire it to anchor target or to closest accessible if anchor target has gone.

> > > actually, it should mean we never need to compute caret offset since we
> > > already computed it for mLastCaretOffset just before we fire the event.
> > 
> > good point, I'll file a bug?
> 
> sure

bug 812967

Comment 12

[Trevor Saunders (:tbsaunde)]

Comment on attachment 682981 [details] [diff] [review]
patch2

> AccEvent::CaptureIsFromUserInput(EIsFromUserInput aIsFromUserInput)
> {
>+  DocAccessible* document = mAccessible->Document();
>+  if (!document) {
>+    NS_ASSERTION(mAccessible == ApplicationAcc(),
>+                 "There should always be a DOM node for an event!");

that's a kind of funny message given the assertion, maybe accesisble other than application accessible should always have a doc?

>+  mIsFromUserInput =
>+    document->PresContext()->EventStateManager()->IsHandlingUserInputExternal();

it seems this is all silly and you could just do
nsEventStateManager::IsHandlingUserInput()

>+  DocAccessible* GetDocAccessible() const
>+    { return mAccessible->Document() : nullptr; }

that seems funny, I wonder if it compiles?

> protected:
>+  nsCOMPtr<nsINode> mNode;

file a bug to take care of mutation events too?

>+  nsIContent* elm = aAccessible->GetContent();
>+  if (!elm->HasAttr(kNameSpaceID_None, nsGkAtoms::role)) {

maybe file bug check accessible->mRoleMapEntry?

>+    nsIContent* elm = aAccessible->GetContent();

don't you already have that?

>+  /**
>+   * Fire saccessible events when attribute is changed.

sasccessible?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> Comment on attachment 682981 [details] [diff] [review]
> patch2
> 
> > AccEvent::CaptureIsFromUserInput(EIsFromUserInput aIsFromUserInput)
> > {
> >+  DocAccessible* document = mAccessible->Document();
> >+  if (!document) {
> >+    NS_ASSERTION(mAccessible == ApplicationAcc(),
> >+                 "There should always be a DOM node for an event!");
> 
> that's a kind of funny message given the assertion, maybe accesisble other
> than application accessible should always have a doc?

fine with me

> >+  mIsFromUserInput =
> >+    document->PresContext()->EventStateManager()->IsHandlingUserInputExternal();
> 
> it seems this is all silly and you could just do
> nsEventStateManager::IsHandlingUserInput()

as follow up, since it seems we need to remove that method at all

> >+  DocAccessible* GetDocAccessible() const
> >+    { return mAccessible->Document() : nullptr; }
> 
> that seems funny, I wonder if it compiles?

perhaps I didn't check that :)

> > protected:
> >+  nsCOMPtr<nsINode> mNode;
> 
> file a bug to take care of mutation events too?

perhaps that makes sense, I'll take a look.

> >+  nsIContent* elm = aAccessible->GetContent();
> >+  if (!elm->HasAttr(kNameSpaceID_None, nsGkAtoms::role)) {
> 
> maybe file bug check accessible->mRoleMapEntry?

Not sure since it changes behavior, mRoleMapEntry is null for unknown roles.

> >+    nsIContent* elm = aAccessible->GetContent();
> 
> don't you already have that?

ok, good catch

Comment 14

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/50a2b272be96

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/50a2b272be96

Comment 16

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #4)
> Jamie, we fire reorder event on iframe when it gets a document accessible.
> If that reorder event is coalesced by reorder event on container, would it
> be a problem for you?
I assume you mean if, for example, an iframe was added to the parent document, reorder would be fired on the document? This should be fine.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #16)
> (In reply to alexander :surkov from comment #4)
> > Jamie, we fire reorder event on iframe when it gets a document accessible.
> > If that reorder event is coalesced by reorder event on container, would it
> > be a problem for you?
> I assume you mean if, for example, an iframe was added to the parent
> document, reorder would be fired on the document? This should be fine.

Currently it might be not fired (coalesced by reorder event on iframe container). Is that a problem?

Comment 18

[James Teh [:Jamie]]

I meant if an iframe gets added to a top level document, surely a reorder would be fired on the top level document?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #18)
> I meant if an iframe gets added to a top level document, surely a reorder
> would be fired on the top level document?

reorder event is fired on container. If you do

<body><div id="div"></div></body>

var iframe = document.createElement("iframe");
iframe.src = "www.mozilla.me";
div.appendChild(iframe); // reorder event is fired on div

var iframe = document.createElement("iframe");
iframe.src = "www.mozilla.you";
div.appendChild(iframe); // reorder event is fired on document

besides these reorder events you can get reorder event on iframe accessible (but it might be coalesced with containing reorder events)

Comment 20

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #19)

> var iframe = document.createElement("iframe");
> iframe.src = "www.mozilla.me";
> div.appendChild(iframe); // reorder event is fired on div
> 
> var iframe = document.createElement("iframe");
> iframe.src = "www.mozilla.you";
> div.appendChild(iframe); // reorder event is fired on document

document.body.appendChild (copy/paste issue)

Comment 21

[James Teh [:Jamie]]

That's all fine.

Comment 22

[alexander :surkov (:asurkov)]

ok, thank you