515141 - move events firing logic from nsDocAccessible and nsAccEvent into special classes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

In the meantime delayed events handling is spread between nsDocAccessible and nsAccEvent classes. It would be nice to have nsEventUtils and nsEventQueue classes which will have all necessary logic to fire events. nsDocAccessible would create an instance of nsEventQueue class to deal with delayed events. nsEventUtills would keep common logic for normal and delayed events firing, for example, it could have FireShowHideEvents() method.

Comment 1

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

Agreed. This logic should sit in one place (class or otherwise). Are you working on this?

Comment 2

[alexander :surkov (:asurkov)]

I plan to start working on this once I'll land patch of bug 514595.

Comment 3

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

Great! Can you post a WIP for discussion, or catch me on IRC? We might not want multiple classes for this. I just want to check with you about the design before you go too far down (with an enormous patch) :)

Comment 4

[alexander :surkov (:asurkov)]

Attachment: not well formed idea

1) probably it's nice to have to files (for nsEventQueue and nsEventUtils)
2) gLastFocusFrameType should live inside of event utils probably if so nsRootAcc should have access to it
3) think about PrepareEvents while you are here
4) probably it's worth to move FireTextEvent/FireShowHide/FireFocus into nsEventUtils
5) should nsDocAcc owns by nsEventQueue or it's preferable (more comfortable) to have its cache independently.

Comment 5

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

Thanks, as per our chat I might take this on so you can work on the table bugs. This will be a good bug to fix first, before our event perf bugs.

Comment 6

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

Wow I spent a lot of time tonight trying take this patch to a clean compile; maybe I was taking it in the wrong direction, not sure.

I'm more comfortable with making smaller changes, compiling as often as I can stand it... I think there is a lot of unfinished business in this patch :)

Hmmm, do you want to continue it, or should I start over?

Comment 7

[alexander :surkov (:asurkov)]

part 1: bug 540262 "get rid nsAccEvent static helpers"

Comment 8

[alexander :surkov (:asurkov)]

part 2: bug 481396 -  rename nsAccessibleEventData file to nsAccEvent

Comment 9

[alexander :surkov (:asurkov)]

part 3:  Bug 540265 -  move nsAccUtils::FireEvent into special class

Comment 10

[alexander :surkov (:asurkov)]

part 4:  Bug 540272 -  don't use nsAccessible::FireAccessibleEvent directly

Comment 11

[alexander :surkov (:asurkov)]

part 5:  Bug 540281 -  rename nsAccessible::FireAccessibleEvent into HandleAccEvent

Comment 12

[alexander :surkov (:asurkov)]

part 6:  Bug 540285 -  stop explicit usage of nsAccEvent::prepareEvent

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 14

[alexander :surkov (:asurkov)]

this patch is built upon the patch of bug 523785.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch2

fix queue processing logic

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> Created an attachment (id=422985) [details]
> patch2
> 
> fix queue processing logic

oops, sorry, missed return.

Comment 17

[alexander :surkov (:asurkov)]

What do you think about inline Get...() method (like GetEventType() and etc) usage instead of making nsAccEventQueue a friend of nsAccEvent? Does it sound reasonable?

Comment 18

[alexander :surkov (:asurkov)]

try server build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-03403abcc7e6

Comment 19

[[e-mail]]

(In reply to comment #17)
> What do you think about inline Get...() method (like GetEventType() and etc)
> usage instead of making nsAccEventQueue a friend of nsAccEvent? Does it sound
> reasonable?
nsAccEventQueue uses those members a lot, so no, I think friend is better.

Comment 20

[[e-mail]]

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

I didn't see any interesting changes...

Comment 21

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

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

r=me, minor nit:

>+++ b/accessible/src/base/nsAccessNode.h
>@@ -163,19 +163,30 @@ class nsAccessNode: public nsIAccessNode
>      */
>     virtual nsresult Shutdown();
> 
>     /**
>      * Return frame for the given access node object.
>      */
>     virtual nsIFrame* GetFrame();
> 
>+  /**
>+   * Return the presentation shell the accessible belongs to.
>+   */
>+  already_AddRefed<nsIPresShell> GetPresShell();

I'd rather the comment didn't use "belongs" since we aren't a member of the pres shell. How about "Return the corresponding press shell for this accessible".

>+
>+  /**
>+   * Return true if the accessible still has presentation shell. Light-weight
>+   * version of IsDefunct() method.
>+   */
>+  PRBool HasWeakShell() const { return !!mWeakShell; }

Good, fast ;)

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)

> >+  /**
> >+   * Return the presentation shell the accessible belongs to.
> >+   */
> >+  already_AddRefed<nsIPresShell> GetPresShell();
> 
> I'd rather the comment didn't use "belongs" since we aren't a member of the
> pres shell. How about "Return the corresponding press shell for this
> accessible".

Done. As well I changed the uuid of nsAccessNode.

Comment 23

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/0f180f44f0b4