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.

Sign up for GitHub

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 when handling keydown events #836

Merged
merged 3 commits into from
Jan 26, 2017
Merged

Prevent default when handling keydown events #836

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
112ed14
Prevent default when handling keydown events
T-Hugs Jan 18, 2017
c4571b6
Update ContextualMenu.tsx
T-Hugs Jan 18, 2017
0557d14
Merge branch 'master' into patch-1
cliffkoh Jan 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext

if (ev.which === openKey) {
this._onItemSubMenuExpand(item, ev.currentTarget as HTMLElement);
ev.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this also have ev.stopPropagation() ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think preventDefault makes sense, maybe for logging or something like that? Where it should continue to propagate but no other native actions should take place?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think from the description it should propagate so that the parent can handle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I'd argue that stopPropagation is almost never what you want. The parent should look at the defaultPrevented property on the event object in most cases to determine if action should be taken. If propagation is stopped, the parent doesn't even get that choice.

@joschect You mention native actions, but is that what you mean? The preventDefault() call obviously prevents native actions being handled, but more importantly parent event handlers can look at the defaultPrevented property on the event to see if the default action has already been taken by a child handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, preventing native actions is all I meant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I talked with @dzearing and I agree with him that it should have both preventDefault and stopPropagation. The code should prevent the browser from handling the event as well as preventing it from getting to other handlers.

Copy link
Member

@dzearing dzearing Jan 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think it would be probably make the most sense to add it. It would at least be consistent with other places where we handle events.

I'd be interested in how it's cleaner to avoid stopPropagation, it's a bit unclear to me, I might be naive here tho! I thought about the purposes of the 2 methods:

preventDefault: Prevents the default browser behavior. It is intended to prevent things like scrollbars moving, buttons submitting forms, and anchors from navigating.

stopPropagation: Prevents the event from bubbling, which avoids user code. It doesn't affect browser behaviors; it simply avoids user code from executing. This is important when an event has been handled and it is intended for no other actions to handle the event.

So interpretting defaultPrevented as a signal for avoiding user code might be presumptuous or wrong.

Again, possible to watch for defaultPrevented, it just may not be the right signal indicating that user code executed an action and subsequent user code actions should be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API gives us two methods, preventDefault and stopPropagation. Given the above argument, it is hard to imagine a scenario where it would be useful to call one of the two methods, but not both. This makes me think they are not necessarily intended to be called as a pair in all/most cases.

Here's an example scenario where it might be useful for a parent to react to events that have been handled by the child:

Say you have a ContextMenu that is configured to hide when the user "clicks away". That is, the user clicks any part of the UI that is not part of the menu. The user might click a button or some other element that handles the click. But you still want the menu to close. If the event propagation was stopped, that event would not be able to make it up to the document triggering the closing of the ContextMenu.

Anyway, I'm not trying to be argumentative here. I think calling both preventDefault and stopPropagation is the more common pattern, so it may be best to follow that.

Copy link
Contributor

@mdahamiwal mdahamiwal Jan 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should blindly use both of them. stopPropagation can be hazardous as evident from the example by @trevorsg. Infact, we just fixed a bug in Dropdown to avoid stop propagating event if it doesn't handle the keypress and give chance to parent components (dialog) to act on it.
preventDefault is mostly used when we want to avoid browser default behavior and handle it ourselves. As in this PR, with custom context menu we don't want browsers to open its default context menu for some element. Or say, we want to avoid link click navigation and handle it ourselves. That said, preventDefault and defaultPrevented can be used as an alternative to stopPropagation. It is explained at dangers of stopping event propagation as one of the best practices to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for finding that article, Manish. It organizes my thoughts on the subject very nicely!

}
}

Expand Down