Fix for pressing enter on upload file dialog cancelling upload

Review Request #10820 — Created Jan. 13, 2020 and updated

hxqlin
Review Board
release-3.0.x
4847
f973c85...
reviewboard

Fix bug where pressing enter on upload file dialog would cancel the upload
instead of actually uploading it. This bug occurred because the first button,
which is the cancel button in the dialog, is always the default button used
to submit a form. The fix is to make the upload button the primary, or
default, button and use the Enter key event to upload the file if one has
been uploaded. This RB changes uploadAttachmentView to extend dialogView
as opposed to calling the dialogView constructor directly. In addition, it
adds a handler for the Escape key so that the dialog can be closed using the
Escape key.

Manually tested:
- uploading a file via the dialog and using the enter key to upload or by
pressing the "Upload" button
- canceling the dialog by pressing the escape key or by pressing the "Cancel"
button

Unit tests:
- new dialog view button properties (id, class, disabled)
- pressing the enter key when the primary button is enabled and disabled and
pressing the escape key for the dialog

Description From Last Updated

In your description, we don't need to talk about browsers without JS--Review Board really isn't usable if JS is turned ...

daviddavid

Just curious, can you test what happens when esc is pressed?

daviddavid

Thanks for working on this, Hannah! Reversing the order of buttons may fix it, but I think only accidentally (and ...

chipx86chipx86

We should call this only if there's an id set. I'd make this its own statement, wrapped in an if ...

chipx86chipx86

Rather than define these, let's use the definitions that already exist: $.ui.keyCode.ENTER and $.ui.keyCode.ESCAPE.

chipx86chipx86

Let's pull buttons[0] into a variable and then test against that. It'll be a little faster (avoids repeated index lookups).

chipx86chipx86

Jasmine has a handy .toBeFalse() and .toBeTrue(), which is more correct in these cases.

chipx86chipx86

You'll want to do this right at the class level, like: events: _.extend({ 'change #path': '.....', }, RB.DialogView.prototype.events),

chipx86chipx86

So something that I noticed when I looked at the original code we had earlier is that we're playing with ...

chipx86chipx86

This is still best done in render. It's kind of like initialize but for the initial display, so it's the ...

chipx86chipx86

Is there a reason why we're using !!buttonInfo.danger instead of just simply buttonInfo.danger?

bui1bui1
david
  1. Code looks pretty good. Just a couple small questions.

    I'd suggest wrapping your description and testing done sections to a reasonable length (80 chars, typically) to make them more readable.

    1. Woops missed the part about the description until now

  2. In your description, we don't need to talk about browsers without JS--Review Board really isn't usable if JS is turned off.

    1. Removed bit about browsers without JS

  3. Just curious, can you test what happens when esc is pressed?

    1. Nothing happens when esc is pressed

    2. Can we add a handler so that esc cancels the dialog?

    3. In fact, while I think switching the order of the buttons is correct, I think we should also have a handler to ensure that the enter key binds to the correct thing.

  4. 
      
hxqlin
chipx86
  1. 
      
  2. Thanks for working on this, Hannah!

    Reversing the order of buttons may fix it, but I think only accidentally (and I don't like having to shuffle around UI to make something work). Since the Upload button is disabled by default, I don't believe we're guaranteed that all browsers will treat it as the primary button.

    It's always better to be explicit in how a bug gets fixed, particularly when we're dependent on browser behavior. Also, we're going to encounter this in other dialogs as well, which may have even more buttons and more complicated button layouts.

    modalBox is ooooold

    So this is old code. We actually have this far-nicer thing called RB.DialogView, which we'd like to port code to. It has a concept of primary buttons, too.

    It's a bit newer, and it might need some improvement, but that's where any work toward doing this correctly should go.

    So I'd suggest changing this code to not call .modalBox() directly (okay, RB.DialogView does call this, but don't worry about that). You can create the element for the contents of the dialog and instantiate using:

    const dialogView = new RB.DialogView({
        title: ...,
        body: yourElement,
        buttons: [
            {
                label: gettext('Upload'),
                primary: true,
                onClick: () => this.send(),
            },
            {
                label: gettext('Cancel'),
            }
        ],
    });
    

    Something like that.

    Now, this probably won't fix it directly, but here's where phase 2 comes in! You can modify reviewboard/static/rb/js/ui/views/dialogView.es6.js to register a key event for Enter and a key event for Escape and hook those up, like:

    RB.DialogView = Backbone.View.extend({
       events: {
           'keypress': _onKeyPress,
       },
    
       ...
    
       _onKeyPress(e) {
           /* Check for Enter and Escape here */
       }
    });
    

    If Enter, activate the primary button if we have one (loop through this.options, look for primary).

    If Escape, just call this.hide().

    I think this would be fantastic addition, and would solve the bug cleanly!

    Make sure that you write unit tests, though. Check out ui/views/tests/dialogView.es6.js. You'll want to simulate key events. Here's how:

    const evt = $.Event('keypress')
    evt.which = $.ui.keyCode.ENTER;
    dialogView.$el.trigger(evt);
    
    1. Oops, that last bit was meant to point you to ui/views/tests/dialogViewTests.es6.js.

      Check out our (short) guide on writing JavaScript unit tests.

    2. Sounds good, I'm looking into it! Have a couple questions though -
      1. when you say "activate the primary button," are you saying just to call the onClick function of the button found by something like this.options.buttons.find(button => button.primary)
      2. what's the best way of checking if this primary button is safe to activate? for the upload attachment dialog, we should only enable the upload button if a file has been uploaded, meaning the file path is not null. In the dialog view though, there's currently no way of enforcing that

    3. Hi Hannah.

      1. Sorta. The onClick being passed in isn't always a function. It can be a name of a function on the instance of RB.DialogView to call (works like the Backbone.View's events mapping). So we can't rely on it being a function. What we can do is call triggerHandler('click') on the button element, which will simulate a click and result in the right function being called.
      2. Excellent point. I think the best approach would be, when generating the list of buttons in _getButtons(), to store the primary button as a member variable on RB.DialogView (this._$primaryButton). No need for a loop then in the Enter handler. We can then check if it's disabled before calling onClick.
  3. 
      
hxqlin
chipx86
  1. 
      
  2. We should call this only if there's an id set. I'd make this its own statement, wrapped in an if that checks for it.

    (Frankly, we should probably be doing that with all the things we're adding -- we're cheating a bit with the toggleClass thing, and it's not as performant as just an if with addClass.)

  3. Rather than define these, let's use the definitions that already exist: $.ui.keyCode.ENTER and $.ui.keyCode.ESCAPE.

  4. Let's pull buttons[0] into a variable and then test against that. It'll be a little faster (avoids repeated index lookups).

  5. Jasmine has a handy .toBeFalse() and .toBeTrue(), which is more correct in these cases.

    1. These can't be used at the moment because of the Jasmine version - the current version is 2.4.1 but toBeFalse and toBeTrue are available since 3.5.0. https://jasmine.github.io/api/edge/matchers.html

  6. You'll want to do this right at the class level, like:

    events: _.extend({
        'change #path': '.....',
    }, RB.DialogView.prototype.events),
    
  7. So something that I noticed when I looked at the original code we had earlier is that we're playing with fire using these IDs. #upload and #path are wayyyyy too generic, and are bound to conflict. There should never be more than one element with an ID, and if there is, bad things can happen with event handling and such.

    So what I'd suggest doing is two things:

    1. Adding support for CSS classes instead of or in addition to IDs, when registering buttons.
    2. Using our CSS standard naming scheme to come up with suitable classes.

    For the latter, I don't want to have you pour too much more work into this, because this can get out of control, so instead of revamping the entire CSS for this dialog, we can make use of these handy js--prefixed classes. So, js-path and js-upload-button.

    What js- means is "I'm a thing owned by some JavaScript, and nothing but the thing that owns me should deal with me." So no custom styling, so page-wide DOM queries, nothing. It's good for this purpose.

    By using those instead of hard-coded IDs, we'll avoid potentially weird bugs down the line.

  8. This is still best done in render. It's kind of like initialize but for the initial display, so it's the right place to pull out variables. We then don't have to define show(), letting us just take advantage of the parent class's version, and trusting that we'll be pulling out variables before it's actually shown.

    Make sure though, especially with the above js- prefixes I mentioned, to isolate the lookup to this element. You can use a shorthand available to views:

    this._$path = this.$('.js-path');
    this._$uploadBtn = this.$('.js-upload-button');
    
    1. I don't think it's possible to do this in render - render gets called before show and _getButtons does. If we tried to do this in render, this.$path and this._$uploadBtn before they even exist in the DOM. Also just wanted to check - did you mean

      this._$path = $('.js-path');
      this._$uploadBtn = $('.js-upload-button');
      

      or the snippet you wrote above?

  9. 
      
hxqlin
hxqlin
Review request changed

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
bui1
  1. 
      
  2. Is there a reason why we're using !!buttonInfo.danger instead of just simply buttonInfo.danger?

    1. This is a JavaScriptism that converts a truthy/falsy value into a true/false.

      For instance, let's say you have a truthy value like "foo". Negating (!"foo") gives you false, and then negating again (!!"foo") gives you true.

      For a falsy value, like "", negating onces (!"") gives you true, and then again (!!"") gives you false.

      So it's really just shorthand for value ? true : false.

  3. Just a question out of curiousity but what will happen to the dialogue if neither enter/esc is pressed on the keyboard but rather another keyCode like any of the digits/alphabet etc?

    Will the dialogue stay open and not produce any errors? Does the dialogue just ignore those key inputs?

  4. Also wondering why 'send' is used for onClick instead of a function that would trigger an event that would allow the user to upload their file?

    1. Ignore this comment above. I just spent some more time reading backbone js documentations and got it!

  5. 
      
Loading...