Add shift+click selection support to ILE #8441

mheiman · 2023-10-19T19:01:27Z

This PR implements support for using shift+click to select/deselect ranges of items in the ILE.

There are different models for how shift+click selection works in interfaces, particularly with regard to how intermediate toggled items are handled; e.g. If a list is unselected except for item 3, and you click on 1 to select it and then shift+click on 5 to extend the selection, does 3 switch states or remain selected? I judged that the most common use case would be selecting or deselecting all the items in a range, and implemented accordingly.

The shift+click function requires two clicks, one to establish the starting point and the action (select/deselect) and one (with the shift key) to indicate the end of the range. Ranges may be selected in either direction. If the shifted click is a different action than the previous click, no range will be set (e.g. item 5 is already selected; you click to select item 1, then shift+click to deselect item 5. Only item 5 is affected)

Because the ILE remembers selections, a user might load a list that already has item 1 selected, and attempt to shift+click on item 5 to extend the range. Accommodating that use case introduces the possibility of other unwanted behaviors, so I did not address it.

Technical

Lists of selectable items in OL don't follow any consistent HTML pattern, so in order to determine the range of selectable items on various pages, I've had to do some on-the-fly tree traversal. It works in all current contexts, and probably will in any new ones that show up.

Shift+clicking on a web page often triggers extended text selections which are unwelcome in this context. It's possible to disable text selection altogether, but that's obviously not appropriate. After some lengthy searching, I was unable to come up with a solution that worked cross-browser to disable selections only in a shift+click context. As a second-best solution, this code clears any text selections that are accidentally created. You may see a brief flash of the selection in the interface.

Testing

This should be tested in the following contexts:
Author pages ( /OL18319A/Mark_Twain )
Work pages ( /books/OL7652865M/Adventures_of_Tom_Sawyer_(Webster's_French_Thesaurus_Edition )
Book search ( /search?q=a )
Author search ( /search/authors?q=t* )

Stakeholders

@cdrini @mheiman

codecov-commenter · 2023-10-19T19:03:56Z

Codecov Report

Attention: 40 lines in your changes are missing coverage. Please review.

Comparison is base (ed4b30b) 16.62% compared to head (925eee1) 15.91%.
Report is 172 commits behind head on master.

Files	Patch %	Lines
.../js/ile/utils/SelectionManager/SelectionManager.js	4.76%	31 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8441      +/-   ##
==========================================
- Coverage   16.62%   15.91%   -0.71%     
==========================================
  Files          84       86       +2     
  Lines        4433     4731     +298     
  Branches      758      837      +79     
==========================================
+ Hits          737      753      +16     
- Misses       3212     3443     +231     
- Partials      484      535      +51

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cdrini

Sweet thanks @mheiman !! A few small pieces of feedback 👍

openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js

cdrini · 2023-11-02T17:10:25Z

openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js

            ($(clickEvent.target).closest('a').is('a') &&
            $(clickEvent.target).not('.ile-select-handle').length > 0)) return;

        const el = clickEvent.currentTarget;
+        const elIndex = $(el).closest('li,tr')?.index();
+
+        if (clickEvent.shiftKey && this.lastSelectedIndex !== -1 && Math.abs(elIndex - this.lastSelectedIndex) > 1) {


Hmm we could have a default lastSelected here if one is missing ; default to the closest sibling element that's selected. That should handle the case where selection are present on page load, but we can't determine what the 'last' selected element was.

This is more complicated than it seems, particularly when you can shift+click either above or below selected elements, and you can shift+click to either select or deselect a range. I think trying to guess the right element to prepopulate here will end up wrong more often than not.

cdrini · 2023-11-02T17:47:41Z

openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js

+            } else {
+                affectedHandles = allHandles.slice(elIndex, this.lastSelectedIndex);
+            }
+            const stateChange = el.classList.contains('ile-selected') ? 'deselect' : 'select';


I think this behaviour is a little odd, I think I would expect it to select everything inbetween if there are any unselected in the range. So something like const hasUnselectedBetween = $(el).prevUntil(this.lastSelected).filter(':not(.ile-selected)')

Based on this comment, I don't think you're accounting for the fact that you can use shift+click to modify a range either before or after the last element clicked, and that you can use that shift-click to either select or deselect in bulk. That's the complexity that makes this code complicated. We have to compare the first and second clicks to find the elements between them, either forward or backward, and determine whether this is a selection or deselection action. I think if you play with it a little it will make more sense.

video1880256056.mp4

A few corrections to the video:

00:31 "It'll deselect everything" ----> "It'll deselect these two selected items"

01:27 "I think it should only deselect if everything I'm shift-clicking over is already selected" (not deselected :P)

And to clarify there are a few ways we could implement this, but the Google Photos approach felt rather natural to me (except for the part where it randomly stopped working :P). I think the logical bit is: everything is relative to the last clicked item. It'll add to the selection by default. It'll only remove from the selection if there's nothing else it can do. I'm surprised there isn't just like a known solution to how this should be implemented! Or maybe I just haven't found it.

I see what you're getting at now. My logic was to change the state based on the first click, but your preference (and Google's) is to change the state based on the second click. I can live with that. I've changed the two lines required to switch the behavior.

cdrini

Niiiice this solution looks great! A few small code changes other lgtm :)

openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js

cdrini · 2023-11-16T23:16:05Z

openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js

+        if ((forceSelected === true && isCurSelected) || (forceSelected === false && !isCurSelected)) return;
+        this.setElementSelectionAttributes(el, !isCurSelected);


That boolean logic is a touch confusing;

Suggested change

if ((forceSelected === true && isCurSelected) || (forceSelected === false && !isCurSelected)) return;

this.setElementSelectionAttributes(el, !isCurSelected);

this.setElementSelectionAttributes(el, forceSelected == null ? !isCurSelected : forceSelected);

And set the default to be null

Your solution is doing something different (that doesn't work :) My confusing logic is there to handle any elements in the range that don't need to be modified because they're already flipped the way they should be. In that case, we just want to bail out of the method because otherwise the element will be incorrectly double-added or removed from the selection set.

The logic is IF (element state is already correct) return ELSE do all the toggle operations.

After staring at it for a bit, I realized I could simplify the test to if (isCurSelected === forceSelected) return; which may be a little less obscure.

Nice! Ideally we'd make setElementSelectionAttributes work when called multiple times, but considering that out of scope of this pr!

openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js

cdrini · 2023-11-16T23:33:45Z

openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js

+            } else {
+                affectedHandles = allHandles.slice(elIndex, this.lastSelectedIndex);
+            }
+            const stateChange = el.classList.contains('ile-selected') ? 'deselect' : 'select';


video1880256056.mp4

A few corrections to the video:

00:31 "It'll deselect everything" ----> "It'll deselect these two selected items"

01:27 "I think it should only deselect if everything I'm shift-clicking over is already selected" (not deselected :P)

And to clarify there are a few ways we could implement this, but the Google Photos approach felt rather natural to me (except for the part where it randomly stopped working :P). I think the logical bit is: everything is relative to the last clicked item. It'll add to the selection by default. It'll only remove from the selection if there's nothing else it can do. I'm surprised there isn't just like a known solution to how this should be implemented! Or maybe I just haven't found it.

Co-authored-by: Drini Cami <cdrini@gmail.com>

cdrini

Ok! One last change ; Google photos is doing something slightly different ; the code for it is pretty simple and I think it's a clearer experience! Code otherwise lgtm, will put on testing.openlibrary.org after the change!

cdrini · 2023-11-20T17:12:42Z

openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js

+                const stateChange = this.lastClicked.classList.contains('ile-selected') ? true : false;
+                for (const element of affectedElements) this.toggleSelected(element, stateChange);


Suggested change

const stateChange = this.lastClicked.classList.contains('ile-selected') ? true : false;

for (const element of affectedElements) this.toggleSelected(element, stateChange);

// Select the affected elements if any are unselected

const select = affectedElements.some(el => !el.classList.contains('ile-selected'));

for (const element of affectedElements) this.toggleSelected(element, select);

This suggestion seems to be based on the assumption that we're only selecting ranges, but my solution implements deselecting a range with shift+click as well, which isn't something that Google Photos does.

I think you're trying to do something that's already implemented, only in toggleSelected rather than here.

Google photos implements range deselection with shift-click, but only if everything in the range is selected. Let's jump on a call in a bit to hammer through this 😁 Thanks for your patience!

cdrini

Talked over the options on a call ; we're still not aligned on what the best ux is 😁 But I'm happy to roll with what we have here and see how it works! Putting on testing, code lgtm

Add shift+click selection support

b03a9a2

cdrini self-assigned this Oct 19, 2023

cdrini reviewed Nov 2, 2023

View reviewed changes

Mark F. Heiman added 2 commits November 3, 2023 21:44

Reworked based on review comments.

7d9ab28

SImplified logic in getSiblingSet

7b5c9a3

cdrini added the Priority: 1 Do this week, receiving emails, time sensitive, . [managed] label Nov 6, 2023

cdrini reviewed Nov 16, 2023

View reviewed changes

cdrini added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Nov 16, 2023

mheiman and others added 4 commits November 17, 2023 19:03

Add return documentation

ed7f878

Co-authored-by: Drini Cami <cdrini@gmail.com>

Clarify setting logic in toggleSelected

1bb11de

Co-authored-by: Drini Cami <cdrini@gmail.com>

rename getSiblingSet to getSelectableRange

68f517d

Co-authored-by: Drini Cami <cdrini@gmail.com>

Changes addressing review comments

925eee1

cdrini removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Nov 20, 2023

cdrini requested changes Nov 20, 2023

View reviewed changes

cdrini approved these changes Nov 20, 2023

View reviewed changes

jimchamp mentioned this pull request Nov 20, 2023

work doesn't get selected when "Want to Read" button is pressed #8529

Merged

cdrini merged commit 9835fae into internetarchive:master Nov 24, 2023
3 checks passed

cdrini changed the title ~~Add shift+click selection support~~ Add shift+click selection support to ILE Nov 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shift+click selection support to ILE #8441

Add shift+click selection support to ILE #8441

mheiman commented Oct 19, 2023

codecov-commenter commented Oct 19, 2023 •

edited

cdrini left a comment

cdrini Nov 2, 2023

mheiman Nov 4, 2023

cdrini Nov 2, 2023

mheiman Nov 3, 2023 •

edited

cdrini Nov 16, 2023 •

edited

mheiman Nov 18, 2023

cdrini left a comment

cdrini Nov 16, 2023

mheiman Nov 18, 2023

cdrini Nov 20, 2023

cdrini Nov 16, 2023 •

edited

cdrini left a comment •

edited

cdrini Nov 20, 2023

mheiman Nov 20, 2023

cdrini Nov 20, 2023

cdrini left a comment

		if ((forceSelected === true && isCurSelected) \|\| (forceSelected === false && !isCurSelected)) return;
		this.setElementSelectionAttributes(el, !isCurSelected);

	if ((forceSelected === true && isCurSelected) \|\| (forceSelected === false && !isCurSelected)) return;
	this.setElementSelectionAttributes(el, !isCurSelected);
	this.setElementSelectionAttributes(el, forceSelected == null ? !isCurSelected : forceSelected);

		const stateChange = this.lastClicked.classList.contains('ile-selected') ? true : false;
		for (const element of affectedElements) this.toggleSelected(element, stateChange);

-                const stateChange = this.lastClicked.classList.contains('ile-selected') ? true : false;
-                for (const element of affectedElements) this.toggleSelected(element, stateChange);
+                // Select the affected elements if any are unselected
+                const select = affectedElements.some(el => !el.classList.contains('ile-selected'));
+                for (const element of affectedElements) this.toggleSelected(element, select);

Add shift+click selection support to ILE #8441

Add shift+click selection support to ILE #8441

Conversation

mheiman commented Oct 19, 2023

Technical

Testing

Stakeholders

codecov-commenter commented Oct 19, 2023 • edited

Codecov Report

cdrini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheiman Nov 3, 2023 • edited

Choose a reason for hiding this comment

cdrini Nov 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrini Nov 16, 2023 • edited

Choose a reason for hiding this comment

cdrini left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrini left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 19, 2023 •

edited

mheiman Nov 3, 2023 •

edited

cdrini Nov 16, 2023 •

edited

cdrini Nov 16, 2023 •

edited

cdrini left a comment •

edited