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

Provides access to event properties as JSON in relevant widgets #8919

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

saqimtiaz
Copy link
Member

@saqimtiaz saqimtiaz commented Jan 30, 2025

This PR adds support to access enumerable event properties as JSON in actions invoked by $eventcatcher and $draggable widgets. This is helpful when access to an event property is needed in actions but not specifically made available as a variable.

  • The new utility method $tw.utils.copyEventProperties returns a JSON object representing the event properties while avoiding circular references, functions and DOM elements.
  • $tw.utils.collectDOMVariables now calls copyEventProperties, this required nested if statements and the diff looks large but nothing else has changed.
  • The changes to $eventcatcher are cursory and only change where in the flow we call $tw.utils.collectDOMVariables
  • Profiling shows that copying the event properties takes between 0.001 and 0.1ms depending on the event tested

To Do:

  • Test performance impact
  • Test the following events in Firefox and Chrome, testing for any failures or errors in serializing the event object:
    • click
    • dblclick
    • contextmenu
    • pointerdown
    • pointermove
    • mousedown
    • mouseup
    • mouseout
    • mouseover
    • wheel
    • keydown
    • keyup
    • focusin
    • focusout
    • drag
    • dragstart
    • dragend
    • dragenter
    • dragleave
    • dragover
    • custom events
  • Update documentation

Copy link

Confirmed: saqimtiaz has already signed the Contributor License Agreement (see contributing.md)

Copy link

netlify bot commented Jan 30, 2025

Deploy Preview for tiddlywiki-previews ready!

Name Link
🔨 Latest commit 7aa82c4
🔍 Latest deploy log https://app.netlify.com/sites/tiddlywiki-previews/deploys/679b9f718e089e0008307d89
😎 Deploy Preview https://deploy-preview-8919--tiddlywiki-previews.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@saqimtiaz
Copy link
Member Author

@Jermolene would appreciate some feedback on this when it is convenient for you, before I work on documentation

Copy link
Member

@Jermolene Jermolene left a comment

Choose a reason for hiding this comment

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

Great thanks @saqimtiaz. There doesn't appear to be anything specific about event handling in the copyEventProperties function. Perhaps it should be called something more generic like sanitizeObject or sanitizeJSON?

core/modules/utils/dom/dom.js Outdated Show resolved Hide resolved
@saqimtiaz
Copy link
Member Author

saqimtiaz commented Jan 30, 2025

There doesn't appear to be anything specific about event handling in the copyEventProperties function. Perhaps it should be called something more generic like sanitizeObject or sanitizeJSON?

@Jermolene I had that thought as well and went with the event associated name for now as that is what the testing has focused on. Perhaps copyObjectProperties? Or copyObjectPropertiesSafe in line with some of our other utility method names?

for(key in obj) {
try{
copy[key] = safeCopy(obj[key]);
} catch(e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When does this happen? safeCopy shouldn't throw, should it?

Copy link
Member Author

@saqimtiaz saqimtiaz Jan 30, 2025

Choose a reason for hiding this comment

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

Hi @CrossEye, thank you for taking a look. Some event properties have the possiblity to throw exceptions when accessed (event.target.contentWindow for cross-origin iframes for example, or event.relatedTarget at times for the focus event), and in particular I have run across libraries that implement custom events in this manner as well. So the try/catch is in the interest of robustness.

@saqimtiaz
Copy link
Member Author

There doesn't appear to be anything specific about event handling in the copyEventProperties function. Perhaps it should be called something more generic like sanitizeObject or sanitizeJSON?

@Jermolene I had that thought as well and went with the event associated name for now as that is what the testing has focused on. Perhaps copyObjectProperties? Or copyObjectPropertiesSafe in line with some of our other utility method names?

@CrossEye any thoughts on a more appropriate function name?

@Jermolene
Copy link
Member

@Jermolene I had that thought as well and went with the event associated name for now as that is what the testing has focused on. Perhaps copyObjectProperties? Or copyObjectPropertiesSafe in line with some of our other utility method names?

I like copyObjectPropertiesSafe

@saqimtiaz
Copy link
Member Author

The new utils function has been renamed to copyObjectPropertiesSafe

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

Successfully merging this pull request may close these issues.

3 participants