Closed Bug 569130 Opened 15 years ago Closed 15 years ago

Intercept specific key commands before sending them to core

Categories

(Camino Graveyard :: History, defect)

1.9.2 Branch
All
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino2.1

People

(Reporter: stuart.morgan+bugzilla, Assigned: stuart.morgan+bugzilla)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files, 1 obsolete file)

Now that we aren't on 1.9.0, we need to re-solve bug 459744. The most sustainable option is to do it at the Camino layer so we aren't subject to the politics and timetable of a Core solution. My current thinking is that we should use a whitelist approach (which is what Safari and Chrome at least do), rather than sending *everything* to the menus first as our 1.9.0 hack did, for consistency with what other browsers are doing. There's a good argument that, for example, if I use Command-I on Google Docs I'm probably much more likely to be trying to make text italic than get page info. I think as a starting place we should protect: - Core window/tab management (new window/tab, close window/tab, cycle windows, switch tabs) - Standard OS shortcuts for app and window management (quit, minimize, hide) - Focusing the location bar (Command-L) as an escape hatch to get focus somewhere where everything else works. That should eliminate most of the surprises without blocking too many web app shortcuts. If there are really good arguments for or against certain shortcuts after we've been living on that for a while, we can evaluate them case-by-case.
(I should mention that if everyone dislikes this idea as much as I used to, I can write something at our layer that is basically what did in core for 1.9.0, with the caveat that if our plugin detection is not 100% reliable we may occasionally break copy/paste in plugins.)
Attached patch whitelist (deleted) — Splinter Review
The layer of forwarding here is because we have to go through the BrowserUIDelegate to call up from BrowserWrapper.
Attachment #448365 - Flags: superreview?(mikepinkerton)
Attached file new nib (obsolete) (deleted) —
New nib, with all the new outlets. This was made on 10.6, so will need re-saving.
How did thakis solve this in chrome? I don't recall a whitelist. We should probably try something similar, no?
I tested the mentioned combinations with my nib and Josh's "blackhole everything" testcase, and they all work. Two things Stuart and I discussed on irc: 1) Reverse-cycle (shift-cmd-` aka cmd-~ on US English keyboards) isn't protected; it probably should be 2) Unfortunately, the shortcut for Cycle Windows is different per keyboard layout (and possibly other factors, like UI locale), e.g. on AZERTY, it's Cmd-Ctrl-< (Cmd-Ctrl->, aka Cmd-Ctrl-Shift-<, for reverse-cycle); searching "key hell" bugs should turn up others, and we can also poke l10n if needed. Stuart suggested this part will probably need to be punted to a follow-up, since there's no good way to figure out what the shortcut currently is, afaik.
(In reply to comment #4) > How did thakis solve this in chrome? I don't recall a whitelist. We should > probably try something similar, no? In Chrome, the browser window overrides performKeyEvent:, it does an event->command ID lookup, then calls the following function: bool Browser::IsReservedCommand(int command_id) { return command_id == IDC_CLOSE_TAB || command_id == IDC_CLOSE_WINDOW || command_id == IDC_NEW_INCOGNITO_WINDOW || command_id == IDC_NEW_TAB || command_id == IDC_NEW_WINDOW || command_id == IDC_RESTORE_TAB || command_id == IDC_SELECT_NEXT_TAB || command_id == IDC_SELECT_PREVIOUS_TAB || command_id == IDC_EXIT; } If that returns true, then the event it sent to the menu, and only forwarded on to the renderer if the menu didn't return YES. I would go so far as to claim that this is, in fact, similar :)
Comment on attachment 448365 [details] [diff] [review] whitelist sr=pink
Attachment #448365 - Flags: superreview?(mikepinkerton) → superreview+
Blocks: 501867
Landed 3108:393a395f478e with Command-~ fixed Filed 569781 for other locales (comment 6 part 2)
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
(Since there is no "Trunk" just yet, let's keep this at "1.9.2 Branch" for tracking purposes. Also, pasting the changeset URL into the comment when closing the bug will be helpful in the future when we do have multiple repos to differentiate ;) )
Version: Trunk → 1.9.2 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: