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

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Jesse Ruderman]

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

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: 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.

Comment 3

[Jesse Ruderman]

Could we use the new activeElement stuff from bug 337631?  I guess that depends on how it interacts with XBL.

Comment 4

[Shawn Wilsher :sdwilsh]

(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)

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: 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?

Comment 6

[[e-mail]]

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.

Comment 7

[Shawn Wilsher :sdwilsh]

(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?

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: 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?

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v4

*sigh*

Two in one day.  As per conversation with Neil on irc.

Comment 10

[[e-mail]]

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.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 230778 [details] [diff] [review]
v4

r=mano

Comment 12

[Asaf Romano (gone)]

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

Comment 13

[aaronr]

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?

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 230778 [details] [diff] [review]
v4

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers

Comment 15

[aaronr]

checked into 1.8 branch

Comment 16

[aaronr]

checked into 1.8.0 branch

Comment 18

[Tracy Walker [:tracy]]

Verified with
2.0.0.4 on Windows
1.5.0.12 on Mac
trunk on Windows

Comment 19

[[e-mail]]

We forgot editable menulists. Oops.

Comment 20

[Shawn Wilsher :sdwilsh]

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

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