Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add link to cancel event to HTMLInputElement page #30946

Closed
wants to merge 2 commits into from
Closed

Conversation

Cipscis
Copy link

@Cipscis Cipscis commented Dec 12, 2023

Description

This PR adds a link to the "cancel" event that can be used with HTMLInputElement to the HTMLInputElement page.

Motivation

The "cancel" event lives with the HTMLElement interface, but it is only used with <dialog> elements and <input type="file"> elements. It is currently linked to on the HTMLElement page and the <input type="file"> page, but these aren't necessarily the most obvious places to find it.

Because it's a JavaScript event, I think there should be a link to it on the HTMLInputElement page.

Additional details

Related issues and pull requests

#31014

@Cipscis Cipscis requested a review from a team as a code owner December 12, 2023 08:12
@Cipscis Cipscis requested review from wbamberg and removed request for a team December 12, 2023 08:12
@github-actions github-actions bot added the Content:WebAPI Web API docs label Dec 12, 2023
Copy link
Contributor

github-actions bot commented Dec 12, 2023

Preview URLs

Flaws (45)

URL: /en-US/docs/Web/API/HTMLInputElement
Title: HTMLInputElement
Flaw count: 45

  • macros:
    • /en-US/docs/Web/API/HTMLInputElement/align does not exist
    • /en-US/docs/Web/API/HTMLInputElement/autocapitalize does not exist
    • /en-US/docs/Web/API/HTMLInputElement/defaultValue does not exist
    • /en-US/docs/Web/API/HTMLInputElement/dirName does not exist
    • /en-US/docs/Web/API/HTMLInputElement/inputmode does not exist
    • and 40 more flaws omitted

(comment last updated: 2023-12-15 06:32:58)

@hamishwillee
Copy link
Collaborator

This fixes mdn/mdn#490, which I raised recently, but not issue 490 on mdn/content so I'm not sure how to link them to resolve the issue automatically

Fixes mdn/mdn#490

That should work. FYI only, best to just create MDN issues related to content into this repo.

@hamishwillee
Copy link
Collaborator

@wbamberg @teoli2003 Unlike methods and properties, the interface template for Events does not refer to parent interfaces for inherited events: https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Page_structures/Page_types/API_reference_page_template#events

Does that mean that the events listed in an interface are "the full set" that can be applied to the interface? If so, this PR is absolutely correct. If not, how do we deal with this case?

As an aside, the HTML tag page should have a table of Events like this https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video#events. The https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input does not have one. If this was created, should it list all events (I would assume so).

@Cipscis
Copy link
Author

Cipscis commented Dec 15, 2023

Fixes #31014

@wbamberg
Copy link
Collaborator

Thanks for the ping, Hamish. I don't think we should do this. I think we should list events on the interface on which they are defined, like we do for methods and properties. What would be the rationale for doing something different for events?

Element and HTMLElement both define lots of events, are we going to duplicate all of them for every subclass (of which there are many)?

@wbamberg @teoli2003 Unlike methods and properties, the interface template for Events does not refer to parent interfaces for inherited events: https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Page_structures/Page_types/API_reference_page_template#events

I'd guess this is an oversight, and the template should be updated.

I appreciate that it's a pain having to check up the hierarchy for these things, but it's a much more maintainable approach, and it probably more usable overall than having huge lists of duplicated events.

@hamishwillee
Copy link
Collaborator

@wbamberg I tend to agree. Though this appears to be inconsistent with what is supposed to be done in the HTML element documentation - such as https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video#events
That's reasonable because from a user-facing point of view an element like input has no derivation.

Anyway, it would not be inconsistent to cross link to the corresponding information in the HTML tag for more information about the events. OR we have to reconsider how we do that too.

@Cipscis
Copy link
Author

Cipscis commented Dec 15, 2023

Not sure if it might help for me to explain why I wanted to add this documentation. I've wanted this event to exist for a few years, but didn't know it exists. Just recently, I ran into another use case where I wanted the functionality is provides.

My first port of call for documentation is MDN, so that's where I went to see if there was a way to detect when a file input's prompt is dismissed without selecting anything.

I now know that this event is documented on the HTMLElement and <input type="file"> pages, and there is a row for it in the compatability table for the HTMLInputElement page, but unfortunately I didn't find those earlier. Because the only place where I would work with this event directly is when I already have an HTMLInputElement, that's the page I checked for information.

I only discovered the event existed when I went to suggest it as an addition to the HTML standard, and found the issue where it had been suggested back in 2021.

I also think it would be consistent with other documentation to mention this event on the HTMLInputElement page.

Though it exists on the HTMLElement interface, "cancel" events are only fired against HTMLInputElement and HTMLDialogElement elements. The "cancel" event is already documented on the HTMLDialogElement page, which links to its primary documentation under HTMLElement.

The documentation on the HTMLDialogElement page doesn't mention that the event is really fired on the HTMLElement interface. I included that text in this PR because it seemed sensible to match the example of the "input" event that also exists on the HTMLElement interface but is included in the documentation for HTMLInputElement.

@teoli2003
Copy link
Contributor

I'd guess this is an oversight, and the template should be updated.

Yes, it is. We should add a sentence here.

@skyclouds2001
Copy link
Contributor

@wbamberg I tend to agree. Though this appears to be inconsistent with what is supposed to be done in the HTML element documentation - such as https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video#events That's reasonable because from a user-facing point of view an element like input has no derivation.

Anyway, it would not be inconsistent to cross link to the corresponding information in the HTML tag for more information about the events. OR we have to reconsider how we do that too.

For example, adding a sentence in the template just before the events list like:

Also inherits events from its parent interface, {{DOMxRef("NameOfParentInterface")}}.

Is it better?

@skyclouds2001
Copy link
Contributor

skyclouds2001 commented Dec 16, 2023

And for cancel event, here is a problem that, it can only be fired on <input> elements (with type attribute set to file) and <dialog> elements (also CloseWatcher) according to spec https://html.spec.whatwg.org/multipage/indices.html#event-cancel

Also, for BCD, the cancel event is only under HTMLInputElement and HTMLDialogElement, not under HTMLElement

So, when going to https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/cancel_event, the BCD table and spec table will report it can't find data

Maybe it is better to follow the behavior of BCD data. That is, create two separate pages for HTMLInputElement cancel event and HTMLDialogElement cancel event and replace the current HTMLElement cancel event (and I think it is not bad because the two events have many differences)


related spec:

https://html.spec.whatwg.org/multipage/indices.html#event-cancel
https://html.spec.whatwg.org/multipage/webappapis.html#handler-oncancel

@skyclouds2001
Copy link
Contributor

Fixes #31014

The text should be added in main content part, not in comment part

@wbamberg
Copy link
Collaborator

wbamberg commented Dec 16, 2023

@Cipscis , I do appreciate that it would be easier to find this if it were on the HTMLInputElement page. But as authors (and readers), it helps to have rules to decide where to document (and where to find documentation for) web platform features, and they need to be objective, easy-ish to apply, and universally applicable.

It seems like there are three options here:

  1. Never duplicate: always document events only on the interface on which they are defined, as we do for methods and properties.
  2. Always duplicate: always document events on all subclasses, as Hamish alludes to.
  3. Sometimes duplicate.

If we choose "sometimes duplicate", what's the principle here? How do we decide what to do in any given case? What happens next week when someone files an issue that cancel is documented in the wrong place because it isn't defined on HTMLInputElement?

Maybe it is better to follow the behavior of BCD data. That is, create two separate pages for HTMLInputElement cancel event and HTMLDialogElement cancel event and replace the current HTMLElement cancel event (and I think it is not bad because the two events have many differences)

@skyclouds2001 , Yes, I wondered about this too, and it sounds like a good idea. The main drawback is that it is inaccurate: cancel AFAICT isn't defined on these subclasses.

Apart from wrongness being a problem in itself, it will be a problem if we ever want to use webref to extract information about specs for MDN, or to validate the reference docs we have on MDN (which we surely do), because webref attaches cancel to HTMLElement.

Anyway, it would not be inconsistent to cross link to the corresponding information in the HTML tag for more information about the events. OR we have to reconsider how we do that too.

@hamishwillee , yeah, it would IMO be a good thing for https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog , in its description, to mention that cancelling the dialog fires a cancel event.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Dec 18, 2023

yeah, it would IMO be a good thing for https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog , in its description, to mention that cancelling the dialog fires a cancel event.

@wbamberg That's not quite what I was suggesting. The HTML element page structure template requires that you list all elements. So we'd list all elements in <dialog> in a table as we do for <video>. That link would go to the actual event defined in HTMLElement.

From HTMLDialogElement > Events we'd add the line about inherited events, remove the cancel event, but also add a link to the newly added table in <dialog> for the list of all events.

This is all "as per the current rules" except for having a link from events in the API to having a link in the HTML element page.

PS, agree that we should follow same rule for events as properties/methods re-inheritance - "link to the parent interface". It makes no sense for anyone to create exceptions - makes things harder for authors and readers.

@skyclouds2001
Copy link
Contributor

skyclouds2001 commented Dec 18, 2023

@skyclouds2001 , Yes, I wondered about this too, and it sounds like a good idea. The main drawback is that it is inaccurate: cancel AFAICT isn't defined on these subclasses.

Apart from wrongness being a problem in itself, it will be a problem if we ever want to use webref to extract information about specs for MDN, or to validate the reference docs we have on MDN (which we surely do), because webref attaches cancel to HTMLElement.

yes, the BCD seems to use the targets that can fire the specific event (for cancel event it is HTMLInputElement and HTMLDialogElement)

but webref use the targets that can listen to the specific event (for cancel event it is HTMElement Document and Window)

the two is quite different on events relates with DOM

@hamishwillee
Copy link
Collaborator

FYI only, updated HTMLDialogElement to remove cancel() in #31131. Still interested in @wbamberg take on #30946 (comment) "do we add events table to <dialog>.

@wbamberg
Copy link
Collaborator

yeah, it would IMO be a good thing for https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog , in its description, to mention that cancelling the dialog fires a cancel event.

@wbamberg That's not quite what I was suggesting. The HTML element page structure template requires that you list all elements. So we'd list all elements in <dialog> in a table as we do for <video>. That link would go to the actual event defined in HTMLElement.

In practice we don't document "all events" on HTML element pages, we document some events, and again the criteria are not clear. But, one problem at a time. The HTML element pages are less structured and more chatty than the Web/API pages (a principled reason for this maybe being that there aren't nearly as many of them). So, I'm not against this.

@hamishwillee
Copy link
Collaborator

@wbamberg Thanks very much - you make good points.

@Cipscis Do Will's arguments above make sense to you too #30946 (comment)? If so, can we close this?

There is still scope to improve this information by adding the events in the https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog or by adding an example that spells this particular case out, or both.

@wbamberg
Copy link
Collaborator

adding an example that spells this particular case out

It would be great to have documented on the <dialog> page the conditions in which cancel is fired, it wasn't obvious to me from the page.

@Cipscis
Copy link
Author

Cipscis commented Dec 19, 2023

@hamishwillee my main concern is making sure it's easier to find out about this "cancel" event for input elements. Obviously I'm the person here who's least familiar with the ways things are standardised when editing MDN documentation, since this is my first foray into that side of it. So if my particular suggested way of surfacing this information isn't the best, then I'd be happy to see a different approach used instead.

If I understand the full discussion correctly, the alternative that's been suggested is to add a table of events to pages for HTML elements such as <input> and <dialog>, similar to the table that's currently on the <video> page, and then the DOM interface pages like HTMLInputElement and HTMLDialogElement would have a brief "Events" section that is mainly just a link to the table on the page for the relevant HTML element?

That solution sounds good to me, and I'd be happy for this PR to be closed in favour of that approach.


Also, a bit separately to the rest of my comment, since the compatability data has come up in discussion it feels relevant for me to mention the issue I raised on the browser-compat-data repo at about the same time as I created this PR: mdn/browser-compat-data#21585

@hamishwillee
Copy link
Collaborator

If I understand the full discussion correctly, the alternative that's been suggested is to add a table of events to pages for HTML elements such as <input> and <dialog>, similar to the table that's currently on the

That ^^^ is partially what I suggested.

However I wasn't proposing modifying the way that we do events in the HTMLDialogElement - other than perhaps providing a link from that doc to <dialog> to say "hey, you can get a list of events for dialog here too".

Will was suggesting an alternative which was just to document the conditions under which cancel can be fired in its own section or example - and you might also do this in HTMLDialogElement. That would be simpler, but I quite like the table approach because it is what our docs say we should do.

I'm not going to comment on the compatability data.

I'll leave this to Will to close as assigned reviewer.

@skyclouds2001
Copy link
Contributor

see https://github.com/orgs/mdn/discussions/360#discussioncomment-7934242 , maybe it is a good idea here?

that is - create two documentations under HTMLInputElement and HTMLDialogElement, and remove the one under HTMLElement, and choose one as the major documentation and another one as the least documentation

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Dec 25, 2023
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@wbamberg
Copy link
Collaborator

see https://github.com/orgs/mdn/discussions/360#discussioncomment-7934242 , maybe it is a good idea here?

that is - create two documentations under HTMLInputElement and HTMLDialogElement, and remove the one under HTMLElement, and choose one as the major documentation and another one as the least documentation

This seems essentially the same as your suggestion in #30946 (comment) ?

Maybe it is better to follow the behavior of BCD data. That is, create two separate pages for HTMLInputElement cancel event and HTMLDialogElement cancel event and replace the current HTMLElement cancel event (and I think it is not bad because the two events have many differences)

...and again, the problem is that this is incorrect, the event is defined on HTMLElement. One reason why this kind of wrongness is a bad idea is that it makes it much harder to use Web IDL to check or provide information for MDN pages.

So I'm closing this PR. Happy to consider one of the suggestions Hamish mentioned in #30946 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants