333262 - In <inspector.xml>, strict warning "reference to undefined property event.originalTarget.nodeType", filling JS Console

Status: VERIFIED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Robert Kaiser]

When using DOM Inspector on a trunk build from today to debug some pref panel weirdness, I had my JS Console filled with identical warnings from DOMI code:

Warning: reference to undefined property event.originalTarget.nodeType
Source File: chrome://inspector/content/inspector.xml
Line: 455

And that's what I was told in #developers about this:
<NeilAway> KaiRo: yeah, that's smaug's fault
<NeilAway> KaiRo: some events now fire on windows, when they didn't use to
<NeilAway> KaiRo: and of course windows aren't nodes

A bugzilla search didn't turn up an existing bug (only bug 331926 comment #3 telling that it "existed before this patch"), so I think it's a good idea to have a new bug for that :)

Comment 1

[Alex Vincent [:WeirdAl]]

cc'ing smaug and marking dependency

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

So earlier focus and blur events which were dispatched to window had
document as their target, now the target is the window itself.
There are separate events dispatched to document as before.
Do we want to revert back the old behaviour?

I don't like having events dispatched to something which is not the same as the
event.originalTarget. For me all those cases are bugs (which unfortunately can't 
always be fixed because of backward compatibility)

Comment 3

[[e-mail]]

A quick look at the event handler in question suggests that it's only interested in focus events on elements so it should just be a case of tweaking the check.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Could someone tell how to reproduce the bug?
Or provide a patch - this should fix it: s/!event.originalTarget/!("nodeType" in event.originalTarget)/ ;)

Btw, according to http://dev.w3.org/cvsweb/~checkout~/2006/webapi/Window/publish/Window.html event handling with window object won't be specified, though that is just a draft.

Comment 5

[Boris Zbarsky [:bzbarsky]]

As a side note, could we just attach the blur/focus listeners in question to the document instead of the window?  And could someone post some LXR URIs to the listeners?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

sicking, has WepAPI WG discussed about Window and events? What should be the event target?

And Kairo, could you tell me how to reproduce this? I think I want to
make that small change to inspector.xml, but it'd be great to be able to test even
that patch.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We have not talked about this in the WebAPI group no.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: untested

Kairo, could you test this?

Comment 9

[Robert Kaiser]

Comment on attachment 218689 [details] [diff] [review]
untested

I guess one review request is enough :)

Actually, I can't reproduce the original problem any more in an un-patched build (perhaps I needed the broken locales panel to trigger it, not sure) and so I have a problem in testing this.
I also see no error messages in the console with the patch applied, in any case.

Comment 10

[Robert Kaiser]

Comment on attachment 218689 [details] [diff] [review]
untested

Clearing review flag. Sorry, I can't reprocude the problem an more, so I can't test if it gets fixed. Maybe something else fixed the problem, not sure...

Comment 11

[Alex Vincent [:WeirdAl]]

I can reproduce it consistently by launching DOM Inspector and trying to put focus on it.  Very painful with Venkman launched too.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102312 Minefield/3.0a9pre

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> Could someone tell how to reproduce the bug?

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/20070515 SeaMonkey/1.5a] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008012302 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Steps:
1. "Tools > Web Development > DOM Inspector".
1r. Get this error, twice.

> Or provide a patch - this should fix it: s/!event.originalTarget/!("nodeType"
> in event.originalTarget)/ ;)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008012302 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

FWIW, this (patch) fixes the error(s).

Now, the discussion seems open as to how best fix this issue...

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Who does DOMi reviews nowadays? If the patch fixes the problem, it IMO should be used. We don't want to special case too many events (load/unload is enough, and those ones are pretty impossible to change because of web compatibility).

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> Who does DOMi reviews nowadays? If the patch fixes the problem, it IMO should
> be used. We don't want to special case too many events (load/unload is enough,
> and those ones are pretty impossible to change because of web compatibility).
me, timeless, db48x
needs sr from neil

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: event.originalTarget instanceof Element

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 299569 [details] [diff] [review]
event.originalTarget instanceof Element

r=sdwilsh

Comment 17

[Damon Sicore (:damons)]

Comment on attachment 299569 [details] [diff] [review]
event.originalTarget instanceof Element

a1.9+=damons

Comment 18

[Serge Gautherie (:sgautherie)]

{{
2008-01-29 11:01	Olli.Pettay%helsinki.fi 	mozilla/extensions/inspector/resources/content/inspector.xml 	1.22
}}

Comment 19

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008013002 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

V.Fixed, per comment 12 steps.