Closed Bug 577331 Opened 14 years ago Closed 1 month ago

Javascript on page overrides custom next/previous tab commands that use Shift

Categories

(Camino Graveyard :: Tabbed Browsing, defect)

Product:
Camino Graveyard
Component:
Tabbed Browsing
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: David, Unassigned)

References

(
URL
)

Details

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.2.7pre) Gecko/20100630 Camino/2.1a1pre (like Firefox/3.6.7pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.2.7pre) Gecko/20100630 Camino/2.1a1pre (like Firefox/3.6.7pre)

Javascript on the page below overrides custom Previous/Next Tab shortcuts.

Reproducible: Always

Steps to Reproduce:
1. Change Previous/Next Tab commands to something custom in System Preferences (I use command-shift-arrows)
2. Go to http://news.bbc.co.uk/2/hi/science_and_environment/10529154.stm
3. Use Previous/Next Tab shortcut on the page (Shortcut, do not go to the menu)

Actual Results:  
The images on the webpage are changed, as if the 'next image'-arrow was clicked.

Expected Results:  
Next or previous tab should be selected.

The normal commands (command-alternate-arrows) works fine. I haven't tested with other shortcuts than the one i use.
The BBC sets those as shortcuts to cycle through the images.

( I think:
http://node1.bbcimg.co.uk/glow/glow/1.7.3/widgets/widgets.js
)

bug 569130 made it so that the _default_ shortcut is protected. I'm not sure if that covers custom shortcuts, though.
(In reply to comment #1)
> bug 569130 made it so that the _default_ shortcut is protected. I'm not sure if
> that covers custom shortcuts, though.

We thought the API worked such that whatever was showing in the menu as the shortcut would be protected (and, I guess, still do think that); Stuart thinks that perhaps shift-handling isn't working right in the protection code.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: camino2.1?
(In reply to comment #2)
> We thought the API worked such that whatever was showing in the menu as the
> shortcut would be protected (and, I guess, still do think that); Stuart
> thinks that perhaps shift-handling isn't working right in the protection
> code.

Stuart, is there something we can log to see what's happening here?

This one sounds possibly fixable, if we can figure out what's going on.
Flags: camino2.1?
Flags: camino2.1.1?
Flags: camino2.1-
Yes, just inside the while loop
+  while ((menuItem = [nonOverridableMenuEnumerator nextObject])) {
+    if (eventModifiers == [menuItem keyEquivalentModifierMask] &&
+        [eventString isEqualToString:[menuItem keyEquivalent]])
+    {
+      return YES;
+    }
+  }
add
  NSLog(@"'%@' %u", [menuItem keyEquivalent], [menuItem keyEquivalentModifierMask]);

and before it add
  NSLog(@"Looking for match for '%@' %u", eventString, eventModifiers);

Then we can see what we're looking for, and what we are finding (and see if we are seeing case mismatch, for example).
First, for reference, what we see with the default Cmd-Opt-L/RArrow:

Next (default)
Camino[7762]: Looking for match for '' 1572864
Camino[7762]: 'q' 1048576
Camino[7762]: 'W' 1048576
Camino[7762]: 'w' 1048576
Camino[7762]: '' 1572864
Camino[7762]: '' 1572864
Previous (default)
Camino[7762]: Looking for match for '' 1572864
Camino[7762]: 'q' 1048576
Camino[7762]: 'W' 1048576
Camino[7762]: 'w' 1048576
Camino[7762]: '' 1572864

Here's the logging from the broken case, using David's Cmd-Shift-L/RArrow:

Next (Cmd-Shift-RightArrow)
Camino[7762]: Looking for match for '' 1048576
Camino[7762]: 'q' 1048576
Camino[7762]: 'W' 1048576
Camino[7762]: 'w' 1048576
Camino[7762]: '' 1179648
Camino[7762]: '' 1179648
Camino[7762]: 'n' 1048576
Camino[7762]: 't' 1048576
Camino[7762]: 'h' 1048576
Camino[7762]: 'm' 1048576
Camino[7762]: 'l' 1048576
Previous (Cmd-Shift-LeftArrow)
Camino[7762]: Looking for match for '' 1048576
Camino[7762]: 'q' 1048576
Camino[7762]: 'W' 1048576
Camino[7762]: 'w' 1048576
Camino[7762]: '' 1179648
Camino[7762]: '' 1179648
Camino[7762]: 'n' 1048576
Camino[7762]: 't' 1048576
Camino[7762]: 'h' 1048576
Camino[7762]: 'm' 1048576
Camino[7762]: 'l' 1048576

(That box is U+F702.)  (Also, the logging is the same whether I modify the shortcuts while Camino is running or whether I launch it after customizing.)
Oh, to further confirm our old working theory that it's shift-handling that's busted, here's the logging for a run with Cmd-Ctrl-L/RArrow, where the shortcuts are successfully protected:

Camino[7855]: Looking for match for '' 1310720
Camino[7855]: 'q' 1048576
Camino[7855]: 'W' 1048576
Camino[7855]: 'w' 1048576
Camino[7855]: '' 1310720
Camino[7855]: '' 1310720
Camino[7855]: Looking for match for '' 1310720
Camino[7855]: 'q' 1048576
Camino[7855]: 'W' 1048576
Camino[7855]: 'w' 1048576
Camino[7855]: '' 1310720
I noticed that we're not actually checking for NSShiftKeyMask when we're creating eventModifiers, which I assume is the source of the problem here, but I also assume we weren't checking for it on purpose…
Shift is super-annoying, which is why I figured it was the problem; it's not always clear what the right thing to check is. I'm not checking shift because, e.g., command-shift-w looks like command-W when we get it IIRC. I think this is the issue here (from the NSMenu docs; I hadn't seen this specifically before):

"NSShiftKeyMask is a valid modifier for any key equivalent in mask. This allows you to specify key-equivalents such as Command-Shift-1 that are consistent across all keyboards. However, with a few exceptions (such as the German “ß” character), a lowercase character with NSShiftKeyMask is interpreted the same as the uppercase character without that mask. For example, Command-Shift-c and Command-C are considered to be identical key equivalents."

I'm not sure how we get the right behavior here given the unspecified "few exceptions" :P

I gave bad advice on the logging though, which makes this harder to reason about. Can you change the %u to %x in both, and add an:
  & NSDeviceIndependentModifierFlagsMask
to the end of each of the calls? E.g,
  NSLog(@"'%@' %x", [menuItem keyEquivalent], [menuItem keyEquivalentModifierMask] & NSDeviceIndependentModifierFlagsMask);

That will save doing the hex conversions on all those numbers every time I try to look at them :)
Sorry it took me so long to get you the hex-less logging (I did wonder initially if there was no better output format available!):

Next (default)
Camino[8954]: Looking for match for '' 180000
Camino[8954]: 'q' 100000
Camino[8954]: 'W' 100000
Camino[8954]: 'w' 100000
Camino[8954]: '' 180000
Camino[8954]: '' 180000
Previous (default)
Camino[8954]: Looking for match for '' 180000
Camino[8954]: 'q' 100000
Camino[8954]: 'W' 100000
Camino[8954]: 'w' 100000
Camino[8954]: '' 180000

Next (Cmd-Shift-RightArrow)
Camino[8954]: Looking for match for '' 100000
Camino[8954]: 'q' 100000
Camino[8954]: 'W' 100000
Camino[8954]: 'w' 100000
Camino[8954]: '' 120000
Camino[8954]: '' 120000
Camino[8954]: 'n' 100000
Camino[8954]: 't' 100000
Camino[8954]: 'h' 100000
Camino[8954]: 'm' 100000
Camino[8954]: 'l' 100000
Previous (Cmd-Shift-LeftArrow)
Camino[8954]: Looking for match for '' 100000
Camino[8954]: 'q' 100000
Camino[8954]: 'W' 100000
Camino[8954]: 'w' 100000
Camino[8954]: '' 120000
Camino[8954]: '' 120000
Camino[8954]: 'n' 100000
Camino[8954]: 't' 100000
Camino[8954]: 'h' 100000
Camino[8954]: 'm' 100000
Camino[8954]: 'l' 100000
Summary: Javascript on page overrides custom next/previous tab commands → Javascript on page overrides custom next/previous tab commands that use Shift
For quick decoding reference:
100000 - Command
80000 - Option
40000 - Control
20000 - Shift

So the menu item has the NSShiftKeyMask, but the generated event with our mask doesn't. I'm going to have to spend some time playing with this to see what cases do and don't have the shift mask in menus, and what the corresponding character-without-modifiers is, to see if I can figure out a version that handles everything correctly.
Here's a walking-down-the-stairs-after-a-shower thought: can we check the key equivalent, and, if it's not a letter/number (or, if it is a whatever-the-arrow-keys-and-other-suff-etc-sends), check for shift only then?

I'm not positive that would cover the "few exceptions" cases, but it should let us fix *this* bug, right?
Flags: camino2.1.2?
Flags: camino2.1.1?
Flags: camino2.1.1-
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.