flake8
passed.
JSHint
passed.
Add event handlers to RB.DialogView
Review Request #10967 — Created March 27, 2020 and submitted
This patch adds event handlers to
RB.DialogViewforsubmitand
keydown. The formsubmitevent handler fixes the bug where pressing
Enteron the upload file dialog (RB.UploadAttachmentView) would
cancel the file upload instead of actually uploading it. Pressing the
return key onRB.DialogViewdialogs that have a completed form will
now submit the form instead of reloading the page and discarding
changes unless theformspecifies an action explicitly. Thekeydown
event handler allows the dialog to be closed when theEscapekey
is pressed.Since
RB.DialogViewis a child of themodalBox, it needs to listen
tokeydownevents 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 theeventsmap for thekeydownevent and have to bind an explicit
keydownevent handler on.modalbox-inner.
New unit tests:
- triggeringsubmitwith primary button enabled triggers the button
- triggeringsubmitwith primary button disabled on a dialog does
nothing
- triggeringsubmitwith primary button enabled on a dialog with a
form does nothing
- triggeringkeydownusing theEscapekey hides the dialogManual testing:
File upload dialog:
- pressingEnterafter opening the dialog and not filling out any
fields does nothing
- pressingEnterafter uploading a file & editing the caption uploads
the file with the caption
- pressingEscapeafter opening the dialog hides the dialogNew review request dialog:
- pressingEscapeafter 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
|
- Change Summary:
-
Add event listener to modal box to handle key events and depends link.
- Description:
-
This patch adds event handlers for the
EscandEnterkey toRB.DialogView, fixing the bug where pressing enter on the upload filedialog ( RB.UploadAttachmentView) would cancel the file upload instead ofactually uploading it. Pressing the return key on RB.DialogViewdialogsnow triggers the primary button if it isn't disabled, which is explicitly set by the caller. Previously, whichever button came first in the document would be triggered, which was the Cancelbutton for the upload file dialog.~ The keyboard events need to have the dialog focused so showing the dialog
~ will focus the primary button if it isn't disabled or the first available ~ button in the list of rendered button elements. In the case of the upload ~ attachment dialog, the primary button is only enabled if a file has been ~ Since
RB.DialogViewis a child of themodalBox, it needs to listen to~ 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 and have to bind an explicit keydownevent handler on.modalbox-inner.- uploaded so we instead focus on the path input, which is a required field in - the dialog. This change makes the dialog workflow keyboard accessible. - Previously, focus wasn't brought to the dialog when it was opened so the next - element tabbed to wouldn't be in the dialog. - Depends On:
-
- Commit:
7e6f021edd9a61ecafff4252be81ffb35175dc45b932d6259770a951486e58b7f7248e8242edc1bc- Diff:
Revision 2 (+103 -6)
Checks run (2 succeeded)
flake8 passed.JSHint passed.
-
-
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)); -
-
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.)
-
-
You can combine these:
evt = $.Event('keydown', {keyCode: $.ui.keyCode.ENTER})Same comments for changes below.
-
For old-school JavaScript, we should only have one block of
varstatements. 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:
-
b932d6259770a951486e58b7f7248e8242edc1bc36944b45dfce917ff486532efc0de62188efb789
Checks run (1 failed, 1 succeeded)
flake8
passed.
JSHint
failed.
JSHint
- Change Summary:
-
Fix lint issue
- Commit:
-
36944b45dfce917ff486532efc0de62188efb7899079a675bf0ea1e96913a2a965b799cc76fb6762
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Handle form submission
- Summary:
-
Add keyboard events to RB.DialogViewAdd event handlers to RB.DialogView
- Description:
-
~ This patch adds event handlers for the
EscandEnterkey to~ RB.DialogView, fixing 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.DialogViewdialogs~ now triggers the primary button if it isn't disabled, which is explicitly ~ set by the caller. Previously, whichever button came first in the document ~ would be triggered, which was the Cancelbutton for the upload file dialog.~ This patch adds event handlers to
RB.DialogViewforsubmitand~ keydown. The formsubmitevent handler fixes the bug where pressing~ Enteron the upload file dialog (RB.UploadAttachmentView) would~ cancel the file upload instead of actually uploading it. Pressing the ~ return key on RB.DialogViewdialogs that have a completed form will~ now submit the form instead of reloading the page and discarding ~ changes unless the formspecifies an action explicitly. Thekeydown+ event handler allows the dialog to be closed when the Escapekey+ is pressed. ~ Since
RB.DialogViewis a child of themodalBox, it needs to listen to~ 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 and have to bind an explicit keydownevent handler on.modalbox-inner.~ Since
RB.DialogViewis a child of themodalBox, it needs to listen~ to keydownevents 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 eventsmap for thekeydownevent and have to bind an explicit+ keydownevent handler on.modalbox-inner. - Testing Done:
-
New unit tests:
~ - triggering enter with primary button disabled does nothing ~ - triggering enter with primary button enabled triggers the button ~ - triggering esc hides the dialog ~ - triggering submitwith primary button enabled triggers the button~ - triggering submitwith primary button disabled on a dialog does~ nothing + - triggering submitwith primary button enabled on a dialog with a+ form does nothing + - triggering keydownusing theEscapekey hides the dialog~ Manual testing:
~ - pressing enter right after opening the upload file dialog (when the ~ upload button is disabled) does nothing ~ - pressing enter after uploading a file using the dialog uploads the file ~ - pressing enter after uploading a file & editing the caption uploads the ~ file with the caption ~ - pressing esc after opening the dialog hides the dialog ~ Manual testing:
~ ~ File upload dialog:
~ - pressing Enterafter opening the dialog and not filling out any~ fields does nothing ~ - pressing Enterafter uploading a file & editing the caption uploads~ the file with the caption + - pressing Escapeafter opening the dialog hides the dialog+ + New review request dialog:
+ - pressing Escapeafter opening the dialog hides the dialog - Commit:
-
9079a675bf0ea1e96913a2a965b799cc76fb67629da1afeb8f43fda1ba655b3b355ed8caf1e23550
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Commit:
-
9da1afeb8f43fda1ba655b3b355ed8caf1e23550f099346a9d375f96e916a6b23f4f469820275f4c
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
