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

prevent-default not working when called from a trigger-real click event inside non-shadow component if registered as a passive event due to no prevent-default being used outside the component. #62

Closed
bob2517 opened this issue Nov 10, 2020 · 9 comments
Assignees
Labels
bug There is definitely a bug. Definitely. done on branch This issue has been committed to the latest branch, and will get merged with the next release weirdness It's just a bit weird.
Milestone

Comments

@bob2517
Copy link
Member

bob2517 commented Nov 10, 2020

This should be bubbling up to prevent-defaulting outside the component too. It works when outside the non-shadow DOM component on an element inside the component, but not inside for some reason.

Test shadow DOM handling for this too.

@bob2517 bob2517 added the bug There is definitely a bug. Definitely. label Nov 10, 2020
@bob2517 bob2517 added this to the 2.4.0 release milestone Nov 10, 2020
@bob2517 bob2517 self-assigned this Nov 10, 2020
@bob2517 bob2517 changed the title prevent-default not working when called from click event inside component. prevent-default not working when called from click event inside non-shadow component. Nov 10, 2020
@bob2517 bob2517 added the in progress This issue is currently being worked on label Nov 10, 2020
@bob2517 bob2517 changed the title prevent-default not working when called from click event inside non-shadow component. prevent-default not working when called from a trigger-real click event inside non-shadow component. Nov 10, 2020
@bob2517 bob2517 added the weirdness It's just a bit weird. label Nov 10, 2020
@bob2517
Copy link
Member Author

bob2517 commented Nov 10, 2020

This doesn't make sense. Upgrading to weirdness. Taking this out of the karma environment into a real setting to see what's going on.

@bob2517
Copy link
Member Author

bob2517 commented Nov 10, 2020

Repeatable outside of karma. Should be a bit easier to debug now.

@bob2517
Copy link
Member Author

bob2517 commented Nov 10, 2020

The problem has nothing to do with the trigger-real command. A regular click produces the same error.

@bob2517 bob2517 changed the title prevent-default not working when called from a trigger-real click event inside non-shadow component. prevent-default not working when called from a trigger-real click event inside non-shadow component if registered as a passive event due to no prevent-default being used outside the component. Nov 10, 2020
@bob2517
Copy link
Member Author

bob2517 commented Nov 10, 2020

Aaaahhhhhh........ it's to do with the "clever" passive event handling internally. Active CSS automatically sets whether an event is passive or not for performance purposes. It's getting confused as it's in a non-shadow component and isn't recognising the status correctly.

Hurray! Perhaps we can fix this now...

@bob2517
Copy link
Member Author

bob2517 commented Nov 10, 2020

This is going to have implications for all the events inside non-shadow components - not just click. It's most notably preventDefault that is affected.

@bob2517
Copy link
Member Author

bob2517 commented Nov 10, 2020

Was considering the removal of passive support to speed up initialization, but according to this link it's probably not a good idea. Currently we're losing on average 10 performance points on the docs website (offline on my ancient dev server) due to handling this passive nonsense. But, anyway... I'll just fix the bug...

WICG/interventions#64

Hmmm... I wonder if I can do something more general and less precise to speed things up. Maybe rather than set it by document/shadow, it just sets it by event. Ie. If an event contains prevent-default then it isn't passive ever. That will be quicker to initialize config and should cover all bases. That's going to be better than getting the host shadow/document for each nested component, etc. Good - going for that as a solution. It doesn't need splitting into components, in an actual practical sense. Every event is passive by default unless prevent-default is being used somewhere in the config for that event. I think the performance benefit from a general handling outweighs the 0.001% of sites that might benefit from a more tailored handling. The fix should be in the browser anyway, not in the core of Active CSS.

@bob2517
Copy link
Member Author

bob2517 commented Nov 10, 2020

The above is done and seems to work ok.

There's another thing. When new config gets loaded with load-config and contains a prevent-default in a previously passive event (which doesn't allow a prevent-default), if it's in the document scope the event gets regenerated and set to non-passive. But currently there is no distinction between document and shadow scopes, so if a shadow DOM component gets added to with additional config (I know right? Active CSS is awesome...) it never regenerates the event as all possible real events for shadow DOMs get set up when the shadow is drawn and never any other time.

There could be multiple shadow DOMs all sharing the same component config. Either I regenerate each event in each shadow DOM (and they each definitely need their own events), which would give a performance hit if there were a lot of components, or just set all shadow DOM events to unchangeable non-passive events by default. If I did this regeneration of events there would be a performance hit, because for every new event in new config loaded would need to be checked if it's a component, checked for it's presence in the DOM, then I'd have to iterate all instances of the shadow DOM component, isolating the exact event listener to remove, remove it for each component, then add a new one for each component. Plus there's the additional core bloat for something brought about by browser weirdness in the first place. I don't think it's worth it.

There's only ever one document event listener for events, so it's ok to regenerate as there is only ever one event regardless of any number of shared components.

I'm very tempted to set all shadow DOM events to non-passive events by default. That way there won't be a performance hit on the regeneration of events. It's a niche scenario. Because this issue could just be solved by some clever coding in the browser itself, I'm tempted to just set all shadow DOM events to be non-passive and leave it at that. I'm gonna ponder it for a bit.

@bob2517
Copy link
Member Author

bob2517 commented Nov 10, 2020

This is done offline. I'm going to leave it for today and come back to it tomorrow, and if all is good after some further testing I'm slapping it up onto the 2.4.0 branch. Offline the docs website feels really snappy.

@bob2517 bob2517 added done offline and removed in progress This issue is currently being worked on labels Nov 10, 2020
@bob2517
Copy link
Member Author

bob2517 commented Nov 11, 2020

Tests are done. Committing to 2.4.0.

@bob2517 bob2517 closed this as completed Nov 11, 2020
@bob2517 bob2517 added done on branch This issue has been committed to the latest branch, and will get merged with the next release and removed done offline labels Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug There is definitely a bug. Definitely. done on branch This issue has been committed to the latest branch, and will get merged with the next release weirdness It's just a bit weird.
Projects
None yet
Development

No branches or pull requests

1 participant