search

whatwg / xhr

XMLHttpRequest Standard
https://xhr.spec.whatwg.org/
Other
314 stars 131 forks source link

Specify target of progress events #246

Closed jugglinmike closed 5 years ago

jugglinmike commented 5 years ago

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

jugglinmike commented 5 years ago

So either you want <b>this</b> or <b>this</b>'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 <b>this</b> or <b>this</b>'s XMLHttpRequestUpload object in each case.

annevk commented 5 years ago

A subclass of XMLHttpRequestEventTarget is <b>this</b> in most cases.

annevk commented 5 years ago

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

annevk commented 5 years ago

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

jugglinmike commented 5 years ago

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 commented 5 years ago

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

jugglinmike commented 5 years ago

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 commented 5 years ago

Why would it not be ambiguous for other events though?

jugglinmike commented 5 years ago

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 commented 5 years ago

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.

jugglinmike commented 5 years ago

Sure do :)