Key navigation (and all shortcuts) broken on webpages

RESOLVED FIXED

Status

()

Core
XUL
P1
blocker
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: mcsmurf, Assigned: Brian Ryner (not reading))

Tracking

({smoketest})

Trunk
x86
Windows 2000
smoketest
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Reporter)

Description

12 years ago
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.
Need fix before the weekend...
Assignee: nobody → bryner
Severity: major → blocker
Keywords: smoketest
(Assignee)

Comment 2

12 years ago
On it (I tested commands like ctrl+t, ctrl+w, I guess these are different?)
So in Firefox, space bar scrolling works, but Ctrl-C to copy does not (on Linux).

Comment 4

12 years ago
I'm assuming that the same bug is affecting thunderbird
trunk today -- all the keystroke shortcuts I've tried
do nothing at all.
(Assignee)

Comment 5

12 years ago
Created attachment 220980 [details] [diff] [review]
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.
Attachment #220980 - Flags: superreview?(bzbarsky)
Attachment #220980 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #220980 - Flags: review? → review?(neil)

Comment 6

12 years ago
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 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...
(Assignee)

Comment 8

12 years ago
(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.
(Assignee)

Comment 9

12 years ago
Created attachment 221021 [details] [diff] [review]
diff -w (contains the owner -> current doc change)
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.  :(
Attachment #220980 - Flags: superreview?(bzbarsky) → superreview+
*** Bug 336864 has been marked as a duplicate of this bug. ***

Comment 12

12 years ago
(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.
(Assignee)

Comment 13

12 years ago
(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?
(Assignee)

Comment 14

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

Comment 15

12 years ago
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.
Attachment #220980 - Flags: review?(neil) → review+
(Assignee)

Comment 16

12 years ago
Checked in, thanks.  I'll be happy to review anything that makes this hack go away :)
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

12 years ago
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.
(Reporter)

Comment 19

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

Comment 20

12 years ago
(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

12 years ago
Created attachment 221246 [details] [diff] [review]
Only create handlers for keys

This -w patch is just a basic sanity check.
Attachment #221246 - Flags: review?(bryner)

Comment 22

12 years ago
Created attachment 221247 [details] [diff] [review]
Includes cleanup
(Assignee)

Updated

12 years ago
Attachment #221246 - Flags: review?(bryner) → review+
(Assignee)

Comment 23

12 years ago
(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

12 years ago
<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.
(Assignee)

Comment 25

12 years ago
(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

12 years ago
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.
Attachment #221247 - Attachment description: Includes cleanup but untested as yet → Includes cleanup
Attachment #221247 - Flags: review?(bryner)

Comment 27

12 years ago
(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

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

Comment 29

12 years ago
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.
Attachment #221247 - Flags: review?(bryner) → review+
(Assignee)

Comment 30

12 years ago
Created attachment 221368 [details] [diff] [review]
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.
Attachment #221368 - Flags: superreview?(bzbarsky)
Attachment #221368 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #221368 - Flags: review? → review?(neil)
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?
Attachment #221368 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 32

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

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

Comment 34

12 years ago
Comment on attachment 221247 [details] [diff] [review]
Includes cleanup

I've locally added a comment and replaced the OOM test with if (!handler) return;
Attachment #221247 - Flags: superreview?(bzbarsky)

Comment 35

12 years ago
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?
Attachment #221368 - Flags: review?(neil) → review+

Comment 36

12 years ago
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?
(Assignee)

Comment 37

12 years ago
(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.
Attachment #221247 - Flags: superreview?(bzbarsky) → superreview+

Comment 38

12 years ago
(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.
(Assignee)

Updated

12 years ago
Priority: -- → P1
(Assignee)

Comment 39

12 years ago
checked in.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 40

12 years ago
(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

12 years ago
Created attachment 221963 [details] [diff] [review]
As per comment 40
Attachment #221963 - Flags: review?(bryner)
(Assignee)

Comment 42

12 years ago
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.
Attachment #221963 - Flags: review?(bryner) → review-

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.