If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

ability to remove <keyset>s

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Neil Deakin, Assigned: Neil Deakin)

Tracking

({dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 324059 [details] [diff] [review]
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.
Attachment #324059 - Flags: superreview?(neil)
Attachment #324059 - Flags: review?(neil)

Comment 1

9 years ago
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.
(Assignee)

Comment 2

9 years ago
(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

9 years ago
(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

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

Comment 5

9 years ago
Hmmm. It seems that the same keyset is being added multiple times, thus there are extra listeners.
 
(Assignee)

Comment 6

9 years ago
Created attachment 324289 [details] [diff] [review]
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.
Attachment #324059 - Attachment is obsolete: true
Attachment #324289 - Flags: superreview?(neil)
Attachment #324289 - Flags: review?(neil)
Attachment #324059 - Flags: superreview?(neil)
Attachment #324059 - Flags: review?(neil)

Comment 7

9 years ago
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 ;-)
Attachment #324289 - Flags: superreview?(neil)
Attachment #324289 - Flags: superreview+
Attachment #324289 - Flags: review?(neil)
Attachment #324289 - Flags: review+

Comment 8

9 years ago
(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.
(Assignee)

Comment 9

9 years ago
Created attachment 325257 [details] [diff] [review]
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
(Assignee)

Comment 10

9 years ago
Created attachment 325537 [details] [diff] [review]
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.
Attachment #324289 - Attachment is obsolete: true
Attachment #325257 - Attachment is obsolete: true
Attachment #325537 - Flags: superreview?(neil)
Attachment #325537 - Flags: review?(neil)

Updated

9 years ago
Attachment #325537 - Flags: superreview?(neil)
Attachment #325537 - Flags: superreview+
Attachment #325537 - Flags: review?(neil)
Attachment #325537 - Flags: review+
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

9 years ago
Created attachment 328966 [details] [diff] [review]
fix the leak by readding the NS_RELEASE I had removed
Attachment #325537 - Attachment is obsolete: true
Attachment #328966 - Flags: superreview?(neil)
Attachment #328966 - Flags: review?(neil)
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.
Attachment #328966 - Flags: superreview?(neil)
Attachment #328966 - Flags: superreview+
Attachment #328966 - Flags: review?(neil)
Attachment #328966 - Flags: review+

Updated

9 years ago
Blocks: 441794
(Assignee)

Updated

9 years ago
Keywords: checkin-needed

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
(Assignee)

Updated

9 years ago
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Updated

9 years ago
Flags: in-testsuite+

Updated

9 years ago
Keywords: dev-doc-needed
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.