Fire EVENT_SCROLLING_START asyncronously

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
major
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: davidb, Assigned: davidb)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][ccbr][critsmash:resolved])

Attachments

(1 attachment, 2 obsolete attachments)

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

There is not reason to fire this event immediately and firing it async is safer.
Attachment #441242 - Flags: review?
Attachment #441242 - Flags: review? → review?(surkov.alexander)
I'm a bit sleepy, so careful review highly recommended ;)

Comment 2

8 years ago
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.
Attachment #441242 - Flags: review?(surkov.alexander)
(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.
Hmm actually that didn't looks so good. We'd end up possibly creating two events (or adding a SetAccessible to nsAccEvent).

Comment 5

8 years ago
(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.
Created attachment 441381 [details] [diff] [review]
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.
Attachment #441242 - Attachment is obsolete: true
Attachment #441381 - Flags: review?(surkov.alexander)

Updated

8 years ago
Whiteboard: [sg:critical?][ccbr]

Comment 7

8 years ago
(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.
I thought you might say that :)

OK back to patch 1, I'll fix the nits and repost (later today or tomorrow)
Attachment #441381 - Flags: review?(surkov.alexander)
Created attachment 441417 [details] [diff] [review]
reworked patch 1
Attachment #441381 - Attachment is obsolete: true
Attachment #441417 - Flags: review?(surkov.alexander)
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

8 years ago
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.
Attachment #441417 - Flags: review?(surkov.alexander) → review+

Comment 12

8 years ago
David, once bug 561541 is fixed then would you mind to convert nsresult to void for NotifyOfAnchorJumpTo?
(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.

Updated

8 years ago
Whiteboard: [sg:critical?][ccbr] → [sg:critical?][ccbr][critsmash:patch]
Landed on central: http://hg.mozilla.org/mozilla-central/rev/28432be5b336

I'll leave it to security to nominate for back-porting (or not).
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Whiteboard: [sg:critical?][ccbr][critsmash:patch] → [sg:critical?][ccbr][critsmash:resolved]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.