561525 - Fire EVENT_SCROLLING_START asyncronously

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

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

Attachment: patch 1 (firedelayed)

There is not reason to fire this event immediately and firing it async is safer.

Comment 1

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

I'm a bit sleepy, so careful review highly recommended ;)

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 441242 [details] [diff] [review]
patch 1 (firedelayed)

I would do

>+  nsIDocument *document = aTarget->GetCurrentDoc();
>+  nsCOMPtr<nsIDOMNode> documentNode(do_QueryInterface(document));
>+  if (documentNode) {

if (!documentNode)
  return;

>+    nsRefPtr<nsDocAccessible> accessibleDoc =
>+      nsAccessNode::GetDocAccessibleFor(documentNode);
>+    if (accessibleDoc) {

if (!accessibleDoc)
  return;

>+      // If current target is not accessible, find nearest accessible parent
>+      if (!targetAcc) {
>+            accessibleDoc->GetAccessibleInParentChain(targetNode, PR_TRUE,
>+                                                      getter_AddRefs(targetAcc));
>+            nsCOMPtr<nsIAccessNode> accNode = do_QueryInterface(targetAcc);
>+            accNode->GetDOMNode(getter_AddRefs(targetNode));
>+      }

then it would be unchanged in history

>+      accessibleDoc->FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_SCROLLING_START,
>+                            targetNode);
>     }

btw, this line is not equivalent to 

if (targetNode)
  FireEvent()

btw, it's funny we get an accessible to get a DOM node to fire delayed event in order to get an accessible from DOM node to fire an event. Would we like to move the accessible search into nsDocAccessible::ProcessPendingEvent? So that if node goes away then we'll be able to fire event still and we don't need dances with node-accessible getting.

Comment 3

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

(In reply to comment #2)
> btw, it's funny we get an accessible to get a DOM node to fire delayed event in
> order to get an accessible from DOM node to fire an event. Would we like to
> move the accessible search into nsDocAccessible::ProcessPendingEvent? So that
> if node goes away then we'll be able to fire event still and we don't need
> dances with node-accessible getting.

Yes, good idea.

Comment 4

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

Hmm actually that didn't looks so good. We'd end up possibly creating two events (or adding a SetAccessible to nsAccEvent).

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> Hmm actually that didn't looks so good. We'd end up possibly creating two
> events (or adding a SetAccessible to nsAccEvent).

The problems of delayed event is node can become inaccessible and even it can be removed from DOM. The same time we don't want to loose scroll event. So no one approach sounds good. Since this is rare case I think then you could go with your original approach, however please add XXX comment.

Comment 6

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

Attachment: patch 2 (create event based on accessible)

This patch overloads FireDelayedAccessibleEvent to accept an nsIAccessible, which might be useful elsewhere I'm not sure. It seems to tidy up patch1 nicely.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Created an attachment (id=441381) [details]
> patch 2 (create event based on accessible)
> 
> This patch overloads FireDelayedAccessibleEvent to accept an nsIAccessible,
> which might be useful elsewhere I'm not sure. It seems to tidy up patch1
> nicely.

I'm skeptic on that score. We always deal with DOM nodes for delayed events and in some cases it's unique right way to go. This method grants us an ability to choose between these methods what I don't find appeal. On the other hand our code is built upon an assumption the delayed event is initialized by DOM node always so event coalescing doesn't work until you do this.

Comment 8

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

I thought you might say that :)

OK back to patch 1, I'll fix the nits and repost (later today or tomorrow)

Comment 9

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

Attachment: reworked patch 1

Comment 10

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

Alexander, note I changed the comment locally to:
  // XXX note in rare cases the node could go away before we flush the queue,
  // for example if the node becomes inaccessible, or is removed from the DOM.

as per chat.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 441417 [details] [diff] [review]
reworked patch 1


>+  NS_ENSURE_TRUE(targetNode, NS_ERROR_FAILURE);

I don't think it should fail ever, I would prefer an assertion and 
if (!tagetNode)
  return NS_OK;

> 
>-  return NS_OK;
>+  // XXX note in rare cases the node could go away before we flush the queue.

please describe rare cases.

Comment 12

[alexander :surkov (:asurkov)]

David, once bug 561541 is fixed then would you mind to convert nsresult to void for NotifyOfAnchorJumpTo?

Comment 13

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

Sure.

Comment 14

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

(In reply to comment #12)
> David, once bug 561541 is fixed then would you mind to convert nsresult to void
> for NotifyOfAnchorJumpTo?

Done. I'll land sometime in the next few days.

Comment 15

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

Landed on central: http://hg.mozilla.org/mozilla-central/rev/28432be5b336

I'll leave it to security to nominate for back-porting (or not).