437640 - ability to remove <keyset>s

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Attachment: support this

1. allow keysets to be removed and have their listener removed
2. support a disabled attribute to disable keysets

Couldn't think of a better way to retrieve the listener again so I store it as a property of the keyset.

Comment 1

[[e-mail]]

Comment on attachment 324059 [details] [diff] [review]
support this

For some reason I'm not seeing the event listeners get removed corretly - the keydown listener is OK but the other two aren't.

Comment 2

[Neil Deakin]

(In reply to comment #1)
> (From update of attachment 324059 [details] [diff] [review])
> For some reason I'm not seeing the event listeners get removed corretly - the
> keydown listener is OK but the other two aren't.

I don't see this problem. How are you checking this?

Comment 3

[[e-mail]]

(In reply to comment #2)
> (In reply to comment #1)
> > (From update of attachment 324059 [details] [diff] [review] [details])
> > For some reason I'm not seeing the event listeners get removed corretly - the
> > keydown listener is OK but the other two aren't.
> I don't see this problem. How are you checking this?
1. Open DOMi and use it to inspect the browser
2. Pick a keyset and remove it from the DOM
3. Switch back to the browser and press a key in that keyset

Actual results: assertion "element must be in document"

Comment 4

[[e-mail]]

I also breakpointed in nsEventListenerManager::RemoveEventListener mListeners.RemoveElementAt(i); and it was only hit once.

Comment 5

[Neil Deakin]

Hmmm. It seems that the same keyset is being added multiple times, thus there are extra listeners.
 

Comment 6

[Neil Deakin]

Attachment: don't add a listener if one already exists

Minor change to just not add a listener if one already exists.

The bug of adding it multiple times is probably something in the document/overlay loading code and should be a different bug.

Comment 7

[[e-mail]]

Comment on attachment 324289 [details] [diff] [review]
don't add a listener if one already exists

>+//
>+// DetachGlobalKeyHandler
>+//
>+// Removes a key handler added by DeatchGlobalKeyHandler.
>+//
Oops ;-)

Comment 8

[[e-mail]]

(In reply to comment #4)
> I also breakpointed in nsEventListenerManager::RemoveEventListener
> mListeners.RemoveElementAt(i); and it was only hit once.
Smaug pointed out that because the listener implements nsIDOMKeyListener it only gets added/removed once.

Comment 9

[Neil Deakin]

Attachment: to be checked in

minor issues with test:
 - disable accesskey test, as it seems eventutils.js doesn't handle it
 - make sure to close the test window when finished

Comment 10

[Neil Deakin]

Attachment: needed to update a uuid as nsIXBLService is changed. I think this should be ok for 1.9.1. Includes the two changes to the test as well.

Comment 11

[Daniel Holbert [:dholbert]]

Reopening -- this was backed out, because it caused some tinderboxen to go orange due to leaks during mochitests.

Sample tinderbox log, with the offending leaks marked "ERROR FAIL":
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla2/1213882948.1213885920.16571.gz&fulltext=1

The backout was changeset 26033045ba14.

Comment 12

[Neil Deakin]

Attachment: fix the leak by readding the NS_RELEASE I had removed

Comment 13

[[e-mail]]

Comment on attachment 328966 [details] [diff] [review]
fix the leak by readding the NS_RELEASE I had removed

>-  // Release.  Do this so that only the event receiver holds onto the key handler.
>+  // the reference added in NS_NewXBLWindowKeyHandler will be taken by the property
>+  if (contentNode)
>+    return contentNode->SetProperty(nsGkAtoms::listener, handler,
>+                                    nsPropertyTable::SupportsDtorFunc, PR_TRUE);
>   NS_RELEASE(handler);
>+  return NS_OK;
>+}
You should fix up the comments to make it clearer who owns what depending on whether or not you have a content node.