Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1089163002:
Pressing 'Delete' key in bookmark manager's search box deletes bookmark. (Closed)
|
Created:
5 years, 8 months ago by Deepak Modified:
5 years, 8 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, tfarina, noyau+watch_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPressing 'Delete' key in bookmark manager's search box deletes bookmark.
SeparateHandler has been added to handle delete and undo commands when search box is active or focused.
BUG=462861
Committed: https://crrev.com/907b409a46af8efb63d83c46f8901d6f0d01c8fe
Cr-Commit-Position: refs/heads/master@{#325258}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : Changes as per review comments. #Patch Set 5 : #
Total comments: 1
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #
Total comments: 1
Messages
Total messages: 20 (2 generated)
deepak.m1@samsung.com changed reviewers: + bauerb@chromium.org
When document.activeElement is input element during focus on search box, we are returning bmm.list.selectedItems, that cause last selected item to delete. Changes done to check things properly. PTAL at this Change.
https://codereview.chromium.org/1089163002/diff/2/chrome/browser/resources/bo... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/1089163002/diff/2/chrome/browser/resources/bo... chrome/browser/resources/bookmark_manager/js/main.js:494: // (fixes http://crbug.com/278112). Otherwise, because (Read my comments bottom-up for context) Here is where we disable the undo command if the search field is active. BTW, another possible yak shaving, I mean improvement: Checking whether a command can run happens by dispatching an event on the active DOM element. If the search field is active, it would get the first chance to handle this event, so we could move these checks to a separate event handler that would be registered on the search field. https://codereview.chromium.org/1089163002/diff/2/chrome/browser/resources/bo... chrome/browser/resources/bookmark_manager/js/main.js:845: if (opt_target) { I don't really understand what's going on here. You are changing this method to return null if the target is null and there is no active element? That doesn't really match the JSDoc comment, which states that the method will never return null (per the exclamation mark in the return type; see http://usejsdoc.org/tags-type.html). Above that, it also says that the method should only be called if the list or the tree is focused, so that case shouldn't even be happening. Also, looking at it in a bigger context, wouldn't this mean that we would silently swallow a Delete key press instead of applying it to the text field? I think what we should instead do is not try to handle the delete command at all if the search field is active, similar to what we do for the undo command (see my other comment).
Thanks for review. I like your idea of silently swallow the delete event when we have search box active. Changes done. PTAL
On 2015/04/15 11:24:24, Deepak wrote: > Thanks for review. > I like your idea of silently swallow the delete event when we have search box > active. > Changes done. > PTAL Uh, what I meant was that we should *not* silently swallow the delete command once we are told to handle it, but instead refuse to handle it beforehand, so that the keypress event that triggered the command will be directly handled instead. It's a bit confusing because there are three different things which are handled via events, and the events are dispatched in a hierarchical way: a) the keyboard event generated by the browser b) the 'canExecute' event generated by handling (a), that determines whether the command can run c) the 'command' event that is dispatched iff (b) decided that the command can in fact run At (c), we are already expected to execute the command, so silently doing nothing would violate that expectation. What we should do here is handle (b) by saying that the command can *not* run, which will continue with dispatching the (a) event, so it will be handled by the text field itself.
On 2015/04/15 11:47:52, Bernhard Bauer wrote: > On 2015/04/15 11:24:24, Deepak wrote: > > Thanks for review. > > I like your idea of silently swallow the delete event when we have search box > > active. > > Changes done. > > PTAL > > Uh, what I meant was that we should *not* silently swallow the delete command > once we are told to handle it, but instead refuse to handle it beforehand, so > that the keypress event that triggered the command will be directly handled > instead. It's a bit confusing because there are three different things which are > handled via events, and the events are dispatched in a hierarchical way: > > a) the keyboard event generated by the browser > b) the 'canExecute' event generated by handling (a), that determines whether the > command can run > c) the 'command' event that is dispatched iff (b) decided that the command can > in fact run > > At (c), we are already expected to execute the command, so silently doing > nothing would violate that expectation. What we should do here is handle (b) by > saying that the command can *not* run, which will continue with dispatching the > (a) event, so it will be handled by the text field itself. Thanks for making my understanding better. I am making canExecute false when we have searchBox active. PTAL at new changes. Thanks
https://codereview.chromium.org/1089163002/diff/70001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/1089163002/diff/70001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/main.js:501: e.canExecute = e.currentTarget.activeElement !== $('term'); Right. This means however that we will always make a decision whether the delete command can execute. If the search field is not active, we want to delegate that decision to canExecuteForList() and canExecuteForTree() below, so you want to set canExecute (and break) only if the search field is active. (BTW, this is why I suggested moving this code to a separate handler on the search field: it removes the necessity of the activeElement check, and it avoids conflating different reasons for allowing a command to execute or not, which is what happens for the 'undo-command' case above.)
On 2015/04/15 12:51:16, Bernhard Bauer wrote: > https://codereview.chromium.org/1089163002/diff/70001/chrome/browser/resource... > File chrome/browser/resources/bookmark_manager/js/main.js (right): > > https://codereview.chromium.org/1089163002/diff/70001/chrome/browser/resource... > chrome/browser/resources/bookmark_manager/js/main.js:501: e.canExecute = > e.currentTarget.activeElement !== $('term'); > Right. This means however that we will always make a decision whether the delete > command can execute. If the search field is not active, we want to delegate that > decision to canExecuteForList() and canExecuteForTree() below, so you want to > set canExecute (and break) only if the search field is active. This part is clear to me. > (BTW, this is why I suggested moving this code to a separate handler on the > search field: it removes the necessity of the activeElement check, and it avoids > conflating different reasons for allowing a command to execute or not, which is > what happens for the 'undo-command' case above.) If I understood correctly then Here I need to define separate handler for searchbox something like: $('term').addEventListener('canExecute', handleCanExecuteForSearchBox); and in will handle 'delete-command' in that something like: function handleCanExecuteForSearchBox(e) { var command = e.command; switch (command.id) { case 'delete-command': // Do nothing when delete key is selected in search box. e.canExecute = false; break; } } please correct me if I miss understood something. Thanks
On 2015/04/15 14:15:04, Deepak wrote: > On 2015/04/15 12:51:16, Bernhard Bauer wrote: > > > https://codereview.chromium.org/1089163002/diff/70001/chrome/browser/resource... > > File chrome/browser/resources/bookmark_manager/js/main.js (right): > > > > > https://codereview.chromium.org/1089163002/diff/70001/chrome/browser/resource... > > chrome/browser/resources/bookmark_manager/js/main.js:501: e.canExecute = > > e.currentTarget.activeElement !== $('term'); > > Right. This means however that we will always make a decision whether the > delete > > command can execute. If the search field is not active, we want to delegate > that > > decision to canExecuteForList() and canExecuteForTree() below, so you want to > > set canExecute (and break) only if the search field is active. > > This part is clear to me. > > > (BTW, this is why I suggested moving this code to a separate handler on the > > search field: it removes the necessity of the activeElement check, and it > avoids > > conflating different reasons for allowing a command to execute or not, which > is > > what happens for the 'undo-command' case above.) > > If I understood correctly then > Here I need to define separate handler for searchbox something like: > $('term').addEventListener('canExecute', handleCanExecuteForSearchBox); > > and in will handle 'delete-command' in that something like: > > function handleCanExecuteForSearchBox(e) { > var command = e.command; > switch (command.id) { > case 'delete-command': > // Do nothing when delete key is selected in search box. > e.canExecute = false; > break; > } > } > > please correct me if I miss understood something. > Thanks Yeah, that's correct! Then you can also move the case for 'undo-command' when the search field is selected there.
Thanks for confirming my understanding. Changes done as suggested. PTAL
https://codereview.chromium.org/1089163002/diff/90001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/js/main.js (left): https://codereview.chromium.org/1089163002/diff/90001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/main.js:496: e.canExecute = e.currentTarget.activeElement !== $('term'); For the undo command we need to keep the behavior if the search field is _not_ selected. Thanks to the way events are propagated, this only gets invoked if the search field handler did not set canExecute, so you know that the search field is not selected, so you can set it unconditionally to true. You can also keep the second part of the comment (everything after "Otherwise"). https://codereview.chromium.org/1089163002/diff/90001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/1089163002/diff/90001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/main.js:480: // Otherwise, because the global undo command has no visible UI, always Remove this sentence (see my comment above).
Comments updated as suggested. PTAL. https://codereview.chromium.org/1089163002/diff/90001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/js/main.js (left): https://codereview.chromium.org/1089163002/diff/90001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/main.js:496: e.canExecute = e.currentTarget.activeElement !== $('term'); On 2015/04/15 14:54:45, Bernhard Bauer wrote: > For the undo command we need to keep the behavior if the search field is _not_ > selected. Thanks to the way events are propagated, this only gets invoked if the > search field handler did not set canExecute, so you know that the search field > is not selected, so you can set it unconditionally to true. You can also keep > the second part of the comment (everything after "Otherwise"). Done. https://codereview.chromium.org/1089163002/diff/90001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/1089163002/diff/90001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/main.js:480: // Otherwise, because the global undo command has no visible UI, always On 2015/04/15 14:54:45, Bernhard Bauer wrote: > Remove this sentence (see my comment above). Done.
Just phrasing nits: https://codereview.chromium.org/1089163002/diff/110001/chrome/browser/resourc... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/1089163002/diff/110001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/js/main.js:479: // Pass the undo command through (fixes http://crbug.com/278112). "Pass the delete and undo commands through". https://codereview.chromium.org/1089163002/diff/110001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/js/main.js:504: // If search box is not active and the global undo command has no visible You don't need to mention the part about the search field. Also, the undo command always has no visible UI, so that's not an "if". Just use the old phrasing without the "otherwise".
Comments updated. PTAL https://codereview.chromium.org/1089163002/diff/110001/chrome/browser/resourc... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/1089163002/diff/110001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/js/main.js:479: // Pass the undo command through (fixes http://crbug.com/278112). On 2015/04/15 15:08:56, Bernhard Bauer wrote: > "Pass the delete and undo commands through". Done. https://codereview.chromium.org/1089163002/diff/110001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/js/main.js:504: // If search box is not active and the global undo command has no visible On 2015/04/15 15:08:56, Bernhard Bauer wrote: > You don't need to mention the part about the search field. Also, the undo > command always has no visible UI, so that's not an "if". Just use the old > phrasing without the "otherwise". Done.
LGTM, thanks!
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1089163002/130001
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/907b409a46af8efb63d83c46f8901d6f0d01c8fe Cr-Commit-Position: refs/heads/master@{#325258}
Message was sent while issue was closed.
https://codereview.chromium.org/1089163002/diff/130001/chrome/browser/resourc... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/1089163002/diff/130001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/js/main.js:506: e.canExecute = e.currentTarget.activeElement !== $('term'); Wait, this should be unconditionally set to true (the current behavior is not wrong, but we know that the search field is not active, and that way this part of the code does not have to deal with the search field at all). |
