586961 - Access key on label clicks label, not controlled element

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. Open Bookmarks Organiser
2. Select a bookmark
3. Press Alt+E to attempt to expand the info pane

Expected result: info pane expands

Actual result: expander focused

PerformAccessKey is calling ClickWithInputSource on the wrong element.

Comment 1

[[e-mail]]

Although even if I hack PerformAccessKey to call ClickWithInputSource on the correct element, it only works the first time for some reason...

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Felipe, could you look at this.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Yes, patch coming.

I've debugged the "only works the first time" problem, and it's unrelated and happens on 3.6 as well. The problem is that the More/Less control changes the accesskey when changing the text. After a sequence of register/unregister accesskey and frame construction/destruction, somehow the label ends up with the accesskey registered twice, and then this loop fails: http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp?mark=1466,1471-1476#1466 because it finds the accesskey but finds another one on the inner _while_, and disables activation.
This should probably be for another bug, right?

Comment 4

[[e-mail]]

(In reply to comment #3)
> I've debugged the "only works the first time" problem, and it's unrelated and
> happens on 3.6 as well.

> This should probably be for another bug, right?

Yes, this bug only covers any regressions from bug 547996.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Neil, are you the right person to review this?

Comment 6

[[e-mail]]

Comment on attachment 465940 [details] [diff] [review]
Patch

>+          nsXULElement* xulElm = FromContent(content);
Since elm is already an nsIDOMXULElement, is it worth making elm itself the nsXULElement* elm = FromContent(content) instead? (Note: this will cause issues with do_QueryInterface. Either remove it or substitute content for elm.)

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch v2

> >+          nsXULElement* xulElm = FromContent(content);
> Since elm is already an nsIDOMXULElement, is it worth making elm itself the
> nsXULElement* elm = FromContent(content) instead? (Note: this will cause issues
> with do_QueryInterface. Either remove it or substitute content for elm.)

Thanks, that's much better.

AIUI _elm_ doesn't need to be a nsCOMPtr since _content_ already is and lives during the entire function. Is that right?

Comment 8

[[e-mail]]

(In reply to comment #7)
> AIUI _elm_ doesn't need to be a nsCOMPtr since _content_ already is and lives
> during the entire function. Is that right?
elm was only an nsCOMPtr before because do_QueryInterface needed it.

Comment 9

[[e-mail]]

(In reply to comment #1)
> Although even if I hack PerformAccessKey to call ClickWithInputSource on the
> correct element, it only works the first time for some reason...
I guess I still need to file a separate bug on this...

Comment 10

[[e-mail]]

I filed bug 589328 on the access key only working the first time issue.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 467826 [details] [diff] [review]
Patch v2

Requesting approval2.0.  Simple patch with tests to fix a regression on accesskeys.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/mozilla-central/rev/18279b0235f7