Closed
Bug 331290
Opened 19 years ago
Closed 19 years ago
xul <key> handling doesn't fire a command event
Categories
(Core :: XUL, defect, P1)
Core
XUL
Tracking
()
RESOLVED
FIXED
People
(Reporter: bryner, Assigned: bryner)
References
Details
(Keywords: fixed1.8.1)
Attachments
(2 files)
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
bzbarsky
:
approval-branch-1.8.1-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
nsXBLPrototypeHandler::ExecuteHandler just finds the command element and executes its script directly. It really should fire a command event so that this follows a similar code path to <button command="cmdFoo"/>. This would make it possible to use an event listener to watch for <key>-triggered commands.
Comment 1•19 years ago
|
||
So this is the case when isXBLCommand and isReceiverCommandElement are both true? Or the case when isReceiverCommandElement is false?
Comment 2•19 years ago
|
||
(In reply to comment #1)
>So this is the case when isXBLCommand and isReceiverCommandElement are both
>true? Or the case when isReceiverCommandElement is false?
This has nothing to do with isXBLCommand (that's for platformHTMLBindings.xml), this is for the isXULKey case; isReceiverCommandElement may be true or false.
Also I think this case is specific to the call from WalkHandlersInternal.
Comment 3•19 years ago
|
||
Hmm. So for isXULKey, this would be the case when the "oncommand" attr is empty and isReceiverCommandElement is true?
Put another way, which node should we be firing the event on?
Comment 4•19 years ago
|
||
The oncommand attribute cannot currently be empty, otherwise nothing would ever get executed. That is the point of this bug...
The event should be fired on the receiver. The caller has already computed that. What I don't know is why the caller didn't fire the command event itself. Note that the event should be fired in a way that preserves the modifiers.
Comment 5•19 years ago
|
||
> The oncommand attribute cannot currently be empty
So what's the code at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp&rev=1.101&mark=378,380-382#375 doing? Is the stuff inside that IsEmpty() if check just dead code?
Comment 6•19 years ago
|
||
Sorry, I meant that from a XUL developer's point of view. Perhaps I should have said that the oncommand attribute currently cannot usefully be empty?
Assignee | ||
Comment 7•19 years ago
|
||
This should apply both to:
<key oncommand="..."/>
and
<key command="..."/>
What would make the most sense to me, in either case, is just to fire a command event at the key element once we've computed that it matches in WalkHandlersInternal (rather than calling ExecuteHandler). nsXULElement::PreHandleEvent will take care of retargeting it in the command= case. For the oncommand= case, I'm assuming that whatever generic code makes onfoo="blah();" install an event handler will have set one up for "command" on the <key> element.
Does that sound workable?
Comment 8•19 years ago
|
||
Sounds good to me. I had a peek at ExecuteHandler myself but wasn't quite sure which bits were used for <keys>, which bits weren't, and which were shared.
Comment 9•19 years ago
|
||
Sounds reasonable to me too.
Assignee | ||
Comment 10•19 years ago
|
||
Assignee: nobody → bryner
Status: NEW → ASSIGNED
Attachment #220863 -
Flags: superreview?(bzbarsky)
Attachment #220863 -
Flags: review?(bzbarsky)
Updated•19 years ago
|
Attachment #220863 -
Flags: superreview?(bzbarsky)
Attachment #220863 -
Flags: superreview+
Attachment #220863 -
Flags: review?(bzbarsky)
Attachment #220863 -
Flags: review+
Assignee | ||
Comment 11•19 years ago
|
||
checked in on trunk
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•19 years ago
|
||
Comment on attachment 220863 [details] [diff] [review]
patch
What do you think about taking this one on the branch? I'd really like to have it there for bug 328069, and it should be pretty safe... maybe we should just give it a few days to shake out any regressions?
Attachment #220863 -
Flags: approval-branch-1.8.1?(bzbarsky)
Comment 13•19 years ago
|
||
I'd give it a week or week and a half on trunk, then see how things look... This code scares me, mostly because I don't understand it as well as I probably should.
Comment 14•19 years ago
|
||
(In reply to comment #4)
>Note that the event should be fired in a way that preserves the modifiers.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 15•19 years ago
|
||
... e.g. the SpaceHit function is called for both space and shift+space events and needs to know which modifiers were pressed; also for consistency with everywhere else that we generate command events from UI events.
Comment 16•19 years ago
|
||
Comment on attachment 220863 [details] [diff] [review]
patch
I've verified (by local backout) that this causes blocker bug 336740. Given that and Neil's comments....
Attachment #220863 -
Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1-
Assignee | ||
Updated•19 years ago
|
Priority: -- → P1
Assignee | ||
Comment 17•19 years ago
|
||
ok, regression fixes are in, marking this FIXED again.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 18•19 years ago
|
||
This has the regression fixes from 336740, and I backported it to use HandleDOMEvent() for dispatching the event.
Attachment #221736 -
Flags: approval-branch-1.8.1?(bzbarsky)
Comment 19•18 years ago
|
||
Comment on attachment 221736 [details] [diff] [review]
revised patch for branch
>Index: content/xbl/src/nsXBLPrototypeHandler.cpp
>+ // if the focused element is a link then we do want space to
>+ // scroll down. focused element may be an element in a
Fix the indent?
a=bzbarsky for the branch.
Attachment #221736 -
Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
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.
Description
•