569653 - Investigate and fix unsafe selection changes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

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

Spin off from bug 423737.

Comment 1

[Damon Sicore (:damons)]

davidb, who owns this?

Comment 2

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

(In reply to comment #1)
> davidb, who owns this?

Sorry. Me. I have a plan - patch expected by end of Monday.

Comment 3

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

Attachment: WIP

Might be this simple.

Comment 4

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

Attachment: WIP

Comment 5

[alexander :surkov (:asurkov)]

nit: double space: nsDocAccessible  *docAccessible = 

you don't want them to coalesce?

mochitest please

Comment 6

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

(In reply to comment #5)
> nit: double space: nsDocAccessible  *docAccessible = 
> 
> you don't want them to coalesce?
> 

No I want to preserve behaviour, at least for now.

> mochitest please

Yep.

Comment 7

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

Comment on attachment 450564 [details] [diff] [review]
WIP

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-a537fbf987aa/

Comment 8

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

Attachment: test

Sorry for delay. Here's a test.

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 451056 [details] [diff] [review]
test

r=me, thanks!

Comment 10

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

Comment on attachment 450564 [details] [diff] [review]
WIP

(In reply to comment #5)
> nit: double space: nsDocAccessible  *docAccessible = 

fixed locally.

> you don't want them to coalesce?

No (as per IRC)

> mochitest please

Added (see other attached diff). We can do more of course, but let's do that on a follow up (Bug 571900)?

Comment 11

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

Comment on attachment 451056 [details] [diff] [review]
test

>+    function addSelection(aNode, aOption)
>+    {

>+      this.getID = function addselection_getID() {
>+        return prettyName(this.DOMNode) + " added to selection";

Note to self: change this to this.optionNode

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 450564 [details] [diff] [review]
WIP


>+  nsDocAccessible  *docAccessible = 

nit: two spaces

>+    nsAccUtils::GetDocAccessibleFor(optionNode);

use GetDocAccessible();

>+  if (!docAccessible)
>+    return;

we don't need a check while IsDefuct() is called 

>+
>+  nsRefPtr<nsAccEvent> eventSelectionWithin =
>+    new nsAccEvent(nsIAccessibleEvent::EVENT_SELECTION_WITHIN, multiSelect);

selectionWithinEvent or selWithinEvent?

>+  if (eventSelectionWithin)

fail if no memory

>-  nsEventShell::FireEvent(eventType, optionAccessible);
>+  nsRefPtr<nsAccEvent> eventAddRemove =

addRemoveSelectionEvent or addRemoveSelEvent?

Comment 13

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

(In reply to comment #12)
> (From update of attachment 450564 [details] [diff] [review])

OK everything updated except:

> >+  nsRefPtr<nsAccEvent> eventSelectionWithin =
> 
> selectionWithinEvent or selWithinEvent?
> 
> >+  nsRefPtr<nsAccEvent> eventAddRemove =
> 
> addRemoveSelectionEvent or addRemoveSelEvent?

I think it is clearer to leave these. Otherwise the meaning is confusing.

Comment 14

[alexander :surkov (:asurkov)]

we just used reorderEvent or refreshEvent for example, should be they converted to eventReorder and etc?

btw, eventAddRemove doesn't say it's about selection. If it is then eventWithin should be enough as well.

Comment 15

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

OK I made the selFooEvent.

Comment 16

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

Test landed: http://hg.mozilla.org/mozilla-central/rev/cfdfc56b8e73
Fix landed: http://hg.mozilla.org/mozilla-central/rev/d2c022e9417f