Specify target of progress events #246

jugglinmike · 2019-04-17T17:33:04Z

The algorithm for "fire a progress event" requires a name and a target,
but only half of the 14 usages specify a target. This is ambiguous
because both the XMLHttpRequestEventTarget and the
XMLHttpRequestUpload support a "progress" event.

Consistently specify the intended target with each invocation. Also
modify a preposition in the algorithm's definition to match the
prevailing usage.

For those more familiar with the spec than I, the intended target may be clear in context. Still, it seems like the ambiguity may have allowed discrepancies amongst implementations. I'm wondering if resolving this will require a survey on what's currently deployed.

Preview | Diff

The algorithm for "fire a progress event" requires a name and a target, but only half of the 14 usages specify a target. This is ambiguous because both the `XMLHttpRequestEventTarget` and the `XMLHttpRequestUpload` support a "progress" event. Consistently specify the intended target with each invocation. Also modify a preposition in the algorithm's definition to match the prevailing usage.

annevk

Now that this is a thing, this should be rather easy to do. So either you want this or this's XMLHttpRequestUpload object. I think that should work in most places. See also #99.

annevk · 2019-04-18T11:57:26Z

xhr.bs

@@ -1884,7 +1886,7 @@ attributes must return the value they were initialized to.

 <h3 id=firing-events-using-the-progressevent-interface>Firing events using the {{ProgressEvent}} interface</h3>

-<p>To <dfn id=concept-event-fire-progress>fire a progress event</dfn> named <var>e</var> at
+<p>To <dfn id=concept-event-fire-progress>fire a progress event</dfn> named <var>e</var> on


The convention is "at", as established by fire an event.

Got it. I'll use "at" in this patch and fix the existing uses of "on" in a follow-up.

jugglinmike · 2019-04-18T16:59:08Z

So either you want this or this's XMLHttpRequestUpload object.

My understanding is that the appropriate target is the XMLHttpRequestEventTarget in some cases and XMLHttpRequestUpload in others. If that's right, then I don't know how to choose between this or this's XMLHttpRequestUpload object in each case.

annevk · 2019-04-18T17:48:32Z

A subclass of XMLHttpRequestEventTarget is this in most cases.

jugglinmike

A subclass of XMLHttpRequestEventTarget is this in most cases.

Understood. My confusion isn't so much about what this refers to--it's that I don't know which target is intended in each case. I've taken a first shot at this using a mix of inference and observation. Please take care to verify each as my guesswork is likely flawed.

Also, I'm referencing the term with the Bikeshed syntax, [=this=] so that it is cross-linked to WebIDL.

xhr.bs

jugglinmike · 2019-04-18T20:43:24Z

xhr.bs

@@ -957,7 +957,7 @@ method must run these steps:
        <a>state</a> changes.

       <li><p><a>Fire a progress event</a> named
-       <a event><code>progress</code></a> with <var>response</var>'s
+       <a event><code>progress</code></a> at [=this=] with <var>response</var>'s


The intended target isn't clear in context. Firefox, Chrome, Edge, and Safari all fire this event at the request object.

jugglinmike · 2019-04-18T20:44:23Z

xhr.bs

@@ -1039,7 +1039,7 @@ method must run these steps:
 <a for=body>total bytes</a>.

 <li><p>If the <a>synchronous flag</a> is unset, <a>fire a progress event</a> named
- <a event><code>progress</code></a> with <var>transmitted</var> and <var>length</var>.
+ <a event><code>progress</code></a> at [=this=] with <var>transmitted</var> and <var>length</var>.


The intended target isn't clear in context. Firefox, Chrome, Edge, and Safari all fire this event at the request object.

jugglinmike · 2019-04-18T20:44:39Z

xhr.bs

@@ -1048,9 +1048,11 @@ method must run these steps:
 <li><p><a>Fire an event</a> named <a event><code>readystatechange</code></a>.

 <li><p><a>Fire a progress event</a> named <a event><code>load</code></a>
+ at [=this=]


The intended target isn't clear in context. Firefox, Chrome, Edge, and Safari all fire this event at the request object.

jugglinmike · 2019-04-18T20:44:53Z

xhr.bs

 with <var>transmitted</var> and <var>length</var>.

 <li><p><a>Fire a progress event</a> named <a event><code>loadend</code></a>
+ at [=this=]'s {{XMLHttpRequestUpload}} object


The intended target isn't clear in context. Firefox, Chrome, Edge, and Safari all fire this event at the request upload object.

jugglinmike · 2019-04-18T20:54:12Z

xhr.bs

@@ -1122,10 +1124,10 @@ exception <var>exception</var> are:
  </ol>

 <li><p><a>Fire a progress event</a> named
- <var>event</var> with 0 and 0.
+ <var>event</var> at [=this=] with 0 and 0.


Since a corresponding event is fired at the request upload object in the previous step, I'm fairly certain the intended target here is the request object.

jugglinmike · 2019-04-18T20:54:27Z

xhr.bs


 <li><p><a>Fire a progress event</a> named
- <a event><code>loadend</code></a> with 0 and 0.
+ <a event><code>loadend</code></a> at [=this=] with 0 and 0.


Since a corresponding event is fired at the request upload object in the previous step, I'm fairly certain the intended target here is the request object.

annevk · 2019-04-23T14:37:09Z

I don't think this should be cross-linked. It adds too much noise.

annevk · 2019-04-23T14:39:27Z

Also, I think this will leave the specification in a confusing state as you're not changing all dispatch callers.

jugglinmike · 2019-04-23T17:21:37Z

I'm happy to help fix those parts, too, but my preference is to do so with a distinct commit that can be labeled as "editorial." I've added another fixup commit which will limit the first change to the normative specification of the algorithm's parameters. The following commit updates the other call sites to use this, and the commit after that updates the preposition.

GitHub doesn't recognize fixup commits, but I can squash those first few and force push the result when/if this patch is otherwise ready.

annevk · 2019-04-23T17:40:58Z

I'm not sure how this is not editorial if the other one would be?

jugglinmike · 2019-04-23T17:51:54Z

Prior to the commit titled "Specify target of progress events," the intended target could be interpreted as either the request object or its corresponding request upload object. I consider this change normative because it is resolving an ambiguity, and that could potentially conflict with an implementer's prior reading (and consequently, their implementation).

The following commits are editorial because they do not influence observable behavior.

annevk · 2019-04-23T17:53:30Z

Why would it not be ambiguous for other events though?

jugglinmike · 2019-04-23T18:14:56Z

I've been focused on "fire a progress event" because that's the text which brought me here. I've only just now realized that XHR's use of "fire an event" is likewise ambiguous.

jugglinmike · 2019-04-24T16:27:13Z

That highlights another misunderstanding in our prior discussion. When you wrote,

you're not changing all dispatch callers

I interpreted this to mean

you're not changing all instances of 'fire a progress event' to use the this keyword

Now I see that you meant,

you're not changing all instances of event dispatch to specify the target

I've pushed up another commit to specify the target for the callers of "fire an event." I believe it is this in all cases. I'm more confident about this change because all events are "readystatechange", and the request upload object does not have a readyState attribute.

Since this expands the scope of the patch, we'll need to update the commit message as well.

annevk

Thanks, this looks good apart from a single instance.

annevk · 2019-05-03T12:27:41Z

xhr.bs

 with <var>transmitted</var> and <var>length</var>.

 <li><p><a>Fire a progress event</a> named <a event><code>loadend</code></a>
+ at <b>this</b>'s {{XMLHttpRequestUpload}} object


This is wrong, this should be at this only.

annevk

Sorry for the delay here. Did you want to add yourself to the Acknowledgments section?

jugglinmike · 2019-05-11T02:07:37Z

Sure do :)

jugglinmike mentioned this pull request Apr 17, 2019

[xhr] Tolerate expected variation web-platform-tests/wpt#16372

Merged

annevk reviewed Apr 18, 2019

View reviewed changes

fixup! Specify target of progress events

b0f1e0e

fixup! Specify target of progress events

3900e65

jugglinmike commented Apr 18, 2019

View reviewed changes

jugglinmike added 3 commits April 23, 2019 13:07

fixup! Specify target of progress events

eb2ee75

Editorial: consistently use this keyword

9030ffc

Editorial: Correct preposition

3c0483c

jugglinmike changed the title ~~[do not merge] Specify target of progress events~~ Specify target of progress events Apr 23, 2019

fixup! Specify target of progress events

93add32

annevk reviewed May 3, 2019

View reviewed changes

fixup! Specify target of progress events

f78b4dd

annevk approved these changes May 10, 2019

View reviewed changes

fixup! Specify target of progress events

05b25a5

annevk merged commit fef8ebc into whatwg:master May 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify target of progress events #246

Specify target of progress events #246

jugglinmike commented Apr 17, 2019 •

edited by pr-preview bot

annevk left a comment

annevk Apr 18, 2019

jugglinmike Apr 18, 2019

jugglinmike commented Apr 18, 2019

annevk commented Apr 18, 2019

jugglinmike left a comment

jugglinmike Apr 18, 2019

jugglinmike Apr 18, 2019

jugglinmike Apr 18, 2019

jugglinmike Apr 18, 2019

jugglinmike Apr 18, 2019

jugglinmike Apr 18, 2019

annevk commented Apr 23, 2019

annevk commented Apr 23, 2019

jugglinmike commented Apr 23, 2019

annevk commented Apr 23, 2019

jugglinmike commented Apr 23, 2019

annevk commented Apr 23, 2019

jugglinmike commented Apr 23, 2019

jugglinmike commented Apr 24, 2019

annevk left a comment

annevk May 3, 2019

jugglinmike May 3, 2019

annevk left a comment

jugglinmike commented May 11, 2019

Specify target of progress events #246

Specify target of progress events #246

Conversation

jugglinmike commented Apr 17, 2019 • edited by pr-preview bot

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jugglinmike commented Apr 18, 2019

annevk commented Apr 18, 2019

jugglinmike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Apr 23, 2019

annevk commented Apr 23, 2019

jugglinmike commented Apr 23, 2019

annevk commented Apr 23, 2019

jugglinmike commented Apr 23, 2019

annevk commented Apr 23, 2019

jugglinmike commented Apr 23, 2019

jugglinmike commented Apr 24, 2019

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

jugglinmike commented May 11, 2019

jugglinmike commented Apr 17, 2019 •

edited by pr-preview bot