514595 - it's not necessary to keep two events type for show and two ones for hide

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

1. It's not correct, JS ATs don't need to know if this event is from DOM and this is from layout. They need to know accessible was shown or hidden.

2. It looks like we don't need two events internally, because all related logic can be hidden inside of accessible event.

3. Provide new constants for layout/dom modules to leave them unchanged.

It's not just clean up bug, this bug will make coalescence of events coming from DOM and layout.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch2

fixed linux bustage

Comment 3

[Marco Zehe (:MarcoZ)]

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

>+      // Because of async layout changes we handle remove child node change
>+      // before than hide parent node change even we hide parent before we
>+      // remove a child. Therefore mark this as todo until we have more smart
>+      // events coalescence.

The way I see it, all the kTodo usages point to something that was "true" even before this, so I don't see any difference to what we had before. Is this change really necessary?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)

> The way I see it, all the kTodo usages point to something that was "true" even
> before this, so I don't see any difference to what we had before. Is this
> change really necessary?

You're right. But the difference is code readability. When I see two boolean one after other then I always think which of them this one is. Also it would be easy to find what we need to do by simple search.

Comment 5

[Marco Zehe (:MarcoZ)]

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

r=me for the test part, thanks for the explanation!

Comment 6

[Ginn Chen]

 void
 nsDocAccessible::ContentRemoved(nsIDocument *aDocument, nsIContent* aContainer,
                                 nsIContent* aChild, PRInt32 aIndexInContainer)
 {
   // Invalidate the subtree of the removed element.
   // InvalidateCacheSubtree(aChild, nsIAccessibleEvent::EVENT_DOM_DESTROY);

Please also update the comments.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch3

Ginn's comment is addressed

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 399049 [details] [diff] [review]
patch3

+  PRBool mIsAsync;

PRPackedBool

+    PRBool isAsynch = nsAccEvent::IsAsyncEvent(accessibleEvent);
+    if (domNode == gLastFocusedNode && isAsynch &&

isAsync

Comment 9

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

Comment on attachment 399049 [details] [diff] [review]
patch3

OK thanks, I can see how this will help coalescence, r=me. We should rename our event constants in events.js etc. (here or a follow up).

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> (From update of attachment 399049 [details] [diff] [review])
> OK thanks, I can see how this will help coalescence, r=me. We should rename our
> event constants in events.js etc. (here or a follow up).

Did I miss something in this patch? At least I renamed them in events.js (mochitests are passed).

Comment 11

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

(In reply to comment #9)
> (From update of attachment 399049 [details] [diff] [review])
> OK thanks, I can see how this will help coalescence, r=me. We should rename our
> event constants in events.js etc. (here or a follow up).

Ah ok - my bad. Thanks.

Comment 12

[alexander :surkov (:asurkov)]

landed on 1.9.3 with addressed Robert's comment - http://hg.mozilla.org/mozilla-central/rev/90cef4e7d833