Closed Bug 1253284 Opened 4 years ago Closed 4 years ago

Cmd+Q isn't reserved on OS X

Categories

(Firefox :: Keyboard Navigation, defect, P4)

defect

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
e10s - ---
firefox47 --- affected
firefox49 --- fixed

People

(Reporter: canuckistani, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Assignee: nobody → 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.
(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 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.
(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?
(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...
Priority: -- → P4
Any update on this? Should we proceed with the current patch?
Flags: needinfo?(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.
Flags: needinfo?(m_kato)
Attachment #8749587 - Flags: review?(masayuki)
Attachment #8749587 - Flags: review?(masayuki) → review+
Comment on attachment 8749587 [details]
MozReview Request: Bug 1253284 - Allow reserved attribute without command attribute

https://reviewboard.mozilla.org/r/51053/#review50224
https://hg.mozilla.org/mozilla-central/rev/d4dab7aaa3f8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.