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

Improve Web/API/Event/Comparison_of_Event_Targets #33644

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

leon-win
Copy link
Member

Description

This PR updates Web/API/Event/Comparison_of_Event_Targets. List of changes (in the order of their location in the document):

  • fix count of event target types,
  • clarify that relatedTarget refers specifically to mouse events,
  • move {{Non-standard_Inline}} labels next to the event target name, similar to this formatting in the sidebar and to relieve the overcrowded third column,
  • remove link to Firefox bug (https://bugzil.la/185889) which was closed 22 years ago (!),
  • remove broken link (https://developer.mozilla.org/en-US/docs/XBL/XBL_1.0_Reference/Anonymous_Content#Event_Flow_and_Targeting),
  • divide first example into sections (CSS, HTML, JS), improve, add EmbedLiveSample and make its header more general ("Difference between event targets"), because this example is so in its implementation,
  • replace markdown table to HTML version and improve wording in "Use of target and relatedTarget" section,
  • replace its obsolete example with actual implementation and add EmbedLiveSample,
  • add "See also" section.

Additional details

Some screenshots after changes:
Screenshot from 2024-05-16 21-53-59
Screenshot from 2024-05-16 21-50-43
Screenshot from 2024-05-16 21-50-58
Screenshot from 2024-05-16 21-51-25

@leon-win leon-win requested a review from a team as a code owner May 16, 2024 18:55
@leon-win leon-win requested review from wbamberg and removed request for a team May 16, 2024 18:55
@github-actions github-actions bot added Content:WebAPI Web API docs size/m 51-500 LoC changed labels May 16, 2024
Copy link
Contributor

github-actions bot commented May 16, 2024

Preview URLs

External URLs (1)

URL: /en-US/docs/Web/API/Event/Comparison_of_Event_Targets
Title: Comparison of Event Targets

(comment last updated: 2024-06-12 20:50:00)

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR, @leon-win !

This is obviously an improvement over what's there now, but I think it has bigger problems, still.

The two main problems, IMO, are:

  • it's in the wrong place. Guide pages that live under interface pages don't appear in the site nav, so are basically unfindable.
  • I don't think it's worth including the three Firefox-only properties in here. That means there are really just two things to talk about: (1) the difference between target and currentTarget, and (2) relatedTarget. However, since relatedTarget is quite specific to just one event type (and is already documented there: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/relatedTarget), it doesn't make sense to talk about it in general event documentation.

So what we should do is:

Please let me know if you'd like to have a go at this, or if you'd rather I close this PR and take care of it myself.

@leon-win
Copy link
Member Author

leon-win commented Jun 6, 2024

Thank you for the review, @wbamberg and I apologize for the long response.

This is obviously an improvement over what's there now, but I think it has bigger problems, still.

I agree with you.

The two main problems, IMO, are:

  • it's in the wrong place. Guide pages that live under interface pages don't appear in the site nav, so are basically unfindable.

  • I don't think it's worth including the three Firefox-only properties in here. That means there are really just two things to talk about: (1) the difference between target and currentTarget, and (2) relatedTarget. However, since relatedTarget is quite specific to just one event type (and is already documented there: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/relatedTarget), it doesn't make sense to talk about it in general event documentation.

So what we should do is:

This is a good solution, but I would like to draw attention to two points:

Therefore, I suggest considering moving the page to a more suitable location instead of deleting it. This will allow to save information about Firefox specific properties (after all, in general it can be useful to someone).

Please let me know if you'd like to have a go at this, or if you'd rather I close this PR and take care of it myself.

Yes, I will try to do this (but it may not work at a sufficient level, since I am not an English speaking person). And it will take some time =)

Therefore, maybe we won’t close this PR, but rather merge it, because it will correct the errors that exist in the current version?

@wbamberg
Copy link
Collaborator

wbamberg commented Jun 6, 2024

Thank you for the review, @wbamberg and I apologize for the long response.

This is obviously an improvement over what's there now, but I think it has bigger problems, still.

I agree with you.

The two main problems, IMO, are:

  • it's in the wrong place. Guide pages that live under interface pages don't appear in the site nav, so are basically unfindable.
  • I don't think it's worth including the three Firefox-only properties in here. That means there are really just two things to talk about: (1) the difference between target and currentTarget, and (2) relatedTarget. However, since relatedTarget is quite specific to just one event type (and is already documented there: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/relatedTarget), it doesn't make sense to talk about it in general event documentation.

So what we should do is:

This is a good solution, but I would like to draw attention to two points:

This is a good point! Then I have a new suggestion: split https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events into two pages, by extracting "Event bubbling" and "Event delegation" into a new page, called "Event bubbling", and include the discussion of target and currentTarget in the second page.

That would improve the existing learn content, by making the event intro page shorter and giving more space for event bubbling, which is a relatively complex bit of events.

  • Now the comparison of events has been placed in a separate document (however, it is located in the wrong place), in my opinion this is a good decision, since the topic is separate (although related to events, of course). Therefore, if we delete it, we will lose some of the information. If we move it to the https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events page, we will increase its size.

I think target versus currentTarget is very closely tied to bubbling, to the extent that it's better to explain it together (note that https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events#event_delegation already mentions it, but not in enough detail).

Therefore, I suggest considering moving the page to a more suitable location instead of deleting it. This will allow to save information about Firefox specific properties (after all, in general it can be useful to someone).

Can it though? I don't know what "f the event was retargeted for some reason other than an anonymous boundary crossing, this will be set to the target before the retargeting occurs." means, and we don't explain. What's anonymous boundary crossing, and when would it be relevant for web developers? We do still have https://developer.mozilla.org/en-US/docs/Web/API/Event/explicitOriginalTarget etc.

I'll ask the other maintainers if anyone thinks we should keep this content.

Please let me know if you'd like to have a go at this, or if you'd rather I close this PR and take care of it myself.

Yes, I will try to do this (but it may not work at a sufficient level, since I am not an English speaking person). And it will take some time =)

Therefore, maybe we won’t close this PR, but rather merge it, because it will correct the errors that exist in the current version?

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/m 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants