Add event handlers to RB.DialogView

Review Request #10967 — Created March 27, 2020 and submitted

Information

Review Board
release-3.0.x
2e812d3...

Reviewers

This patch adds event handlers to RB.DialogView for submit and
keydown. The form submit event handler fixes the bug where pressing
Enter on the upload file dialog (RB.UploadAttachmentView) would
cancel the file upload instead of actually uploading it. Pressing the
return key on RB.DialogView dialogs that have a completed form will
now submit the form instead of reloading the page and discarding
changes unless the form specifies an action explicitly. The keydown
event handler allows the dialog to be closed when the Escape key
is pressed.

Since RB.DialogView is a child of the modalBox, it needs to listen
to keydown events from the main box area and not from its own content
area so that it can handle events from within it. This means we can't
use the events map for the keydown event and have to bind an explicit
keydown event handler on .modalbox-inner.

New unit tests:
- triggering submit with primary button enabled triggers the button
- triggering submit with primary button disabled on a dialog does
nothing
- triggering submit with primary button enabled on a dialog with a
form does nothing
- triggering keydown using the Escape key hides the dialog

Manual testing:

File upload dialog:
- pressing Enter after opening the dialog and not filling out any
fields does nothing
- pressing Enter after uploading a file & editing the caption uploads
the file with the caption
- pressing Escape after opening the dialog hides the dialog

New review request dialog:
- pressing Escape after opening the dialog hides the dialog

Description From Last Updated

I think using evt would be better than e here to discribe event of the keyboard. A file such as …

xiaole2xiaole2

Just to be completely sure we have the right thing, let's use this.$el.closest('.modalbox-inner'). And in fact, it would be better …

chipx86chipx86

This is missing an "Args" section in the docs.

chipx86chipx86

This can be simplified by letting the browser determine whether to handle the click: if (e.which === $.ui.keyCode.ENTER) { e.stopPropagation(); …

chipx86chipx86

We'll want to prevent bubbling here.

chipx86chipx86

You can combine these: evt = $.Event('keydown', {keyCode: $.ui.keyCode.ENTER}) Same comments for changes below.

chipx86chipx86

For old-school JavaScript, we should only have one block of var statements. They can be assigned a value here, but …

chipx86chipx86

Col: 38 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 38 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 38 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Just relaizing, let's rename this to _onDialogKeyDown, to reduce chances of conflicting with a subclass that might use the more …

chipx86chipx86

Is it possible to have a dialog now without a primary button? If so, we should check for that here.

chipx86chipx86

I think we still want to block form submission without a primary button. We should just have the check around …

chipx86chipx86
xiaole2
  1. 
      
  2. Show all issues

    I think using evt would be better than e here to discribe event of the keyboard.
    A file such as menuButtonView.es6.js is using evt

    1. I think e is used more frequently in the codebase actually so I'm going to leave that as is for now

  3. 
      
hxqlin
chipx86
  1. 
      
  2. Show all issues

    Just to be completely sure we have the right thing, let's use this.$el.closest('.modalbox-inner').

    And in fact, it would be better to simplify this code with:

    this.$el.closest('.modalbox-inner')
        .on('keydown', this._onKeyDown.bind(this));
    
  3. Show all issues

    This is missing an "Args" section in the docs.

  4. reviewboard/static/rb/js/ui/views/dialogView.es6.js (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    This can be simplified by letting the browser determine whether to handle the click:

    if (e.which === $.ui.keyCode.ENTER) {
        e.stopPropagation();
        e.preventDefault();
    
        this._$primaryButton[0].click();
    }
    

    If the button is disabled, click() will do nothing. If it's enabeld, it will work.

    (Please verify I'm not totally wrong, but from local testing, this is the behavior.)

    1. I think you're right with the click() behaviour, but I'm not entirely sure that logic is right. The snippet you have above means that any enter key event will try to click the primary button, so any normal behavior for the enter key won't work. For example, on the file upload dialog, if you bring focus to the caption or path input with that logic, it'll try to call click() on the primary button without doing anything else, which means it becomes impossible to fill out the fields in the dialog.

      And upon further thought, I'm not even sure what I had before was right either. For example, what if someone fills out all the fields in the file upload dialog, enabling the Upload button (the primary button) then tabs to the Cancel button and presses the enter key. They'd expect to cancel the dialog right? But this key handler will end up submitting the form anyway because this situation satisfies the condition e.which === $.ui.keyCode.ENTER && !this._$primaryButton.prop('disabled')

      I added another check to see if the event target is a button or not. If it is, don't try to trigger the default button and instead use the original behavior. That fixes the case where the form is completed so the primary button is enabled but the user focuses on the Cancel button and presses enter. The dialog will close instead of submitting the contents. I'm not sure if that's the best solution but I think that behavior makes more sense. What do you think?

    2. Very good points!

      Been thinking through this, and looking at what other dialog implementations do... Here's my thought on it right now.

      Escape key makes sense, and we should support that. Looks like other dialog implementations hard-code support for the Escape key just like your change does.

      However, others do not hard-code support for the Enter key, and I think maybe that's for the best? The reason is that, unlike a native UI where this stuff is all pretty clearly baked in, the web world is still the Wild West, and a lot of things might go into a dialog that respond to the Enter key but do not prevent bubbling up (even <input type="file"> does this!). Submitting a form when a user means to interact with an element could result in data loss or unintentionally posted data.

      There are cases where Enter would definitely make sense, of course. And probably, the correct route for this would be to update modalBox to use the newer <dialog> element, does have built-in semantics around the Enter key (which we'd otherwise just be trying to re-implement), and even the Escape key. Trouble is browser compatibility, necessitating a polyfill, so that becomes a bigger issue...

      (Interestingly, even Chrome's own <dialog> polyfill doesn't handle the Enter key.)

      It's a tough call, because there's absolutely merit to handling Enter universally, but I'm kind of leaning toward erring on the side of safety and not implementing Enter, just Escape.

    3. I see; those are all strong arguments for not implementing Enter at the moment. With that, is there anything we could do to address the bug that originally led us into investigating this?

    4. So I'm not clear on the bug why this is happening, but I suspect that focus was on something that resulted in the dismissal of the dialog. Are you able to fully reproduce the bug with the Enter key handling being removed but with the new modalBox focus code being enabled?

    5. Yes, I'm still able reproduce the bug. I think it's something to do with the default behavior of inputs in a form? The specific path I'm using to reproduce the bug is 1. open the dialog, 2. click file input & upload file, 3. fill in caption field, 4. press Enter and expect it to submit - instead, it refreshes the page, effectively discarding all changes.

  5. Show all issues

    We'll want to prevent bubbling here.

  6. Show all issues

    You can combine these:

    evt = $.Event('keydown', {keyCode: $.ui.keyCode.ENTER})
    

    Same comments for changes below.

  7. Show all issues

    For old-school JavaScript, we should only have one block of var statements. They can be assigned a value here, but you should define them above.

    Same comments for changes below.

  8. 
      
hxqlin
Review request changed
Change Summary:

Fix issues from review

Commit:
b932d6259770a951486e58b7f7248e8242edc1bc
36944b45dfce917ff486532efc0de62188efb789

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

hxqlin
hxqlin
chipx86
  1. 
      
  2. Show all issues

    Just relaizing, let's rename this to _onDialogKeyDown, to reduce chances of conflicting with a subclass that might use the more general _onKeyDown.

  3. Show all issues

    Is it possible to have a dialog now without a primary button? If so, we should check for that here.

  4. 
      
hxqlin
chipx86
  1. 
      
  2. reviewboard/static/rb/js/ui/views/dialogView.es6.js (Diff revision 6)
     
     
     
     
     
    Show all issues

    I think we still want to block form submission without a primary button. We should just have the check around the click event.

  3. 
      
hxqlin
david
  1. Ship It!
  2. 
      
hxqlin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (bceb0b6)