Summary: | Error event in <script> element shouldn't bubble | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yury Semikhatsky <yurys> | ||||||||||
Component: | DOM | Assignee: | Yury Semikhatsky <yurys> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, ap, eric, ggaren, ian, Ms2ger, pfeldman, rniwa, sullivan, tonyg, webkit.review.bot, zimmermann | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 8519, 58981 | ||||||||||||
Attachments: |
|
Description
Yury Semikhatsky
2010-12-14 09:38:11 PST
fast/events/onerror-bubbling.html was introduced in http://trac.webkit.org/changeset/14007 and it's not clear why the event was expected to bubble for scripts. Created attachment 76543 [details]
Patch
> it's not clear why the event was expected to bubble for scripts.
ChangeLog says: "The DOM spec says the event should bubble, and that's how it works in Firefox." Is that incorrect? Did you test IE and Firefox?
(In reply to comment #3) > > it's not clear why the event was expected to bubble for scripts. > > ChangeLog says: "The DOM spec says the event should bubble, and that's how it works in Firefox." Is that incorrect? I think the author is talking about this section http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-eventgroupings-htmlevents which contains the following sentence: " error The error event occurs when an image does not load properly or when an error occurs during script execution. This event is valid for OBJECT elements, BODY elements, and FRAMESET element. Bubbles: Yes Cancelable: No Context Info: None " Note that SCRIPT element is not mentioned while HTML5 spec explicitly describes its behavior. > Did you test IE and Firefox? Both old and new version of the test fail in IE9 and FF3.6.12 They don't seem to invoke onerror handler on the script elements. Is it possible to make a test that works in IE and Firefox? A regression test that only works in WebKit isn't very future proof. If no other browser ever invokes onerror on a script element, them maybe HTML5 should be changed to match WebKit as the only implementation. (In reply to comment #5) > Is it possible to make a test that works in IE and Firefox? A regression test that only works in WebKit isn't very future proof. > I'm not sure I'm following you here. The test doesn't pass in IE/FF since they don't support script.onerror, in their case we could produce a result that would state that the feature is not implemented in these browsers. Can you elaborate on what you mean by "making a test that works in IE and Firefox"? > If no other browser ever invokes onerror on a script element, them maybe HTML5 should be changed to match WebKit as the only implementation. OK - if they don't support script.onerror at all, then we should check with HTML5 working group about making its behavior match WebKit. (In reply to comment #7) > OK - if they don't support script.onerror at all, then we should check with HTML5 working group about making its behavior match WebKit. One more problem with that is that according to the spec window.onerror handler is a function accepting three arguments(message, filename, line number), "error" event listeners on window will exect ErrorEvent as a parameter while the error event fired for <script> elements is a simple event, i.e. it's an instance of Event and as such doesn't contain additional data such as message, url and line number which would have to be provided to window.onerror should script.onerror bubble. At the same time script.onerror is supposed to be called on script load errors(for example a DNS error, or an HTTP 404 error) and there is no additional file name/line number in that case. window.onerror is supported in IE and Firefox and we hardly can change its specification. Adding Ian who may know why it's been decided to make <script onerror> a simple event. I'm also wondering about SVGScriptElement's error event. It doesn't bubble to document element's onerror in Opera, but does in WebKit. I don't know what SVG spec says, but if we can HTML and SVG <script> have compatible semantics, that would probably be good. I think the behavior of the error event of SVGScriptElement should be consistent with that of HTMLScriptElement. Description of the SVG script element is quite vague (http://www.w3.org/TR/SVG11/script.html#ScriptElement):"A ‘script’ element is equivalent to the ‘script’ element in HTML" (In reply to comment #10) > I'm also wondering about SVGScriptElement's error event. It doesn't bubble to document element's onerror in Opera, but does in WebKit. I don't know what SVG spec says, but if we can HTML and SVG <script> have compatible semantics, that would probably be good. Could you add a window.onerror = function(msg, file, line) { Fail(); } to the test? As you pointed out, load events bubbling up to window.onerror is the real problem with WebKit's current behavior. Based on behavior of all the other major UAs, I suspect that web devs would be surprised to catch a loading error here as opposed to a scripting error. Nor is it a sane API for the arguments to change. For this reason, I also think the spec is intentional about specifying no bubbling for script loading errors. Regarding SVGScriptElement, I agree it makes sense to make it match HTMLScriptElement in this regard. In fact, it might be nice if createScriptErrorEvent() were moved to ScriptElement so that it could be shared by SVGScriptElement, HTMLScriptElement and HTMLScriptRunner. AP, unless you still see issues, I'm happy to review this change. Created attachment 90209 [details]
Patch
(In reply to comment #12) > Could you add a window.onerror = function(msg, file, line) { Fail(); } to the test? Done. The test looks good. What about the bit about SVGScriptElement and possible refactoring? Created attachment 90326 [details]
Patch
(In reply to comment #15) > The test looks good. What about the bit about SVGScriptElement and possible refactoring? Done. ScriptElement::dispatchErrorEvent is not virtual any more and has common implementation for both SVG and HTML. The error event doesn't bubble. Comment on attachment 90326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90326&action=review > Source/WebCore/svg/SVGScriptElement.cpp:57 > + else if (attr->name() == HTMLNames::onerrorAttr) Adding support for the onerror attribute on SVG scripts should be called out in the ChangeLog. Created attachment 90328 [details]
Patch for landing
Fixed ChangeLog entry as Tony suggested.
Committed r84357: <http://trac.webkit.org/changeset/84357> http://trac.webkit.org/changeset/84357 might have broken Qt Linux Release The following tests are not passing: http/tests/misc/unloadable-script.html |