Prevent default when handling keydown events #836

T-Hugs · 2017-01-18T18:31:14Z

Pull request checklist

Addresses an existing issue: ContextualMenu: Default not prevented after handling keydown #835
Include a change request file if publishing
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Description of changes

Adds preventDefault() when handling keydown event on a ContextualMenu. This is necessary so that event handlers on parent elements know whether or not the event was handled. In our case, an arrow key pressed in a menu might not have an effect (e.g. there is no right sub-menu), in which case the parent needs to handle the user's intent (move to the next horizontal menu item, for example).

Focus areas to test

(optional)

msftclas · 2017-01-18T18:31:35Z

Hi @trevorsg, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Trevor Gau). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

T-Hugs · 2017-01-18T18:34:56Z

By the way, this PR is submitted on behalf of Craig Harry, who cannot log into GitHub because he does not have a phone for 2FA.

dzearing · 2017-01-18T19:34:01Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

@@ -459,6 +459,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext

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


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

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?

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

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.

Yeah, preventing native actions is all I meant.

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.

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.

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.

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.

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

cliffkoh · 2017-01-24T09:13:32Z

Hi @trevorsg, to get this checked in, could you just run rush change to generate a change file?

T-Hugs · 2017-01-25T21:38:51Z

@cliffkoh Is there a doc I'm supposed to read about how to do that? I npm installed @microsoft/rush, then ran rush change, but I get this error...

C:\dev\office-ui-fabric-react\packages\office-ui-fabric-react\src\components\ContextualMenu>rush change

Rush Multi-Package Build Tool 1.5.1

Found configuration in C:\dev\office-ui-fabric-react\rush.json

events.js:160
      throw er; // Unhandled 'error' event
      ^

Error: This socket is closed
    at WriteStream.Socket._writeGeneric (net.js:683:19)
    at WriteStream.Socket._write (net.js:734:8)
    at doWrite (_stream_writable.js:334:12)
    at writeOrBuffer (_stream_writable.js:320:5)
    at WriteStream.Writable.write (_stream_writable.js:247:11)
    at WriteStream.Socket.write (net.js:661:40)
    at Console.warn (console.js:51:16)
    at RushCommandLineParser.trapErrors (C:\Users\trgau\AppData\Roaming\npm\node_modules\@microsoft\rush\lib\actions\RushCommandLineParser.js:72:25)
    at RushCommandLineParser.onExecute (C:\Users\trgau\AppData\Roaming\npm\node_modules\@microsoft\rush\lib\actions\RushCommandLineParser.js:57:14)
    at RushCommandLineParser.CommandLineParser.execute (C:\Users\trgau\AppData\Roaming\npm\node_modules\@microsoft\rush\node_modules\@microsoft\ts-command-line\lib\CommandLineParser.js:76:14)

cschleiden · 2017-01-25T22:02:33Z

@trevorsg I just run npm run change from the root

T-Hugs · 2017-01-25T22:05:07Z

@cschleiden I still get the same error. Is there an order of operations? I made a commit then ran npm run change - am I missing a step?

cschleiden · 2017-01-26T01:37:55Z

@trevorsg Hm, not that I'm aware of. That's how it worked for me last night :)

cliffkoh · 2017-01-26T01:41:38Z

@trevorsg, for the issue you encountered, @qinweiz and @pgonzal can help. An alternative is to just manually create a change file in common/changes, something like this: https://github.com/OfficeDev/office-ui-fabric-react/blob/master/common/changes/separate-button-sass-fixes_2017-01-25-16-36.json

octogonz · 2017-01-26T02:03:10Z

Can you run rush -d change and paste the console output?

Also, do you have a repro? (e.g. enlist in branch X and run these commands, something like that?) I wasn't following this thread, so I don't have much context, but if there's a Rush bug, we're interested.

@trevorsg

T-Hugs · 2017-01-26T14:59:02Z

@pgonzal
Here's the output with -d:

C:\dev\office-ui-fabric-react>rush -d change

Rush Multi-Package Build Tool 1.5.1


events.js:160
      throw er; // Unhandled 'error' event
      ^

Error: write EINVAL
    at exports._errnoException (util.js:1022:11)
    at WriteStream.Socket._writeGeneric (net.js:715:26)
    at WriteStream.Socket._write (net.js:734:8)
    at doWrite (_stream_writable.js:334:12)
    at writeOrBuffer (_stream_writable.js:320:5)
    at WriteStream.Writable.write (_stream_writable.js:247:11)
    at WriteStream.Socket.write (net.js:661:40)
    at Object.execSync (child_process.js:526:20)
    at Function.VersionControl.getChangedFolders (C:\Users\trgau\AppData\Roaming\npm\node_modules\@microsoft\rush\node_modules\@microsoft\rush-lib\lib\utilities\VersionControl.js:8:36)
    at ChangeAction._getChangedPackageNames (C:\Users\trgau\AppData\Roaming\npm\node_modules\@microsoft\rush\lib\actions\ChangeAction.js:99:56)

Repro:

Clone this repo
CD into repo
npm i
npm i -g @microsoft/rush
<make change>
git commit -am "foo"
rush change

Let me know if I can get you any more info.

octogonz · 2017-01-26T18:53:40Z

@trevorsg What was step #5? :-) I tried commiting a trivial change to DetailsList.ts and then running "rush change", but it works fine for me.

Do I need to switch to a certain branch? Also, what OS are you using?

T-Hugs · 2017-01-26T18:59:11Z

@pgonzal Updated :) I forgot to escape the < and >

octogonz · 2017-01-26T19:02:00Z

@trevorsg Thanks, however I still can't repro the bug. I tried to find your branch trevorsg:patch-1, but it looks like it got squashed when the PR was merged. :-(

cliffkoh · 2017-01-26T19:46:42Z

@trevorsg 1 thought - what version of node are you running on? (node -v), and what OS? :)

T-Hugs · 2017-01-26T20:09:16Z

@cliffkoh I was on 7.1.0. Looks like I was hitting something related to this issue: nodejs/node#9542

I updated to 7.2.1 and now everything is working. Thanks @pgonzal for helping debug.

octogonz · 2017-01-26T20:14:01Z

@cliffkoh @dzearing I noticed that your rush.json file has this line:

  "nodeSupportedVersionRange": ">=4.3.0",

I would suggest to restrict it to something like this:

"nodeSupportedVersionRange": ">=6.9.0 <7.0.0",

Supporting a wide range of NodeJS versions has been a nuisance for us in the past. For our projects, we generally require people to be on the recent LTS version range. NodeJS rolls them pretty often, so you won't be missing any killer features, and it's hardly any hassle if people are using nodist or nvm (which I strongly recommend).

cliffkoh · 2017-01-26T21:14:41Z

@pgonzal Created an issue to track this, will get to it later today or tomorrow. Thanks!

dzearing · 2017-01-27T07:46:44Z

@pgonzal thanks!

…ft#836) * Prevent default when handling keydown events * Update ContextualMenu.tsx

Prevent default when handling keydown events

112ed14

msftclas added the cla-not-required label Jan 18, 2017

Update ContextualMenu.tsx

c4571b6

joschect approved these changes Jan 18, 2017

View reviewed changes

dzearing reviewed Jan 18, 2017

View reviewed changes

Merge branch 'master' into patch-1

0557d14

dzearing merged commit 179d155 into microsoft:master Jan 26, 2017

cliffkoh mentioned this pull request Jan 26, 2017

Further restrict rush versions per @pgonzal recommendations. #895

Closed

mdahamiwal mentioned this pull request Feb 2, 2017

Pickers: Make suggestions dismiss properly in dialog. #933

Merged

4 tasks

aleksandrjPersonal pushed a commit to aleksandrjPersonal/office-ui-fabric-react that referenced this pull request Feb 17, 2017

ContextualMenu: Prevent default when handling keydown events (microso…

3fc4055

…ft#836) * Prevent default when handling keydown events * Update ContextualMenu.tsx

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent default when handling keydown events #836

Prevent default when handling keydown events #836

T-Hugs commented Jan 18, 2017 •

edited

Loading

msftclas commented Jan 18, 2017

T-Hugs commented Jan 18, 2017

dzearing Jan 18, 2017

joschect Jan 18, 2017

cschleiden Jan 18, 2017

T-Hugs Jan 18, 2017

joschect Jan 18, 2017

joschect Jan 18, 2017

dzearing Jan 18, 2017 •

edited

Loading

T-Hugs Jan 18, 2017

mdahamiwal Jan 19, 2017 •

edited

Loading

T-Hugs Jan 19, 2017

cliffkoh commented Jan 24, 2017

T-Hugs commented Jan 25, 2017

cschleiden commented Jan 25, 2017

T-Hugs commented Jan 25, 2017

cschleiden commented Jan 26, 2017

cliffkoh commented Jan 26, 2017

octogonz commented Jan 26, 2017

T-Hugs commented Jan 26, 2017 •

edited

Loading

octogonz commented Jan 26, 2017 •

edited

Loading

T-Hugs commented Jan 26, 2017

octogonz commented Jan 26, 2017

cliffkoh commented Jan 26, 2017 •

edited

Loading

T-Hugs commented Jan 26, 2017

octogonz commented Jan 26, 2017

cliffkoh commented Jan 26, 2017

dzearing commented Jan 27, 2017

Prevent default when handling keydown events #836

Prevent default when handling keydown events #836

Conversation

T-Hugs commented Jan 18, 2017 • edited Loading

Pull request checklist

Description of changes

Focus areas to test

msftclas commented Jan 18, 2017

T-Hugs commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzearing Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdahamiwal Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffkoh commented Jan 24, 2017

T-Hugs commented Jan 25, 2017

cschleiden commented Jan 25, 2017

T-Hugs commented Jan 25, 2017

cschleiden commented Jan 26, 2017

cliffkoh commented Jan 26, 2017

octogonz commented Jan 26, 2017

T-Hugs commented Jan 26, 2017 • edited Loading

octogonz commented Jan 26, 2017 • edited Loading

T-Hugs commented Jan 26, 2017

octogonz commented Jan 26, 2017

cliffkoh commented Jan 26, 2017 • edited Loading

T-Hugs commented Jan 26, 2017

octogonz commented Jan 26, 2017

cliffkoh commented Jan 26, 2017

dzearing commented Jan 27, 2017

T-Hugs commented Jan 18, 2017 •

edited

Loading

dzearing Jan 18, 2017 •

edited

Loading

mdahamiwal Jan 19, 2017 •

edited

Loading

T-Hugs commented Jan 26, 2017 •

edited

Loading

octogonz commented Jan 26, 2017 •

edited

Loading

cliffkoh commented Jan 26, 2017 •

edited

Loading