Move items between zones in assessment mode #84

mtyaka · 2016-07-25T05:07:12Z

Description

This PR implements the following use case:

As a learner, I want to be able to move items between drop targets before checking (submitting) my answer in assessment mode. This should be possible for any item, regardless of whether I placed it on a correct or incorrect zone before. I also want to be able to drag items back to the item bank without having to use the reset button.

UX

The existing implementation does not allow learners to move an item to a different zone once it has been placed on the board. We will keep this behavior for drag-and-drop problems in standard mode.

In assessment mode, learners should be able to move items that have already been placed on the board, regardless of whether they are located in a correct or incorrect zone. To do this, learners will be able to use the same workflows that are already established for moving items from the item bank to the board:

Items can be moved to a different zone using the mouse. Workflow:
1. Pick item up by clicking it.
2. Drag item to desired zone.
3. Drop item.
Items can be moved to a different zone using the keyboard. Workflow:
1. (Shift-)TAB to item.
2. Pick item up by pressing "Enter", "Space", "Ctrl-m", or "⌘-m".
3. TAB to desired zone.
4. Drop item by pressing "Enter", "Space", "Ctrl-m", or "⌘-m".
5. Press "Esc" to cancel drop operation.

With no item selected, learners will be able to (Shift-)TAB to items that have been placed on the board.

Once an item has been picked up, pressing (Shift-)TAB will only navigate between zones and the item bank.

JIRA tickets: SOL-1943

Discussions: Discovery document for DnDv2 assessment mode (relevant section: 2.2.2)

Dependencies: None

Sandbox URLs (installed version de0312b):

DnD example is located at http://dndv2-sandbox5.opencraft.hosting/courses/course-v1:OpenCraftX+OC1+1/courseware/bdeaa08976fa4645a277edcaf5e8148c/e8ec2f5c98f4436abf532cccd6a71668/1?activate_block_id=block-v1%3AOpenCraftX%2BOC1%2B1%2Btype%40vertical%2Bblock%40e3c62b19090948b8ad89d3386bff4435

Testing instructions:

Add a DnDv2 block to a unit in Studio.
Click "EDIT" and change "Problem mode" to "assessment".
Drag items from the bank into drop zones. You may place some items in wrong zones.
Drag and drop already placed items between zones, both with mouse and keyboard.
Observe that the items "stick" in the placed position (are not moved back to the bank or previous zone). Observe that per-item feedback is not displayed.
Drag some items back to the bank with both mouse and keyboard.
Change "Problem mode" back to "standard".
Make sure everything still works correctly in standard mode.

Author notes and concerns:

If you put multiple items into the same zone with keyboard in assessment mode, you can only see the topmost item, the rest of the items are hidden behind it (the same thing can happen when dragging with the mouse, but when dragging with the mouse you usually don't drop the item in exactly the same place, so parts of overlapping items are usually still visible).

Using the existing zone alignment feature (where you can specify how dropped items automatically align inside zones) prevents this problem.

Reviewers

OpenCraft/Solutions: @haikuginger
TNL: @cahrens and/or @staubina
a11y: @edx/edx-accessibility
Product: @sstack22

haikuginger · 2016-07-27T12:06:03Z

tests/integration/test_interaction.py

-        self.interact_with_keyboard_help(use_keyboard=use_keyboard)
+    # @data(False, True)
+    # def test_keyboard_help(self, use_keyboard):
+    #     self.interact_with_keyboard_help(use_keyboard=use_keyboard)


Is there a reason we've got several tests commented out here?

@haikuginger There isn't, thanks for catching this. I commented them out while debugging a specific test and then forgot to uncomment them :/.

haikuginger · 2016-07-27T13:55:33Z

So, I've checked this out - all tests pass locally, and the interaction definitely works as intended. I do have a small interaction concern, though - there's no way to remove an item from the board once it's been placed, besides pressing the reset button. In general, this isn't a huge issue, except when it comes to decoy items.

Right now, when an item is dropped on an area that isn't an active zone, it jumps back to where it was placed before. I kind of wonder if they should jump back to the palette instead. Either that, or we should possibly make the palette a full zone that we can drag to in its own right.

mtyaka · 2016-07-27T14:08:05Z

@haikuginger Thanks a lot for the review. That's a very good point. I think it would make sense to be able to drag items back to the bank.

bradenmacdonald · 2016-07-27T15:46:36Z

drag_and_drop_v2/public/js/drag_and_drop.js

-                configuration.items[i].grabbed = grabbed;
+    var setGrabbedState = function(item_id, grabbed, interaction_type) {
+        for (var i = 0, item; i < configuration.items.length; i++) {
+            item = configuration.items[i];


Nit: Just use configuration.items.forEach(function(item) { which is nicer and used elsewhere in the code.

Thanks, changed.

bradenmacdonald · 2016-07-27T17:50:25Z

Yes, one should be able to drag items back to the bank.

If you put multiple items into the same zone with keyboard in assessment mode, you can only see the topmost item, the rest of the items are hidden behind it

Can we use the existing "alignment" code to avoid this? It already has an algorithm for moving items around within a zone so that they don't overlap, if I remember correctly.

mtyaka · 2016-07-28T13:51:38Z

Can we use the existing "alignment" code to avoid this? It already has an algorithm for moving items around within a zone so that they don't overlap, if I remember correctly.

I was not familiar with this feature, but it seems to be working well and prevents the problem.

mtyaka · 2016-07-28T14:22:22Z

@haikuginger I implemented ability to drag items back to the bank, please take a look.

haikuginger · 2016-07-28T19:31:43Z

👍

I tested this: I ran automated tests locally and confirmed all tests passed. I also installed the XBlock in a devstack and manually tested the interaction. Buttons can now be dragged between targets in Assessment Mode, and they can also be dragged back to the item bank.
I read through the code
Includes documentation

mtyaka · 2016-07-31T10:17:58Z

Rebased cd7e92e onto master

cahrens · 2016-08-01T16:22:07Z

When I used the keyboard to move items, I ended up with "Goes to the top" completely behind "Goes to the middle". Probably keyboard moves always go to the same place?

And actually, I can reproduce this with the mouse as well.

Keyboard reproduction steps:

Using the keyboard, move "Goes to the top" into the top third of the triangle.
Using the keyboard, move any of the longer items into the top third of the triangle.

cahrens · 2016-08-01T16:44:14Z

drag_and_drop_v2/public/js/drag_and_drop.js

+        }
+        var style = bankItemWidthStyles(item, ctx);
+        // Placeholder should never be visible.
+        style.visibility = 'hidden';


They will be invisible to screen readers as well?

@cahrens Yes, elements with visibility: hidden are ignored by screen readers.

cahrens · 2016-08-01T17:08:49Z

Just a few questions (see above), and I noted a situation in which an item can get completely hidden behind another element.

bradenmacdonald · 2016-08-01T18:55:14Z

@cahrens The problem with overlapping drop targets was a pre-existing issue AFAIK, and after it was brought up on another PR I created https://openedx.atlassian.net/browse/SOL-1979 - so we will deal with it in an upcoming PR.

mtyaka · 2016-08-02T04:19:47Z

@cahrens The item overlap issue will be addressed by https://openedx.atlassian.net/browse/SOL-1979. I addressed all of your other comments, please have a look.

staubina · 2016-08-02T12:04:42Z

My only concern is that focus goes to the bottom of the page when selecting a drop zone before selecting something to drag. Is this expected behavior? Could we instead focus on the problem again?

Other than that 👍

mtyaka · 2016-08-02T13:03:37Z

@staubina Thanks for the review! I believe that's the expected behavior. It has worked that way before the changes in this PR.

cahrens · 2016-08-02T13:57:58Z

👍 , though I would think that the scrolling behavior @staubina mentioned will have to be fixed at some point. If you don't think it makes sense to do as part of this PR, can you update an appropriate ticket with this information?

Note that the keyboard focus is correct (it's still on the zone you selected), just the spurious scroll occurs.

mtyaka · 2016-08-02T14:18:34Z

@staubina @cahrens Sorry, I think we might have been thinking about different issues. What kind of scrolling behavior are you seeing?

Is it when you press the space bar while the focus is on a zone without any item being selected?

cahrens · 2016-08-02T14:35:28Z

@mtyaka That's what I was talking about. @staubina were you talking about the same thing?

staubina · 2016-08-02T14:37:14Z

@cahrens @mtyaka Yes that is the issue I was referring to.

mtyaka · 2016-08-02T15:28:00Z

@cahrens @staubina I see. Scrolling the page down when you press space bar is the default behavior in most browsers. When you pick up an item we hijack the space bar from the browser so that it drops the item rather than scroll the page.

When no item is picked up, space bar is not hijacked so pressing it produces the standard scroll down effect. I can see why this can be distracting while working on a Drag and Drop exercise, especially if you think you've grabbed an item when you haven't, so I think it would make sense to hijack the space bar every time one of the zones is focused, regardless of whether an item has been picked up or not.

I will check to see whether that's easy to do. If easy, I'll add it to this PR, if not I'll create a separate ticket.

bradenmacdonald · 2016-08-02T21:05:10Z

@mtyaka I think it's simply a matter of calling preventDefault() or similar in the event handler, to stop the spacebar event from bubbling up to the body and causing the page scroll.

mtyaka · 2016-08-03T04:10:37Z

Thanks for the suggestion @bradenmacdonald, preventDefault() did it.

@cahrens @staubina I added some code to prevent default browser scroll from happening when pressing space bar while focused on a zone with no item selected.

mtyaka · 2016-08-03T10:24:51Z

@sstack22 Please review when you get a chance.

staubina · 2016-08-03T11:14:31Z

@mtyaka Thanks for making that change. Did you update the Sandbox?

👍

mtyaka · 2016-08-03T11:48:02Z

@staubina The sandbox hasn't been updated yet, I'll do it in a couple of minutes.

mtyaka · 2016-08-03T12:33:20Z

@staubina Sandbox has been updated.

staubina · 2016-08-03T12:54:11Z

Looks good to me
👍

sstack22 · 2016-08-03T15:21:03Z

@mtyaka - a couple small notes:

When you move an item after having placed it, the text disappears and there is only a loading sign. Is it possible to have the text appear as well (similar to the initial drop?)
I was able to drop an item on the left side background. If I try to drop an item on the right side, the item is automatically placed in the related zone. I would have expected the same behavior for the left side background.

Otherwise, looks good.

mtyaka · 2016-08-03T16:45:21Z

Thanks for the review @sstack22!

When you move an item after having placed it, the text disappears and there is only a loading sign. Is it possible to have the text appear as well (similar to the initial drop?)

Thanks for pointing that out, I never noticed the way text temporarily disappeared. It is now fixed and the sandbox has been updated with the fix.

I was able to drop an item on the left side background. If I try to drop an item on the right side, the item is automatically placed in the related zone. I would have expected the same behavior for the left side background.

The way we currently determine whether the item was dropped into a zone is by looking at the position of mouse pointer at the time you release the item. If the mouse pointer is inside a zone, the drop is successful, but if the mouse pointer is outside the zone, the drop is reverted.
So when you dropped the item to the left edge, you were probably holding it somewhere on the right side, so that when you released it, mouse pointer was still inside the zone. It is easier to understand the behavior when the zone is highlighted:

sstack22 · 2016-08-03T17:22:59Z

@mtyaka - thanks for the clarification! I confirmed the fix for #1. This looks great, 👍

mtyaka · 2016-08-04T03:45:28Z

Squashed from 30173c4

In standard mode, once you drop an item into the correct zone, you can no longer move it until you reset the exercise. In assessment mode, we want users to be able to freely move items between zones until they explicitly submit their solution. This commit implements the ability to move dropped items between zones in assessment mode. It does not make any changes to standard mode.

Child elements of items can get removed/reordered, so we have to mark each of them children with a unique key for virtual dom to be able to reorder them correctly. The absence of keys was causing some weird glitches where the content would disappear while the item spinner was visible.

mtyaka force-pushed the mtyaka/assessment-movable-items branch from c097fdd to efdb838 Compare July 25, 2016 08:05

mtyaka changed the title ~~WIP: Move items between zones in assessment mode~~ Move items between zones in assessment mode Jul 27, 2016

haikuginger reviewed Jul 27, 2016
View reviewed changes

bradenmacdonald reviewed Jul 27, 2016
View reviewed changes

mtyaka force-pushed the mtyaka/assessment-movable-items branch from 5496a36 to cd7e92e Compare July 28, 2016 13:58

mtyaka force-pushed the mtyaka/assessment-movable-items branch from cd7e92e to 062bac8 Compare July 31, 2016 10:17

cahrens reviewed Aug 1, 2016
View reviewed changes

mtyaka force-pushed the mtyaka/assessment-movable-items branch 2 times, most recently from 777eb07 to b53246c Compare August 2, 2016 05:11

staubina mentioned this pull request Aug 2, 2016

[SOL-1944] Submit Answer UI #86

Merged

4 tasks

mtyaka force-pushed the mtyaka/assessment-movable-items branch from 2b11b55 to ddb9fd3 Compare August 4, 2016 03:45

mtyaka force-pushed the mtyaka/assessment-movable-items branch from ddb9fd3 to 165fae0 Compare August 4, 2016 04:25

mtyaka added 2 commits August 4, 2016 12:55

mtyaka force-pushed the mtyaka/assessment-movable-items branch from 165fae0 to ed661bf Compare August 4, 2016 04:55

mtyaka merged commit 2afee49 into openedx:master Aug 4, 2016

mtyaka deleted the mtyaka/assessment-movable-items branch August 4, 2016 05:13

Move items between zones in assessment mode #84

Move items between zones in assessment mode #84

Conversation

mtyaka commented Jul 25, 2016 • edited Loading

haikuginger Jul 27, 2016

Choose a reason for hiding this comment

mtyaka Jul 27, 2016

Choose a reason for hiding this comment

haikuginger commented Jul 27, 2016

mtyaka commented Jul 27, 2016

bradenmacdonald Jul 27, 2016

Choose a reason for hiding this comment

mtyaka Jul 28, 2016

Choose a reason for hiding this comment

bradenmacdonald commented Jul 27, 2016

mtyaka commented Jul 28, 2016

mtyaka commented Jul 28, 2016

haikuginger commented Jul 28, 2016

mtyaka commented Jul 31, 2016

cahrens commented Aug 1, 2016 • edited Loading

cahrens Aug 1, 2016

Choose a reason for hiding this comment

mtyaka Aug 2, 2016

Choose a reason for hiding this comment

cahrens commented Aug 1, 2016

bradenmacdonald commented Aug 1, 2016

mtyaka commented Aug 2, 2016

staubina commented Aug 2, 2016

mtyaka commented Aug 2, 2016

cahrens commented Aug 2, 2016

mtyaka commented Aug 2, 2016

cahrens commented Aug 2, 2016

staubina commented Aug 2, 2016

mtyaka commented Aug 2, 2016

bradenmacdonald commented Aug 2, 2016

mtyaka commented Aug 3, 2016

mtyaka commented Aug 3, 2016

staubina commented Aug 3, 2016

mtyaka commented Aug 3, 2016

mtyaka commented Aug 3, 2016

staubina commented Aug 3, 2016

sstack22 commented Aug 3, 2016

mtyaka commented Aug 3, 2016

sstack22 commented Aug 3, 2016

mtyaka commented Aug 4, 2016

mtyaka commented Jul 25, 2016 •

edited

Loading

cahrens commented Aug 1, 2016 •

edited

Loading