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.
Could we use the new activeElement stuff from bug 337631? I guess that depends on how it interacts with XBL.
(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)
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?
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.
(In reply to comment #6) > (From update of attachment 228541 [details] [diff] [review] ) > 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?
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?
Created attachment 230778 [details] [diff] [review] v4 *sigh* Two in one day. As per conversation with Neil on irc.
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 on attachment 230778 [details] [diff] [review] v4 r=mano
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 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 on attachment 230778 [details] [diff] [review] v4 approved for 126.96.36.199 and 188.8.131.52, a=dveditz for release-drivers
checked into 1.8 branch
checked into 1.8.0 branch
Verified with 184.108.40.206 on Windows 220.127.116.11 on Mac trunk on Windows
We forgot editable menulists. Oops.
(In reply to comment #19) > We forgot editable menulists. Oops. Did this cause a regression? I'm not sure I follow your comment.