1253284 - Cmd+Q isn't reserved on OS X

Status: RESOLVED FIXED

(Firefox :: Keyboard Navigation, defect, P4)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1203059#c52

TL;DR we're reserving certain keybindings for the browser only and need some Firefox code changed to fully support this.

Comment 1

[Makoto Kato [:m_kato]]

<key id="key_quitApplication" key="&quitApplicationCmdUnix.key;" modifiers="accel"/> has no command attribute, so even if adding reserved="true", it doesn't check this attribute.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Makoto Kato [:m_kato] from comment #1)
> <key id="key_quitApplication" key="&quitApplicationCmdUnix.key;"
> modifiers="accel"/> has no command attribute, so even if adding
> reserved="true", it doesn't check this attribute.

Is that really not checking?

nsXBLWindowKeyHandler checks the reserved event here:
https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/dom/xbl/nsXBLWindowKeyHandler.cpp#604,607-608,613,618-620

But GetElementForHandler() looks like returning key element if there is no proper command element:
https://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLWindowKeyHandler.cpp#717

Comment 3

[Makoto Kato [:m_kato]]

Attachment: MozReview Request: Bug 1253284 - Allow reserved attribute without command attribute

Review commit: https://reviewboard.mozilla.org/r/51053/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51053/

Comment 4

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 4/29-5/8) from comment #2)
> (In reply to Makoto Kato [:m_kato] from comment #1)
> > <key id="key_quitApplication" key="&quitApplicationCmdUnix.key;"
> > modifiers="accel"/> has no command attribute, so even if adding
> > reserved="true", it doesn't check this attribute.
> 
> Is that really not checking?
> 
> nsXBLWindowKeyHandler checks the reserved event here:
> https://dxr.mozilla.org/mozilla-central/rev/
> fc15477ce628599519cb0055f52cc195d640dc94/dom/xbl/nsXBLWindowKeyHandler.
> cpp#604,607-608,613,618-620
> 
> But GetElementForHandler() looks like returning key element if there is no
> proper command element:
> https://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLWindowKeyHandler.
> cpp#717

In WalkHandlersAndExecute(), IsExecutableElement() returns false due to no command attribute.  So HasHandlerForEvent returns false.  Then mIsReserved isn't set to true.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Makoto Kato [:m_kato] from comment #3)
> Created attachment 8749587 [details]
> MozReview Request: Bug 1253284 - Allow reserved attribute without command
> attribute
> 
> Review commit: https://reviewboard.mozilla.org/r/51053/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/51053/

Looks good, should I review this?

Comment 6

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 4/29-5/8) from comment #5)
> (In reply to Makoto Kato [:m_kato] from comment #3)
> > Created attachment 8749587 [details]
> > MozReview Request: Bug 1253284 - Allow reserved attribute without command
> > attribute
> > 
> > Review commit: https://reviewboard.mozilla.org/r/51053/diff/#index_header
> > See other reviews: https://reviewboard.mozilla.org/r/51053/
> 
> Looks good, should I review this?

I am investigating whether there is another way...

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Any update on this? Should we proceed with the current patch?

Comment 8

[Makoto Kato [:m_kato]]

humm, although I investigated another way like reading attribute or hooking on global menu bar, but no way to hook it.... So we should handle it on nsXBLWindowKeyHandler like my first idea.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8749587 [details]
MozReview Request: Bug 1253284 - Allow reserved attribute without command attribute

https://reviewboard.mozilla.org/r/51053/#review50224

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4dab7aaa3f8

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4dab7aaa3f8