Bug 28260

Summary:

onhashchange property cannot be set from javascript

Product:

WebKit

Reporter:

Brady Eidson <beidson>

Component:

Page Loading

Assignee:

Brady Eidson <beidson>

Status:

RESOLVED FIXED

Severity:

Normal

Keywords:

InRadar

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Fix + layout test	eric: review+, eric: commit-queue-

Brady Eidson

Reported 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...

Attachments
Fix + layout test (10.73 KB, patch) 2009-08-13 09:51 PDT, Brady Eidson	eric: review+ eric: commit-queue-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Brady Eidson

Comment 1 2009-08-13 09:51:27 PDT

Created attachment 34752 [details] Fix + layout test

Eric Seidel (no email)

Comment 2 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.

Eric Seidel (no email)

Comment 3 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.

Brady Eidson

Comment 4 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!

Brady Eidson

Comment 5 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.

Note You need to log in before you can comment on or make changes to this bug.