Events in FileAPI http://dev.w3.org/2006/webapi/FileAPI/#events specify that the events are NOT cancelable and the value in ProgressEvent must be false. Currently these is no provision to set canBubble and cancelable while creating ProgressEvent.
Created attachment 197591 [details] Patch
FYI - changed the assignee and status of this bug.
Should progress events ever be cancelable? The progress events spec says something I can't easily decipher: "Throughout the web platform the error, abort, and load event types have their bubbles and cancelable attributes initialized to false, so it is suggested that for consistency all events using the ProgressEvent interface do the same."
It is up to the specification dispatching the event to decide that, basically.
Comment on attachment 197591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197591&action=review > Source/WebCore/ChangeLog:11 > + Reviewed by NOBODY (OOPS!). "Reviewed by" line usually comes after the bug URL. > LayoutTests/fast/files/file-reader-event-listener.html:31 > + if (event.bubbles) { You should probably use shouldBeFalse() instead. > LayoutTests/fast/files/file-reader-event-listener.html:35 > + if (event.cancelable) { Ditto.
(In reply to comment #4) > It is up to the specification dispatching the event to decide that, basically. Going by ProgressEvent specifications, default value of "cancelable" for error, abort and load events should be false. So, is it fine if I set that to false in ProgressEvent.h?
(In reply to comment #6) > (In reply to comment #4) > > It is up to the specification dispatching the event to decide that, basically. > > Going by ProgressEvent specifications, default value of "cancelable" for error, abort and load events should be false. So, is it fine if I set that to false in ProgressEvent.h? Based on the spec and what Anne said, I personally think it is a good idea to set cancelable argument's default value to false, instead of true for ProgressEvent.
Created attachment 198019 [details] Patch
Comment on attachment 198019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198019&action=review > Source/WebCore/ChangeLog:3 > + ProgressEvent should provide provision to receive canBubble and cancelable values. Please update ChangeLog to match current bug title. > Source/WebCore/dom/ProgressEvent.cpp:56 > +ProgressEvent::ProgressEvent(const AtomicString& type, bool lengthComputable, unsigned long long loaded, unsigned long long total, bool canBubble, bool cancelable) > + : Event(type, canBubble, cancelable) If we never need other values, why make the constructor slower by making this configurable? > LayoutTests/fast/files/file-reader-event-listener.html:6 > +<script src="../js/resources/js-test-pre.js"></script> Please use the rest of js-test functionality here (window.jsTestIsAsync, finishJSTest()). testRunner.dumpAsText() is the default when using js-test.
(In reply to comment #9) > (From update of attachment 198019 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=198019&action=review > > > Source/WebCore/ChangeLog:3 > > + ProgressEvent should provide provision to receive canBubble and cancelable values. > > Please update ChangeLog to match current bug title. I will update this in the next patch. > > > Source/WebCore/dom/ProgressEvent.cpp:56 > > +ProgressEvent::ProgressEvent(const AtomicString& type, bool lengthComputable, unsigned long long loaded, unsigned long long total, bool canBubble, bool cancelable) > > + : Event(type, canBubble, cancelable) > > If we never need other values, why make the constructor slower by making this configurable? I checked other specifications which are using ProgressEvent and found that in XMLHTTPRequest, http://www.w3.org/TR/XMLHttpRequest/#garbage-collection, all events must be cancelable. Here, except for readystatechange, all are ProgressEvents. Maybe, we should retain the code as in first patch, along with setting default value while declaration, in order to support XMLHttpRequest. Let me know your suggestion. > > > LayoutTests/fast/files/file-reader-event-listener.html:6 > > +<script src="../js/resources/js-test-pre.js"></script> > > Please use the rest of js-test functionality here (window.jsTestIsAsync, finishJSTest()). testRunner.dumpAsText() is the default when using js-test. I will update this in the next patch.
> in XMLHTTPRequest, http://www.w3.org/TR/XMLHttpRequest/#garbage-collection, all events must be cancelable. I could not easily find this in the spec. Could you please quote it? Does XHR 2 say the same? If this patch used to break XHR, we should add a test tat would have caught that.
(In reply to comment #11) > > in XMLHTTPRequest, http://www.w3.org/TR/XMLHttpRequest/#garbage-collection, all events must be cancelable. > > I could not easily find this in the spec. Could you please quote it? Does XHR 2 say the same? > > If this patch used to break XHR, we should add a test tat would have caught that. There is a layout test http/tests/xmlhttprequest/infoOnProgressEvent.html which checks if event.cancelable is true. Strangely, it din't fail on efl port(not blocked by TestExpectations either). But Mac bot seems to have caught the error. Specifications doesn't mention anything specific to cancelable in XHR and maybe my interpretation was wrong about canceling fetch algorithm and making ProgressEvents of XHR cancelable.
Created attachment 198036 [details] Patch All comments are included in the patch. infoOnProgressEvent-expected.txt is modified to receive corrected state of Cancelable.
FYI, none of the events in http://xhr.spec.whatwg.org/ are cancelable. preventDefault() certainly never had an effect.
Comment on attachment 198036 [details] Patch Attachment 198036 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/10265 New failing tests: fast/xmlhttprequest/xmlhttprequest-get.xhtml http/tests/ssl/ping-with-unsafe-redirect.html
Created attachment 198045 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.2
Comment on attachment 198036 [details] Patch Attachment 198036 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/42423 New failing tests: fast/xmlhttprequest/xmlhttprequest-get.xhtml
Created attachment 198171 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Created attachment 198221 [details] Patch Failed test expectation files are updated. Bug title updated to be more generic.
Created attachment 198222 [details] Patch
Comment on attachment 198222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198222&action=review > Source/WebCore/ChangeLog:10 > + Spec: https://dvcs.w3.org/hg/progress/raw-file/tip/Overview.html#suggested-names-for-events-using-the-progressevent-interface Please refer to a specific version of the spec. > Source/WebCore/ChangeLog:12 > + No new tests. file-reader-event-listener.html and infoOnProgressEvent-expected.txt are updated. It appears to me that we also need to add tests to ensure these events are indeed NOT cancelable (i.e. canceling event shouldn’t have any effect). > LayoutTests/ChangeLog:9 > + file-reader-event-listener.html, xmlhttprequest-get-expected.txt and infoOnProgressEvent-expected.txt are updated. > + Did we used to allow these events to be cancelled? If so, how important is it to maintain the backward compatibility?
(In reply to comment #21) > (From update of attachment 198222 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=198222&action=review > > > Source/WebCore/ChangeLog:10 > > + Spec: https://dvcs.w3.org/hg/progress/raw-file/tip/Overview.html#suggested-names-for-events-using-the-progressevent-interface ok. I will update this in the next patch. > > Please refer to a specific version of the spec. > > > Source/WebCore/ChangeLog:12 > > + No new tests. file-reader-event-listener.html and infoOnProgressEvent-expected.txt are updated. > > It appears to me that we also need to add tests to ensure these events are indeed NOT cancelable (i.e. canceling event shouldn’t have any effect). The test cases are updated to check 'cancelable' value. I feel that covers all possible scenarios. > > > LayoutTests/ChangeLog:9 > > + file-reader-event-listener.html, xmlhttprequest-get-expected.txt and infoOnProgressEvent-expected.txt are updated. > > + > > Did we used to allow these events to be cancelled? If so, how important is it to maintain the backward compatibility? ProgressEvent spec released in April 2007(http://www.w3.org/TR/2007/WD-progress-events-20070419/) says "These events must not bubble, and must be cancelable.". Around the same time, these test cases too were added. After that, 5 more versions were released starting from October 2007(http://www.w3.org/TR/2007/WD-progress-events-20071023/) and all of them recommand to set 'cancelable' parameter to FALSE. However, the test cases were not updated following the spec changes. ProgressEvent specification hasn't changed 'cancelable' default value for a long time now and hence backward compatibility shouldn't be an issue, IMO.
(In reply to comment #22) > (In reply to comment #21) > > (From update of attachment 198222 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=198222&action=review > > > > > Source/WebCore/ChangeLog:10 > > > + Spec: https://dvcs.w3.org/hg/progress/raw-file/tip/Overview.html#suggested-names-for-events-using-the-progressevent-interface > > ok. I will update this in the next patch. > > > > > Please refer to a specific version of the spec. > > > > > Source/WebCore/ChangeLog:12 > > > + No new tests. file-reader-event-listener.html and infoOnProgressEvent-expected.txt are updated. > > > > It appears to me that we also need to add tests to ensure these events are indeed NOT cancelable (i.e. canceling event shouldn’t have any effect). > > The test cases are updated to check 'cancelable' value. I feel that covers all possible scenarios. It just tells us that the 'cancelable' returns the right value. It doesn't tell us that the event is not cancelable.
Created attachment 203310 [details] Patch event.preventDefault() is called to update 'canceled flag' of the event after 'cancelable' is set to FALSE.
Comment on attachment 203310 [details] Patch Clearing flags on attachment: 203310 Committed r150949: <http://trac.webkit.org/changeset/150949>
All reviewed patches have been landed. Closing bug.