Add event handlers to RB.DialogView
Review Request #10967 — Created March 27, 2020 and submitted
This patch adds event handlers to
RB.DialogView
forsubmit
and
keydown
. The formsubmit
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 onRB.DialogView
dialogs that have a completed form will
now submit the form instead of reloading the page and discarding
changes unless theform
specifies an action explicitly. Thekeydown
event handler allows the dialog to be closed when theEscape
key
is pressed.Since
RB.DialogView
is a child of themodalBox
, it needs to listen
tokeydown
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 theevents
map for thekeydown
event and have to bind an explicit
keydown
event handler on.modalbox-inner
.
New unit tests:
- triggeringsubmit
with primary button enabled triggers the button
- triggeringsubmit
with primary button disabled on a dialog does
nothing
- triggeringsubmit
with primary button enabled on a dialog with a
form does nothing
- triggeringkeydown
using theEscape
key hides the dialogManual testing:
File upload dialog:
- pressingEnter
after opening the dialog and not filling out any
fields does nothing
- pressingEnter
after uploading a file & editing the caption uploads
the file with the caption
- pressingEscape
after opening the dialog hides the dialogNew review request dialog:
- pressingEscape
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 … |
xiaole2 | |
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 … |
chipx86 | |
This is missing an "Args" section in the docs. |
chipx86 | |
This can be simplified by letting the browser determine whether to handle the click: if (e.which === $.ui.keyCode.ENTER) { e.stopPropagation(); … |
chipx86 | |
We'll want to prevent bubbling here. |
chipx86 | |
You can combine these: evt = $.Event('keydown', {keyCode: $.ui.keyCode.ENTER}) Same comments for changes below. |
chipx86 | |
For old-school JavaScript, we should only have one block of var statements. They can be assigned a value here, but … |
chipx86 | |
Col: 38 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 38 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 38 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Just relaizing, let's rename this to _onDialogKeyDown, to reduce chances of conflicting with a subclass that might use the more … |
chipx86 | |
Is it possible to have a dialog now without a primary button? If so, we should check for that here. |
chipx86 | |
I think we still want to block form submission without a primary button. We should just have the check around … |
chipx86 |
-
-
reviewboard/static/rb/js/ui/views/dialogView.es6.js (Diff revision 1) I think using
evt
would be better thane
here to discribe event of the keyboard.
A file such asmenuButtonView.es6.js
is usingevt
Change Summary:
Add event listener to modal box to handle key events and depends link.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Depends On: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+103 -6) |
Checks run (2 succeeded)
-
-
reviewboard/static/rb/js/ui/views/dialogView.es6.js (Diff revision 2) 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));
-
reviewboard/static/rb/js/ui/views/dialogView.es6.js (Diff revision 2) This is missing an "Args" section in the docs.
-
reviewboard/static/rb/js/ui/views/dialogView.es6.js (Diff revision 2) 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.)
-
reviewboard/static/rb/js/ui/views/dialogView.es6.js (Diff revision 2) We'll want to prevent bubbling here.
-
reviewboard/static/rb/js/ui/views/tests/dialogViewTests.js (Diff revision 2) You can combine these:
evt = $.Event('keydown', {keyCode: $.ui.keyCode.ENTER})
Same comments for changes below.
-
reviewboard/static/rb/js/ui/views/tests/dialogViewTests.js (Diff revision 2) 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.
Change Summary:
Fix issues from review
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+122 -6) |
Checks run (1 failed, 1 succeeded)
JSHint
-
reviewboard/static/rb/js/ui/views/tests/dialogViewTests.js (Diff revision 3) Col: 38 Expected an assignment or function call and instead saw an expression.
-
reviewboard/static/rb/js/ui/views/tests/dialogViewTests.js (Diff revision 3) Col: 38 Expected an assignment or function call and instead saw an expression.
-
reviewboard/static/rb/js/ui/views/tests/dialogViewTests.js (Diff revision 3) Col: 38 Expected an assignment or function call and instead saw an expression.
Change Summary:
Fix lint issue
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+122 -6) |
Checks run (2 succeeded)
Change Summary:
Handle form submission
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+198 -6) |
Checks run (2 succeeded)
-
-
reviewboard/static/rb/js/ui/views/dialogView.es6.js (Diff revision 5) Just relaizing, let's rename this to
_onDialogKeyDown
, to reduce chances of conflicting with a subclass that might use the more general_onKeyDown
. -
reviewboard/static/rb/js/ui/views/dialogView.es6.js (Diff revision 5) Is it possible to have a dialog now without a primary button? If so, we should check for that here.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+198 -6) |
Checks run (2 succeeded)
-
-
reviewboard/static/rb/js/ui/views/dialogView.es6.js (Diff revision 6) I think we still want to block form submission without a primary button. We should just have the check around the click event.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+201 -6) |