search

mdn / sprints

Archived: MDN Web Docs issues are tracked in the content repository.
https://github.com/mdn/content
Creative Commons Zero v1.0 Universal
150 stars 143 forks source link

Refactor progress, composition, focus events #1101

Closed wbamberg closed 5 years ago

wbamberg commented 5 years ago

This is a work item for #685.

progress:

focus:

composition

AC:

wbamberg commented 5 years ago

@chrisdavidmills I am as usual confused about the proper targets for the progress events. I'm pretty sure XMLHttpRequest is one target, so I've migrated them under there: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#Events

It seems as though they are also on FileReader (see https://searchfox.org/mozilla-central/search?q=ProgressEvent&path=).

The docs pages themselves suggest they are also on image and video elements, but although I can get events on those elements with names like loadstart, load, progress, they don't seem to be actual ProgressEvent objects (they don't have the loaded or total properties). Do you know what the story is here?

chrisdavidmills commented 5 years ago

@wbamberg you sure do pick the fun ones ;-)

So, the FIle API definitely uses proper ProgressEvents for event objects: https://www.w3.org/TR/FileAPI/#event-summary

As for images and video, is it not just that they inherit from HTMLElement: https://html.spec.whatwg.org/multipage/dom.html#htmlelement

Which implements the GlobalEventHandlers mixin: https://html.spec.whatwg.org/multipage/webappapis.html#globaleventhandlers

?

wbamberg commented 5 years ago

As for images and video, is it not just that they inherit from HTMLElement: https://html.spec.whatwg.org/multipage/dom.html#htmlelement

Which implements the GlobalEventHandlers mixin: https://html.spec.whatwg.org/multipage/webappapis.html#globaleventhandlers

Oh thank you, that does make sense (I think). Except that these are just Event objects, so the documentation is consistently wrong here (as it suggests that listening to, say progress on a video will get you a ProgressEvent).

But also! Your link points to https://html.spec.whatwg.org/multipage/media.html#event-media-progress, which seems to be telling me that these events are fired only on media elements (https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement, presumably) - and AFAIK this does not include image elements, so the existing docs are wrong there, too?

So it seems like what we should do here is: write pages for the relevant events under https://html.spec.whatwg.org/multipage/media.html#event-media-progress (specifically, I guess, loadstart, progress, abort, error) and hang them off HTMLMediaElement?

wbamberg commented 5 years ago

I've added:

chrisdavidmills commented 5 years ago

Good approach Will — I think this works well, and I really like the live examples.

I've looked through the pages and they generally seem pretty decent. the FileReader events section is missing the opening paragraph about using addEventListener, and I wonder whether the live examples need a note to say that you could also use on... handler properties instead of addEventListener()?

wbamberg commented 5 years ago

@chrisdavidmills , I reckon these pages are ready for a look.

progress events

This is as noted in https://github.com/mdn/sprints/issues/1101#issuecomment-470346536. I added the paragraph in FileReader#Events as you asked, but didn't add a note to the live samples - I could, but it seemed like hammering the point.

focus events

As far as I can tell Window is not a target for focusin or focusout, contra the docs.

composition events

Interestingly these do not seem to have a corresponding on- property. I managed to get a live sample but I don't know how to trigger it except on macOS. I'd be happy to make it multiplatform but I don't know how. We could use a guide to working with input method editors for this stuff, as they are hard to understand without that context.

chrisdavidmills commented 5 years ago

Hi @wbamberg ! Some feedback for you (these largely look really good):

FileReader events

Element focus events

Window focus events

Composition events

wbamberg commented 5 years ago

Thanks Chris.

FileReader events

  • The pages for onloadend/onloadstart/onprogress seem to be missing.

I'm not proposing to add these pages where they don't exist as I feel it's out of scope for refactoring the existing event ref pages. I can if you like though, but of course it will make everything take longer.

Element focus events

  • The pages for onfocusin/onfocusout seem to be missing

Yup, as above.

  • These still have the old-style compat tables. I'm assuming you are waiting to submit the BCD before updating them?

Yes.

  • The event pages for these (and the Window events) seem to have two extra columns in the blue boxes — "Sync/Async" and "Composed" — which I've not seen before. How many event pages have you come across that have these? Should we add these for other event pages; I guess you're keeping them because it is not a good idea to arbitrarily delete information that might be useful, which I think is a good call for now. But we'll need to think about this for the future maintenance work.

Yes, basically this (especially "Composed" - although we ought to explain somewhere that that means, since it is very non-obvious to me) seemed useful, and I don't think it's a good idea removing potentially useful entries without having thought about what this box should contain.

I don't really want to go back adding these rows to all the other tables at this stage. In general I think it's not realistic or a sensible use of our time to expect this degree of consistency out of the Wiki. If we want to keep these boxes, and we want a high degree of consistency, then in the medium term one approach could be to keep the data in mdn/data and build it using a macro. Long term we should fold it into an overall structured content plan.

Window focus events

  • In the window.focus page you still mention window.focusin, even though it doesn't exist.

Oops. Removed.

Composition events

  • The example is is a difficult one. I think composition events might be easier to fire on mobile browsers, e.g. through an IME, but I'm no exxpert here. If we did want to write something more cross-platform, it might be worth asking one of the engineers in this area, like Masayuki.

We could ask. I don't have contacts there though. I don't think we should block this work for that.

  • I struggled to get these events firing even on Mac. I think you really mean Opt when you say Alt? Maybe say Opt/Alt, as I'm sure a large number of mac folk know it as Opt. Also, maybe write in some more specific insrtructions, e.g. focus the input element. then press Opt/Alt + `, then press a, or some other printable key (most seem to work?).

Thanks, I've tried improving the instructions. I didn't say "some other printable key" since I think giving very specific instructions is likely to be more helpful.

chrisdavidmills commented 5 years ago

I'm not proposing to add these pages where they don't exist as I feel it's out of scope for refactoring the existing event ref pages. I can if you like though, but of course it will make everything take longer.

Nah, that's fine. As we agreed in that other thread with @a2sheppy , we are gonna leave creating new pages for now, to make the scope of work more manageable.

I don't really want to go back adding these rows to all the other tables at this stage. In general I think it's not realistic or a sensible use of our time to expect this degree of consistency out of the Wiki. If we want to keep these boxes, and we want a high degree of consistency, then in the medium term one approach could be to keep the data in mdn/data and build it using a macro. Long term we should fold it into an overall structured content plan.

That's perfectly fine, and makes sense.

We could ask. I don't have contacts there though. I don't think we should block this work for that.

I'm in favour of leaving this for now, as we have enough to do already. This could be part of a future maintenance run.

Thanks, I've tried improving the instructions. I didn't say "some other printable key" since I think giving very specific instructions is likely to be more helpful.

This is much better. I just increased the height of the live examples to avoid a vertical scrollbar on Firefox; I'm happy with this now.

So I think one looks done. Happy to close if you are. Oh, wait, do we have BCD in place for these?

wbamberg commented 5 years ago

do we have BCD in place for these?

I have a PR for it: https://github.com/mdn/browser-compat-data/pull/3585. It would I guess be great to get it reviewed before tomorrow's push. If you don't have time I could ask someone else though.

wbamberg commented 5 years ago

I forgot to update BCD for focus events. I have just filed https://github.com/mdn/browser-compat-data/pull/3650 for that.

wbamberg commented 5 years ago

BCD is updated and deployed, pages are refreshed. Closing.