336740 - Key navigation (and all shortcuts) broken on webpages

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Frank Wein [:mcsmurf]]

In a current SeaMonkey trunk build navigation via the keyboard (for example scrolling with the space key or moving to links with the tab key) does not work anymore. Also Ctrl+C and similar shortcuts seem to be broken.
This looks like a fall-out from Bug 331290.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Need fix before the weekend...

Comment 2

[Brian Ryner (not reading)]

On it (I tested commands like ctrl+t, ctrl+w, I guess these are different?)

Comment 3

[Boris Zbarsky [:bzbarsky]]

So in Firefox, space bar scrolling works, but Ctrl-C to copy does not (on Linux).

Comment 4

[walter]

I'm assuming that the same bug is affecting thunderbird
trunk today -- all the keystroke shortcuts I've tried
do nothing at all.

Comment 5

[Brian Ryner (not reading)]

Attachment: patch

Ok, this should fix things.  There were two problems:

1. Not propagating modifiers, as Neil pointed out
2. "dummy" key elements that don't have a command/oncommand should be skipped over... in the old code ExecuteHandler would return NS_ERROR_FAILURE.  I just added this check in with the "disabled" check in nsXBLWindowHandler. Does that seem ok?  I also cleaned up the loop in WalkHandlersInternal a bit.

Tested with ctrl+c in content area on Firefox, Windows.


I'd like to do a follow-up to this that makes the <key> handling a little more sane... it's so disjoint from most of XBL that the extra logic in nsXBLPrototypeHandler to accomodate it just makes things confusing.  I'm thinking we really just want to factor things so that we can share the modifier/filtering code and otherwise have the implementation be separate.

Comment 6

[[e-mail]]

Comment on attachment 220980 [details] [diff] [review]
patch

This is just a quick glance for now.

>+    nsIDocument *doc = mHandlerElement->GetOwnerDoc();
I'm sure I saw the caller use GetCurrentDoc(); I'd like to think they can't both be right ;-)

>+      // Check that there is an oncommand handler
>+      commandElt->GetAttribute(NS_LITERAL_STRING("oncommand"), value);
>+      if (value.IsEmpty()) {
>+        continue;  // nothing to do
>       }
I'm not sure what the point of this is; what if someone wanted to use an event listener, or an oncommand handler on a parent element?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 220980 [details] [diff] [review]
patch

>Index: content/xbl/src/nsXBLPrototypeHandler.cpp
>+    nsIDocument *doc = mHandlerElement->GetOwnerDoc();

I think you want GetCurrentDoc here.

Could you maybe post a diff -w of the nsXBLWindowHandler.cpp change?  It's hard to tell what's really changing there...

Comment 8

[Brian Ryner (not reading)]

(In reply to comment #6)
> >+      // Check that there is an oncommand handler
> >+      commandElt->GetAttribute(NS_LITERAL_STRING("oncommand"), value);
> >+      if (value.IsEmpty()) {
> >+        continue;  // nothing to do
> >       }
> I'm not sure what the point of this is; what if someone wanted to use an event
> listener, or an oncommand handler on a parent element?
> 

This is actually crucial to keep things working.  browser.xul (browser-sets.inc, I should say, has elements like this):

    <key id="key_copy"
         key="&copyCmd.key;"
         modifiers="accel"/>

This is bound to a menuitem:

                <menuitem label="&copyCmd.label;"
                          key="key_copy"
                          accesskey="&copyCmd.accesskey;"
                          command="cmd_copy"/>

I talked to Ben about this, and he said using the <key> element like this makes the right shortcut show up in the menu item.  However, we definitely don't want to dispatch the actual command to this key element, because nothing will happen.  You're right that you can't attach an command event listener with addEventListener or put it on a parent element... I was preserving the existing (buggy) behavior, because I couldn't think of a good way to do otherwise without mandating that command listeners call preventDefault() on their own, which would be a pretty huge change.

Comment 9

[Brian Ryner (not reading)]

Attachment: diff -w (contains the owner -> current doc change)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 220980 [details] [diff] [review]
patch

Looks OK, though maybe we can add an attribute on these <key> elements that are just being used for menu purposes to indicate that that's what they're for?  Then we could check that instead of checking the oncommand attr...  That would require changes to various XUL code that uses menus, I guess.  :(

Comment 11

[Peter van der Woude [:Peter6]]

*** Bug 336864 has been marked as a duplicate of this bug. ***

Comment 12

[[e-mail]]

(In reply to comment #8)
>(In reply to comment #6)
>>>+      // Check that there is an oncommand handler
>>>+      commandElt->GetAttribute(NS_LITERAL_STRING("oncommand"), value);
>>>+      if (value.IsEmpty()) {
>>>+        continue;  // nothing to do
>>>       }
>>I'm not sure what the point of this is; what if someone wanted to use an event
>>listener, or an oncommand handler on a parent element?
>This is actually crucial to keep things working.
Actually, I tracked the problem down to nested <keyset>s: <keyset>s assume that they only contain key elements; when it finds a nested key element it always matches because there's no key attribute on the element, so what happens when you type is that the command events keep getting fired on that child keyset.

Comment 13

[Brian Ryner (not reading)]

(In reply to comment #12)
> Actually, I tracked the problem down to nested <keyset>s: <keyset>s assume that
> they only contain key elements; when it finds a nested key element it always
> matches because there's no key attribute on the element, so what happens when
> you type is that the command events keep getting fired on that child keyset.
> 

I'm not sure I follow, which problem did you track down?  Where do we have nested keysets?

Comment 14

[Brian Ryner (not reading)]

I just rechecked the copy command case, it's definitely hitting the <key> with no command/oncommand, not a <keyset>.

Comment 15

[[e-mail]]

Comment on attachment 220980 [details] [diff] [review]
patch

As this is a smoketest blocker I'll grant r+ on the immediate fix but I'd appreciate your review of my forthcoming oncommand attribute avoidance patch.

Comment 16

[Brian Ryner (not reading)]

Checked in, thanks.  I'll be happy to review anything that makes this hack go away :)

Comment 17

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060506 Minefield/3.0a1 ID:2006050621 [cairo]

confirming ctrl+A/C/V/Z work again 

Comment 18

[u60234]

Mozilla/5.0 (Windows; U; Windows NT 5.1; sv-SE; rv:1.9a1) Gecko/20060507 Minefield/3.0a1 Build-ID:2006050700

In a non-Places Minefield trunk build, the shortcuts for History (Ctrl+H) and Bookmarks (Ctrl+B) sidebars is still broken.

Comment 19

[Frank Wein [:mcsmurf]]

*** Bug 336979 has been marked as a duplicate of this bug. ***

Comment 20

[[e-mail]]

(In reply to comment #13)
>I'm not sure I follow, which problem did you track down?
The key navigation issue; the shortcuts issue is a different issue although the same workaround happens to work there too.

>Where do we have nested keysets?
They are all over the place in SeaMonkey; Thunderbird also has at least one.

Comment 21

[[e-mail]]

Attachment: Only create handlers for keys

This -w patch is just a basic sanity check.

Comment 22

[[e-mail]]

Attachment: Includes cleanup

Comment 23

[Brian Ryner (not reading)]

(In reply to comment #18)
> In a non-Places Minefield trunk build, the shortcuts for History (Ctrl+H) and
> Bookmarks (Ctrl+B) sidebars is still broken.

Confirmed, the reason is that it's doing this:

<broadcaster id="viewBookmarksSidebar" autoCheck="false" label="&bookmarksButton.label;"
             type="checkbox" group="sidebar" sidebarurl="chrome://browser/content/bookmarks/bookmarksPanel.xul"
             oncommand="toggleSidebar('viewBookmarksSidebar');"/>

<key id="viewBookmarksSidebarKb" key="&bookmarksSidebarCmd.commandkey;" command="viewBookmarksSidebar" modifiers="accel"/>

... and the code in nsXULElement::PreHandleEvent explicitly checks that the element referenced by "command" is a <command> before forwarding the event.  I'm fine with allowing it for <broadcaster> as well, but I have two questions:

1. Can someone explain how and why this is supposed to work?  I read the XULPlanet docs on broadcasters and there's no mention of using them to double as commands.  Is <broadcaster> intended to be a superset of the functionality of <command>?  Or is this a misusage, where there should really be a separate <command>?

2. Do we need to care what type of element we're forwarding the event to?  If a XUL element has command="cmd_foo", and there's an element with id cmd_foo, does it matter what type of element it is?

Comment 24

[Neil Deakin]

<broadcaster> is just a placeholder tag. Attribute forwarding does the same thing if the tag was <wildpig>.

An observes or command attribute on an element are both equivalent on all tags except <menuitem> and <key>, where only certain attributes are forwarded (label, disabled and checked). Looks like the sidebar shortcuts might be relying on this. What else is observing "viewBookmarksSidebar"?

The <command> element should semantically though be used for commands (operations) and broadcaster should be used for flags or whatever. The sidebar is doing something strange.

Comment 25

[Brian Ryner (not reading)]

(In reply to comment #24)
> An observes or command attribute on an element are both equivalent on all tags
> except <menuitem> and <key>, where only certain attributes are forwarded
> (label, disabled and checked). Looks like the sidebar shortcuts might be
> relying on this. What else is observing "viewBookmarksSidebar"?

Ok, some experimentation (with FF1.5) shows that they are mostly equivalent.  I found one difference, which is explained by the code in nsXULElement::PreHandleEvent -- anything that generates a command event, like clicking a button, will only forward via the "command" attribute if that attribute references a <command>.  Does not work for a <broadcaster>, nor a <wildpig>.  I'm guessing the reason this works right for observes= is that the oncommand= from the broadcaster is forwarded onto the observing element, rather than needing to forward the event to the command element.

In any case, we should fix the inconsistency -- either we should only allow command= to forward to a command, which will break this chunk of XUL, or we should remove the check and allow it to forward anywhere.  Doing one thing for <key> and another thing for UI elements doesn't make a lot of sense.

Comment 26

[[e-mail]]

Comment on attachment 221247 [details] [diff] [review]
Includes cleanup

As I'm touching the code anyway, I thought I'd rewrite the loop to simplify it. See what you think.

Comment 27

[[e-mail]]

(In reply to comment #24)
>What else is observing "viewBookmarksSidebar"?
Strangely enough, a menuitem. And yes, it is using observes, not command (I guess it wouldn't have worked with command). Perhaps the simplest solution is to change viewBookmarksSidebar to a command (which still works with observes).

Comment 28

[Neil Deakin]

The menu code doesn't check if it is a <command> either, so probably best to just allow any element.

Comment 29

[Brian Ryner (not reading)]

Comment on attachment 221247 [details] [diff] [review]
Includes cleanup

>--- nsXBLWindowKeyHandler.cpp	14 Mar 2006 16:58:22 -0000	1.31
>+++ nsXBLWindowKeyHandler.cpp	7 May 2006 23:43:48 -0000
>@@ -81,23 +81,20 @@
> static void
> BuildHandlerChain(nsIContent* aContent, nsXBLPrototypeHandler** aResult)
> {
>-  nsXBLPrototypeHandler *firstHandler = nsnull, *currHandler = nsnull;
>+  *aResult = nsnull;
> 
>-  PRUint32 handlerCount = aContent->GetChildCount();
>-  for (PRUint32 j = 0; j < handlerCount; j++) {
>-    nsIContent *handler = aContent->GetChildAt(j);
>+  for (PRUint32 j = aContent->GetChildCount(); j--; ) {

This is a clever way of looping such that the handler chain is built up in reverse order.  I'd suggest adding a comment that says what the intent is, since at first glance it looks like it's trying to give later children precedence in the handler chain, when in fact it's the opposite.

>-    if (newHandler) {
>-      if (currHandler)
>-        currHandler->SetNextHandler(newHandler);
>-      else firstHandler = newHandler;
>-      currHandler = newHandler;
>+      if (handler) {

if handler is null here, maybe we should just break?

Looks good otherwise.

Comment 30

[Brian Ryner (not reading)]

Attachment: allow command= to point to anything

This removes the tag check for the target of command=, so that it can point to <broadcaster> or anything else.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 221368 [details] [diff] [review]
allow command= to point to anything

Hmm... sr=bzbarsky, because we can already get in trouble (infinite stack recursion) this way if two <xul:command>s are nested... But with this patch isn't that even easier?

Comment 32

[Brian Ryner (not reading)]

Comment on attachment 221368 [details] [diff] [review]
allow command= to point to anything

Um, I guess, do you have another suggestion?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Not offhand; I'd have made it if I did.  :(  Maybe not retargeting to things that are descendants of |this| or something?

Comment 34

[[e-mail]]

Comment on attachment 221247 [details] [diff] [review]
Includes cleanup

I've locally added a comment and replaced the OOM test with if (!handler) return;

Comment 35

[[e-mail]]

Comment on attachment 221368 [details] [diff] [review]
allow command= to point to anything

Perhaps you should test for a command attribute on the alleged command node?

Comment 36

[[e-mail]]

Comment on attachment 221368 [details] [diff] [review]
allow command= to point to anything

Perhaps you should test for a command attribute on the alleged command node?

Comment 37

[Brian Ryner (not reading)]

(In reply to comment #36)
> (From update of attachment 221368 [details] [diff] [review] [edit])
> Perhaps you should test for a command attribute on the alleged command node?
> 

You mean an oncommand attribute?  I thought you wanted to get rid of that sort of checking.

Comment 38

[[e-mail]]

(In reply to comment #37)
>(In reply to comment #36)
>>(From update of attachment 221368 [details] [diff] [review] [edit] [edit])
>>Perhaps you should test for a command attribute on the alleged command node?
>You mean an oncommand attribute?
Sorry for being unclear, I meant as a suggestion to avoid comment 31.

Comment 39

[Brian Ryner (not reading)]

checked in.

Comment 40

[[e-mail]]

(In reply to comment #14)
>I just rechecked the copy command case, it's definitely hitting the <key> with
>no command/oncommand, not a <keyset>.
I was checking the scrolling keys at the time, but I see what you mean.

(In reply to comment #10)
>(From update of attachment 220980 [details] [diff] [review])
>Looks OK, though maybe we can add an attribute on these <key> elements that are
>just being used for menu purposes to indicate that that's what they're for? 
How about we use disabled="true" for those keys?

Comment 41

[[e-mail]]

Attachment: As per comment 40

Comment 42

[Brian Ryner (not reading)]

Comment on attachment 221963 [details] [diff] [review]
As per comment 40

This would be confusing to me as a XUL author... it seems to imply that if you hook this <key> up to a menuitem, the menuitem will be disabled.