-
Notifications
You must be signed in to change notification settings - Fork 144
Refactor events: a mixed bag, part 2 #1126
Comments
|
According to the spreadsheet, the reviewer for this ticket is @Elchi3 (remember to tag the reviewer when you've finished a bit of work) |
@Elchi3, you can take a look at these. I am in the process of creating the PR and will post a link here as soon as I'm done. |
For reference, I am now reviewing these. |
OK @irenesmith, I've reviewed the documents. There are a few bits in here that require changes in where you place things in BCD, so I'd hold off on the BCD until you've explored these. popstate, resize, storageI don't know why you've listed these events on the https://developer.mozilla.org/en-US/docs/Web/API/Document/defaultView page. These events are fired on the Window interface (and Element, in the case of resize), not the defaultView property; see https://html.spec.whatwg.org/#event-popstate So the pages and the "Events" listing need to be moved. This also means that the BCD will need updating (if you've created it), as will the compat macro calls. popstate(https://developer.mozilla.org/en-US/docs/Web/API/Document/defaultView/popstate_event) The title will need to be changed when it is moved to under the Window interface. "Browsers tend to handle the popstate event differently on page load. Chrome (prior to v34) and Safari always emit a popstate event on page load, but Firefox doesn't." - this should be represented in the BCD. On https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onpopstate, it would be nice if you included a proper Specification table in this page. On https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onpopstate, you should update the link to the event page so it points to the correct place, once you've moved it. resizeThe text in the page introduction on resizeobserver is a bit out of date, e.g. it mentions 2017. Can you update this? On https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onresize, you should update the link to the event page so it points to the correct place, once you've moved it. On https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onresize, the example could do with some rewording — it talks about resizing the browser window, whereas in actual fact the iframe needs to be resized for the event to fire. Ideally this would work better as an example hosted on github that can be linked to, but in the short term maybe you could just update the wording? storageThe SpecName call is incorrect — correct spec, but incorrect URL fragment and string. On https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onstorage, once you've corrected the position of the event page you should update the links to it that exist on this page. scroll and wheelThese look correctly placed. On https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onscroll and https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onwheel, update the links to the relevant events to point to the correct places. wheelThere is no Example on https://developer.mozilla.org/en-US/docs/Web/API/Element/wheel_event — simple examples of addEventListener/onwheel need adding. resourcebuffertimingfullThis has been put at https://developer.mozilla.org/en-US/docs/Web/API/Document/resourcetimingbufferfull_event, and looks correct as per the spec... ...in this case it would need to be added at https://developer.mozilla.org/en-US/docs/Web/API/Document#Events and https://developer.mozilla.org/en-US/docs/Web/API/Element#Events ...however, this is confusing, as onresourcetimingbufferfull is defined on Performance, not Document. I think it makes more sense to put the event page under Performance too? Although it seems to reckon it is fired on document/element? Weird. On the event page we do state in the table that the Interface is Performance, not document. Also on the event pages (same for https://developer.mozilla.org/en-US/docs/Web/API/Element/resourcetimingbufferfull_event and ):
search
For the event page itself:
selectLooks generally OK, but for a couple of things:
slotchangeGenerally OK; you just need to update the compat macro reference to api.HTMLSlotElement.slotchange_event timeout
|
I'm now working on making the changes listed above. XMLHttpRequest timeout event: all changes listed above have been made. |
HTMLSlotElement slotchange event: change listed above has now been made |
Element select event: changes listed above now made Element show event: I also fixed up this page. One major change is that I am pretty sure this is deprecated, so I marked it as such. It was present in the W3C HTML5 snapshot, but not in W3C HTML 5.1 snapshot or WHATWG HTML |
HTMLInputElement search event: changes listed above now all done |
Performance onresourcetimingbufferfull event: I backtracked on my original thoughts about this one. The spec language is hugely confusing about where this event is fired, and I've seen a couple of examples that contradict one another, but from testing it does appear that the event/handler arefound on the Performance object itself. I have therefore moved the event page to https://developer.mozilla.org/en-US/docs/Web/API/Performance/resourcetimingbufferfull_event And added a Events section here: https://developer.mozilla.org/en-US/docs/Web/API/Performance#Events |
Document/Element scroll event and Document/Element wheel event: changes listed above now all done. |
Window popstate event, Window storage event, Window resize event: changes listed above all now done. I ended up moving them all to Window (and nowhere else); the others are obviously Window, and resize is the really confusing one. resize only fires on Elements in old versions of IE; in modern Firefox/Chrome the onresize handler exists on elements, but no events are actually fired on elements; just window. |
Note: The search event already has BCD, so I don't need to do that one. |
BCD now written for all these events, see mdn/browser-compat-data#4008. @wbamberg you can probably check the pages now; I don't know if @Elchi3 has time to review the BCD. If you are both feeling too busy in the next few days, we could delegate it to someone else. |
@chrisdavidmills , thanks for doing all these! I have taken a look and decided to just fix the minor problems I could see. So these are all good. Except the note here: https://developer.mozilla.org/en-US/docs/Web/API/Window/popstate_event#Browser_compatibility which I think we should capture in the BCD. |
BCD review comments responded to. Thanks Will! |
I just merged the BCD so I think this one can be closed. Thanks for all your work on it Chris! |
This is a work item for #685.
Acceptance criteria
The text was updated successfully, but these errors were encountered: