49120 - link properties not reflected until user interacts with the link

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[sean echevarria]

Link properties like onmouseover are not accessible from javascript until the
user has interacted with the link.

They should be fully reflected by the time that a onload handler is called.

Comment 1

[sean echevarria]

Attachment: testcase

Comment 2

[sean echevarria]

nominating nsbeta3

Comment 3

[sean echevarria]

cc tim (this breaks the beatnik music object wizard extensions)

Comment 4

[Nisheeth Ranjan]

Marking nsbeta3-.

Comment 5

[sean echevarria]

What does 'egk' in the whiteboard mean?

Comment 6

[ekrock's old account (dead)]

It means it's a bug that I'm quite concerned about having nsbeta3-ed and am 
marking for possible status review or exception fix status for RTM.

Comment 7

[sean echevarria]

This breaks link mouseovers on the Beatnik website.  The mouseovers are handled 
by a js script that we distribute to content developers - so problem is not 
limited to www.beatnik.com.

Rather than adding mouseover code by hand to every link, it is done dynamically 
via a sourced in js script to add sound effects, etc to links and form elements.

The documentation for the script is at:
http://www.beatnik.com/software/documentation/music_object/index.html
In the index frame, scroll down to the section labeled "Music Object 
Extensions" - the Wizards extension works with mouseover and mouseclick 
properties of links.

A more direct url to the documentation (less the frames):
http://www.beatnik.com/software/documentation/music_object/music-object-
reference/extensions/wizards/index.html

I previously wrote that the link properties should be fully reflected by the 
time that the document loads.  I think this can be refined: link properties 
should be fully reflected when they are accessed via js (even if the user has 
not interacted with them).

Comment 8

[ekrock's old account (dead)]

Increasing priority to P2 as this is high profile backward compatibility
(breaking JS 1.0 DOM0 and showing up on beatnik.com site). No workaround either.
This would qualify for a check-in through 9/21 with mozilla reviewers approval.
Can we find someone to fix this?

Comment 9

[joki (gone)]

Okay, I'll take this one.  We know the problem and have a plan for a fix.

Comment 10

[Johnny Stenback (:jst)]

*** Bug 46868 has been marked as a duplicate of this bug. ***

Comment 11

[Nisheeth Ranjan]

Marking nsbeta3+.  Tom knows how to fix this.

Comment 12

[Phil Peterson]

PDT agrees P2

Comment 13

[Nisheeth Ranjan]

This didn't make the beta 3 train.  Marking nsbeta3- and nominating for rtm.
Requesting an exception fix approval from PDT by marking this rtm+

Comment 14

[Phil Peterson]

PDT marking [rtm need info] until patch and code reviews are available.

Comment 15

[joki (gone)]

Okay, I've got the fix ready and tested for this.  Basically what I did is to 
add an CompileScriptEventListener call onto nsIEventListenerManager.  I then 
called into this in the JS Resolve methods on nsDocument and nsGenericElement.  
I also added it into the Get{event handler name} methods on nsGlobalWindow.  So 
if someone asks for a JS event listener, we force it to compile, if it exists.  
Otherwise we do nothing.

Comment 16

[joki (gone)]

Attachment: Proposed fix

Comment 17

[Nisheeth Ranjan]

Tom, please check this into the trunk after doing pre-checkin testing.  
Unfortunately, it'll be difficult to justify checking this fix into the rtm 
branch.

Marking rtm-.

Comment 18

[sean echevarria]

adding ns601 keyword

Comment 19

[Brendan Eich [:brendan]]

I don't think ns601 means "please get in for Netscape 6.01", but maybe ekrock 
can say more about this misnamed keyword.

I removed nsbeta3 and rtm, and added mozilla0.9.  joki, is this on the trunk 
yet?  I don't see reviewer-stamps in the bug; I have a few comments myself:

+    nsAutoString mPropName, mPrefix;
+    mPropName.Assign(NS_REINTERPRET_CAST(const PRUnichar*, 
JS_GetStringChars(JS_ValueToString(aContext, aID))));
+    if (mPropName.Length() > 2)
+      mPrefix.Assign(mPropName.GetUnicode(), 2);
+    if (mPrefix.EqualsWithConversion("on")) {

and similar code in nsGenericElement.cpp should not copy into two nsAutoStrings 
just to check for "on" (also, this particular case should not use the mPropName 
convention reserved for class members, but that variable's unnecessary anyway).

Instead, use a zero-string-copy test:

+    JSString *str = JSVAL_TO_STRING(aID);
+    if (JS_GetStringLength(str) > 2) {
+      const jschar *chars = JS_GetStringChars(str)));
+      if (chars[0] == 'o' && chars[1] == 'n') {

Does the new "replaceable" attribute in IDL help at all in shrinking the size of 
this patch?  It can be used to define properties lazily from resolve. Cc'ing jst 
and vidur for advice.

/be

Comment 20

[Johnny Stenback (:jst)]

*** Bug 35463 has been marked as a duplicate of this bug. ***

Comment 21

[Johnny Stenback (:jst)]

*** Bug 37584 has been marked as a duplicate of this bug. ***

Comment 22

[ekrock's old account (dead)]

Nom. nsbeta1. DOM correctness bug that's also actually affecting a real web site
and plug-in (Beatnik).

Comment 23

[joki (gone)]

I still have this fix from the last rtm cycle.  I'll get it updated and checked 
in.

Comment 24

[joki (gone)]

*** Bug 57367 has been marked as a duplicate of this bug. ***

Comment 25

[joki (gone)]

I've been talking with jst as I integrated this patch and we're going to change 
the code this lives in in the next week or two.  So instead of putting this in 
now and then rewriting it next week I'm going to move this out to moz0.9 and put 
in the fix with the new code.

Comment 26

[joki (gone)]

Attachment: Patch for 49120 and 53640, part 1

Comment 27

[joki (gone)]

Attachment: Patch for 49120 and 53640, part 2

Comment 28

[joki (gone)]

sr = jst

Comment 29

[joki (gone)]

r = peterv

Fixed.

Comment 30

[sean echevarria]

vrfy