If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

XUL textbox in XHTML page: focusing gives document.commandDispatcher has no properties

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Toolkit
XUL Widgets
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: sdwilsh)

Tracking

(Blocks: 1 bug, {testcase, verified1.8.0.12, verified1.8.1.4})

Trunk
mozilla1.9alpha1
testcase, verified1.8.0.12, verified1.8.1.4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
Steps to reproduce:
1. Load the testcase.
2. Click the textbox.

Result:
JavaScript error: chrome://global/content/bindings/textbox.xml, line 173: document.commandDispatcher has no properties
(Reporter)

Comment 1

12 years ago
Created attachment 214061 [details]
testcase
(Assignee)

Comment 2

11 years ago
Created attachment 225828 [details] [diff] [review]
Patch

This will solve the problem, but I don't think it is a good solution.

document.commandDispatcher is used all over the place with a textbox, but commandDispatcher is only in XUL documents, which is why the error occurs in the first place.
(Reporter)

Comment 3

11 years ago
Could we use the new activeElement stuff from bug 337631?  I guess that depends on how it interacts with XBL.
(Assignee)

Comment 4

11 years ago
(In reply to comment #3)
> Could we use the new activeElement stuff from bug 337631?  I guess that depends
> on how it interacts with XBL.

From what I can tell, that should do the trick.  It is in a couple of places, but if we change all of the document.commandDispatcher.focusedElement to a conditional testing first for document.commandDispatcher and if not use document.activeElement.

Once that bug lands, I think I'll write up a patch for that as I think document.activeElement will work within a binding (I don't see why it wouldn't)
(Assignee)

Comment 5

11 years ago
Created attachment 228541 [details] [diff] [review]
Patch v2

I hadn't realized that there were two textbox bindings, but I fixed this for both (mcsmurf has informed me that one is for Seamonkey/Mozilla, one for Firefox/Thunderbird).  The change is that it first checks document.commandDispatcher, and then sees if it is the focused element, and if document.commandDispatcher does not exists, it uses document.activeElement.

neil - can you review this for xpfe/ as r+sr?
Assignee: nobody → comrade693
Attachment #225828 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #228541 - Flags: first-review?(neil)

Comment 6

11 years ago
Comment on attachment 228541 [details] [diff] [review]
Patch v2

It seems to me that event.originalTarget would be more reliable although you should run the idea past bryner our focus guru to be sure.
Attachment #228541 - Flags: first-review?(neil)
(Assignee)

Comment 7

11 years ago
(In reply to comment #6)
> (From update of attachment 228541 [details] [diff] [review] [edit])
> It seems to me that event.originalTarget would be more reliable although you
> should run the idea past bryner our focus guru to be sure.
> 

I've been trying to get a hold of him for over a week with no luck.  Anyone know of the best way to get a hold of him?
(Assignee)

Comment 8

11 years ago
Created attachment 230768 [details] [diff] [review]
v3

After a conversation with pkasting/byrner on irc, document.commandDispatcher.focusedElement and document.activeElement can return different things.

Recap (from pkasting):
The two will do the same thing if there is a focused element and if that element is in the current document. However, if the focused element is in a child document (for example, an iframe), the activeElement returns the iframe in this document that's an ancestor of the focused element, while the commandDispatcher will give you the element directly, even though it isn't actually "in this document", and For some kinds of focuses, commandDispatcher will give you nothing while activeElement won't.  For example, if an iframe itself is focused.
Lastly, if both these fail, activeElement returns BODY or else the root element. The upshot is that activeElement is guaranteed to give you an answer (unless it throws) and the answer will be an element in the current document, whereas focusedElement guarantees none of this, and basically just exposes the implementation of focus internally.

So, since they can give different things I wanted to make sure that we never ever use activeElement if we have a commandDispatcher.  Instead of that nice one line change we were hoping for Neil, we end up with a four liner.  This will always resort to the same old behavior for XUL documents that we see now, but XHTML (or other xml based) documents will use activeElement.

Once again, Neil can you review this for xpfe/ as r+sr?
Attachment #228541 - Attachment is obsolete: true
Attachment #230768 - Flags: first-review?(neil)
(Assignee)

Comment 9

11 years ago
Created attachment 230778 [details] [diff] [review]
v4

*sigh*

Two in one day.  As per conversation with Neil on irc.
Attachment #230768 - Attachment is obsolete: true
Attachment #230778 - Flags: first-review?(neil)
Attachment #230768 - Flags: first-review?(neil)

Comment 10

11 years ago
Comment on attachment 230778 [details] [diff] [review]
v4

r+sr=me for xpfe, now that (thanks to pkasting) I'm sure that we're doing the right thing.
Attachment #230778 - Flags: first-review?(neil) → first-review+
(Assignee)

Updated

11 years ago
Attachment #230778 - Flags: second-review?(danodemano)
(Assignee)

Updated

11 years ago
Attachment #230778 - Flags: second-review?(danodemano) → second-review?(bugs.mano)
Comment on attachment 230778 [details] [diff] [review]
v4

r=mano
Attachment #230778 - Flags: second-review?(bugs.mano) → second-review+
Checking in toolkit/content/widgets/textbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/textbox.xml,v  <--  textbox.xml
new revision: 1.36; previous revision: 1.35
done
Checking in xpfe/global/resources/content/bindings/textbox.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/textbox.xml,v  <--  textbox.xml
new revision: 1.43; previous revision: 1.42
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
OS: Mac OS X 10.2 → All
Hardware: Macintosh → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha1

Comment 13

11 years ago
Comment on attachment 230778 [details] [diff] [review]
v4

We (XForms) have run into this issue on the branches using our custom spinbox widget.  It is causing us problems.

We are seeking approval for putting this patch on the branches.  This is a relatively simple fix that will help anyone else that uses xul textboxes in xhtml.  This isn't absolutely critical to us since we have been able to work around this issue since we are already extending textbox with our xbl binding.  But it may be an issue to others who aren't already using XBL so I'm submitting the request on their behalf.  Plus it keeps us free from having to support another hack in our code.

Does anyone in toolkit land have a problem with this patch going onto the branches?
Attachment #230778 - Flags: approval1.8.1.4?
Attachment #230778 - Flags: approval1.8.0.12?
Comment on attachment 230778 [details] [diff] [review]
v4

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers
Attachment #230778 - Flags: approval1.8.1.4?
Attachment #230778 - Flags: approval1.8.1.4+
Attachment #230778 - Flags: approval1.8.0.12?
Attachment #230778 - Flags: approval1.8.0.12+

Comment 15

11 years ago
checked into 1.8 branch
Keywords: fixed1.8.1.4

Comment 16

11 years ago
checked into 1.8.0 branch
Keywords: fixed1.8.0.12

Updated

11 years ago
Duplicate of this bug: 377958
Verified with
2.0.0.4 on Windows
1.5.0.12 on Mac
trunk on Windows
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.12, fixed1.8.1.4 → verified1.8.0.12, verified1.8.1.4

Comment 19

11 years ago
We forgot editable menulists. Oops.
(Assignee)

Comment 20

11 years ago
(In reply to comment #19)
> We forgot editable menulists. Oops.

Did this cause a regression?  I'm not sure I follow your comment.

Updated

11 years ago
Blocks: 381508
You need to log in before you can comment on or make changes to this bug.