Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent unnecessary deselect/select when an already selected item is selected #1449

Merged
merged 1 commit into from
Apr 7, 2020
Merged

Prevent unnecessary deselect/select when an already selected item is selected #1449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions Selection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,12 @@ define([
// except that clicks/keystrokes without modifier keys will clear
// the previous selection.

// If the target is already selected, and is the sole selection, ignore a user action
// that would simply select the target (causing unnecessary deselect/select).
if (!event[ctrlEquiv] && this.isSelected(target) && this.getSelectedCount() === 1) {
Copy link
Contributor

@maier49 maier49 Apr 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the line below where we're checking for the control key the logic used is
!(event.keyCode ? event.ctrlKey : event[ctrlEquiv])

Should we be replicating the same check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of why that logic would be used anywhere. It only makes sense to me if:

  1. Mac browsers typically don't supply a value for event.keyCode
  2. There is a specific browser for which has('mac') is true, and that browser does supply a value for event.keyCode, and instead of event.metaKey it sets event.ctrlKey when the Command key is pressed

I haven't found any discussion of ctrlKey/metaKey that justifies this logic. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I don't know of any situation where that makes sense.

return;
}

// Clear selection first for right-clicks outside selection and non-ctrl-clicks;
// otherwise, extended mode logic is identical to multiple mode
if (event.button === 2 ? !this.isSelected(target) :
Expand Down Expand Up @@ -643,6 +649,18 @@ define([
this._fireSelectionEvents();
},

getSelectedCount: function () {
var count = 0;

for (var id in this.selection) {
if (id) {
count += 1;
}
}

return count;
},

isSelected: function (object) {
// summary:
// Returns true if the indicated row is selected.
Expand Down
1 change: 1 addition & 0 deletions doc/components/mixins/Selection.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ Method | Description
`selectAll()`| Programmatically selects all rows in the component. Note that only rows that have actually been loaded will be represented in the `selection` object.
`clearSelection()`| Programmatically deselects all rows in the component.
`isSelected(row)`| Returns `true` if the given row is selected.
`getSelectedCount()` | Returns the number of selected rows/cells.

**Note:** The `select`, `deselect`, and `isSelected` methods can be passed any
type of argument acceptable to List's `row` method; see the [List](../core-components/List.md) APIs for
Expand Down