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

Site Settings: Add stopPropagation to SELECT onClick events #1228

Merged
merged 1 commit into from
Dec 11, 2015

Conversation

artpi
Copy link
Contributor

@artpi artpi commented Dec 3, 2015

What?

While nested in LABEL, SELECT onClick event was propagating
onClick was propagating to label, triggering click events higher
in DOM tree in Safari. Fixes #86

Problem

(Copied from #86 )
d176b8a4-cbc6-11e4-9899-d19b1c557455

My Sites > Settings > Discussion

Click on a select element, and it toggles the checkbox. Looks like it only affects Safari, not Chrome or Firefox. IE untested.

Testing

  1. Open Safari
  2. Go to My Sites > Settings > Discussion
  3. Change nested comments dopth
  4. Notice that toggle doesent switch

onClick was propagating to label, triggering click events higher
in DOM tree in Safari. Fixes #86
@artpi artpi added this to the Core: Iteration 17 milestone Dec 3, 2015
@artpi artpi added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 3, 2015
@@ -62,6 +62,11 @@ module.exports = {

},

recordClickEventAndStop: function( recordObject, clickEvent ) {
this.recordEvent( recordObject );
clickEvent.preventDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

@artpi I think the intent here is to stop the click event from bubbling up to the label as you noted so we only need stopPropagation. Using preventDefault here might mislead some one into thinking we want to prevent the default action of the select element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@omarjackman : TLDR: stopPropagation doesent work.
I have tried that before, but I dont know what happens on Safari that even when I stop propagation, it still goes up the DOM tree.
This is the only solution that works. Maybe some problem with React synthetic events...

@gwwar
Copy link
Contributor

gwwar commented Dec 7, 2015

@artpi I've confirmed that stopPropagation doesn't work here, and stopImmediatePropagation is unavailable on a synthetic event. Since we're adding this to work around a browser bug could you add a comment to recordClickEventAndStop and reference this issue?

You're good to 🚢 once that's done.

@gwwar gwwar added [Status] Ready to Merge [Feature] Site Settings All other general site settings. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 7, 2015
@omarjackman
Copy link
Contributor

@artpi I ran into a similar issue and found this answer on StackOverflow which makes sense of whats going on here. Cant stop propagation because it has already propagated by the time your handler is invoked. Just stating the obvious that preventDefault seems to be the best option here.

artpi added a commit that referenced this pull request Dec 11, 2015
…i-toggle

Site Settings: Add stopPropagation to SELECT onClick events
@artpi artpi merged commit 733f1f3 into master Dec 11, 2015
@artpi artpi deleted the fix/my-sites__settings-safari-toggle branch December 11, 2015 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Settings All other general site settings.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants