Bug 28260

Summary: onhashchange property cannot be set from javascript
Product: WebKit Reporter: Brady Eidson <beidson>
Component: Page LoadingAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal Keywords: InRadar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Fix + layout test eric: review+, eric: commit-queue-

Description Brady Eidson 2009-08-13 09:22:34 PDT
onhashchange property cannot be set from javascript.

window.onhashchange = eventHandler;
...and...
document.body.onhashchange = eventHandler;
...should both work.

In radar as <rdar://problem/7138591>

Patch is forthcoming...
Comment 1 Brady Eidson 2009-08-13 09:51:27 PDT
Created attachment 34752 [details]
Fix + layout test
Comment 2 Eric Seidel (no email) 2009-08-13 10:04:32 PDT
Comment on attachment 34752 [details]
Fix + layout test

I'm confused by your test case.

You never clear the previous listeners, I would expect them to be called again for every new change.  Except from the code it looks like there is only one listener, and it's on the window object.  The test doesn't make that very clear.  If there is only one listener ever it seems like we should explicitly check for that, or check that body.onhashchange == window.onhashchange, etc. after setting.  In fact, we don't even really need to test that it gets called, only that if you set one, they're all equal, and that if any of them are set it gets called (although testing each as you have is fine too).

Is the hash supposed to be the new value or the old value during the callback?  Seems we should test that.

The change looks fine, but I think the test case could/should be made more clear.
Comment 3 Eric Seidel (no email) 2009-08-13 10:05:20 PDT
Comment on attachment 34752 [details]
Fix + layout test

Actually, I certainly don't need to see this again.  so here's your r+.  But I do hope you make some of the above mentioned test improvements when landing.
Comment 4 Brady Eidson 2009-08-13 14:19:44 PDT
window.onhashchange, body.onhashchange, and <body onhashchange=""> all refer to the exact same entity.  This is why you don't need to "clear" it - each stage of the test overwrites the previous value.

Checking equality might be an interesting addition to the test, but actually testing the function being called seems like it covers me code, as well.

In practice the hash value at the time of event delivery is irrelevant since:
1 - The hashchange event is defined to be asynchronous.
2 - The event itself has no knowledge of the old or new value - it's just a simple event.

I'll add the equality checks to the test before landing.  Thanks!
Comment 5 Brady Eidson 2009-08-13 14:36:05 PDT
Sending        LayoutTests/ChangeLog
Sending        LayoutTests/fast/dom/Window/window-properties-expected.txt
Adding         LayoutTests/fast/loader/onhashchange-attribute-listeners-expected.txt
Adding         LayoutTests/fast/loader/onhashchange-attribute-listeners.html
Sending        WebCore/ChangeLog
Sending        WebCore/html/HTMLBodyElement.cpp
Sending        WebCore/html/HTMLBodyElement.h
Sending        WebCore/html/HTMLBodyElement.idl
Sending        WebCore/html/HTMLFrameSetElement.cpp
Sending        WebCore/html/HTMLFrameSetElement.h
Sending        WebCore/html/HTMLFrameSetElement.idl
Sending        WebCore/page/DOMWindow.cpp
Sending        WebCore/page/DOMWindow.h
Sending        WebCore/page/DOMWindow.idl
Transmitting file data ..............
Committed revision 47233.