Chris Lord [:cwiiis] Assignee
	Description • 10 years ago

If you drag a collapsed over the first row of icons, it will highlight, and dropping it will place it above that group.

While this doesn't feel weird, it should only highlight when dragged to the top of the container (at which point the blue indicator expands to indicate it can be dropped, as well as the group highlighting).

	Chris Lord [:cwiiis] Assignee
	Comment 2 • 10 years ago

Comment on attachment 8565472 [details]
[gaia] Cwiiis:bug1131039-fix-group-drag-first-row-highlight > mozilla-b2g:master

Fix bug, add unit test for it.

	Kevin Grandon :kgrandon
	Comment 3 • 10 years ago

Comment on attachment 8565472 [details]
[gaia] Cwiiis:bug1131039-fix-group-drag-first-row-highlight > mozilla-b2g:master

This looks good to me, and we can probably land it, though I would like some clarification on the intended behavior before we do. Right now it feels more inconsistent than what's on master. Take the following example:

[Group 1]
[Group 2]
[Group 3]

If I long tap on Group 2, it immediately turns blue.
If I drag it up to where it overlaps groups 1, it loses the highlight.
If I drag it back down, and over group 2, it's now blue again.

With the patch applied, dragging the group around seems very inconsistent in highlighting, so I'm not sure if we should land as-is. Let me know what you think, thanks!

	Chris Lord [:cwiiis] Assignee
	Comment 4 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #3)
> Comment on attachment 8565472 [details]
> [gaia] Cwiiis:bug1131039-fix-group-drag-first-row-highlight >
> mozilla-b2g:master
> 
> This looks good to me, and we can probably land it, though I would like some
> clarification on the intended behavior before we do. Right now it feels more
> inconsistent than what's on master. Take the following example:
> 
> [Group 1]
> [Group 2]
> [Group 3]
> 
> If I long tap on Group 2, it immediately turns blue.
> If I drag it up to where it overlaps groups 1, it loses the highlight.
> If I drag it back down, and over group 2, it's now blue again.
> 
> With the patch applied, dragging the group around seems very inconsistent in
> highlighting, so I'm not sure if we should land as-is. Let me know what you
> think, thanks!

What you describe is the expected behaviour - it should only turn blue if letting it go at that point would make the group stay there (this is the same behaviour as is on master, to my knowledge, just you can erroneously drop it on the first row of icons).

This means it turns blue when it's over its original position, when it's over one of the blue drop-indicator lines, or when it's at the very top of the container (where a blue line will also grow into position to demonstrate where the group will land).

Do you see anything that doesn't conform to this?

	Kevin Grandon :kgrandon
	Comment 5 • 10 years ago

Hey Chris - I think I'm still a bit confused then, going by "it should only turn blue if letting it go at that point would make the group stay there".

With the patch applied, when I drag a middle group ~50% above the top group, it loses the highlighting. Letting go returns it to the original position. Should it be blue in this case?

When I drag over the bottom group 50%, the group remains highlighted, and the indicator after the last group activates. Letting go places the group under the bottom group, though I would assume it should return to the original position because it's blue?

I'm seeing this on a flame, and these two cases together make it seem fairly inconsistent.

	Chris Lord [:cwiiis] Assignee
	Comment 6 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #5)
> Hey Chris - I think I'm still a bit confused then, going by "it should only
> turn blue if letting it go at that point would make the group stay there".
> 
> With the patch applied, when I drag a middle group ~50% above the top group,
> it loses the highlighting. Letting go returns it to the original position.
> Should it be blue in this case?
> 
> When I drag over the bottom group 50%, the group remains highlighted, and
> the indicator after the last group activates. Letting go places the group
> under the bottom group, though I would assume it should return to the
> original position because it's blue?
> 
> I'm seeing this on a flame, and these two cases together make it seem fairly
> inconsistent.

This is expected - it's blue in its original position because that's a valid drop position - it goes grey when dropping it would not be a valid drop, so then it goes back to its original position too. (I think this is in the spec, but I'm happy to be corrected). So whenever it turns blue, dropping it would leave it in that position. When it's grey, dropping it would leave it in its original position.

You could certainly argue that it should be grey in its default position (although then you could argue "why isn't it blue when if I drop it here, it stays here?") - One for UX I think, if you want to take it further :)

	Kevin Grandon :kgrandon
	Comment 7 • 10 years ago

Comment on attachment 8565472 [details]
[gaia] Cwiiis:bug1131039-fix-group-drag-first-row-highlight > mozilla-b2g:master

Ok, I think the code is fine, but the interaction feels *wrong* to as it seems very inconsistent. If you're happy with this, feel free to land it, but I would personally seek to make this more consistent, and update the spec if needed.

	Chris Lord [:cwiiis] Assignee
	Comment 8 • 10 years ago

What do you think Tiffanie? I'm happy either way, the change wouldn't take much effort.

	Tiffanie Shakespeare [:tif]
	Comment 9 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #5)
> Hey Chris - I think I'm still a bit confused then, going by "it should only
> turn blue if letting it go at that point would make the group stay there".
> 
> With the patch applied, when I drag a middle group ~50% above the top group,
> it loses the highlighting. Letting go returns it to the original position.
> Should it be blue in this case?
> 
> When I drag over the bottom group 50%, the group remains highlighted, and
> the indicator after the last group activates. Letting go places the group
> under the bottom group, though I would assume it should return to the
> original position because it's blue?
> 
> I'm seeing this on a flame, and these two cases together make it seem fairly
> inconsistent.

Is this what you are talking about Kevin? On master, the behaviour is consistent with it turning blue in both instances.

Per usual my short video is too large, please find here: http://youtu.be/vTRFp6Q4mtE

	Kevin Grandon :kgrandon
	Comment 10 • 10 years ago

(In reply to Tiffanie Shakespeare from comment #9)
> Is this what you are talking about Kevin? On master, the behaviour is
> consistent with it turning blue in both instances.
> 
> Per usual my short video is too large, please find here:
> http://youtu.be/vTRFp6Q4mtE

Is that with the patch applied? It looks like the behavior is different for the top group and the bottom group, and it seems inconsistent to me.

	Tiffanie Shakespeare [:tif]
	Comment 11 • 10 years ago

Yep that video is what the patch does. Master has the same behaviour between the bottom and top groups. I agree, Kevin that the patch is inconsistent. Chris, can we make the top and bottom groups behave the same? (i.e. in the video make the top behave like the bottom) Thanks!

	Chris Lord [:cwiiis] Assignee
	Comment 12 • 10 years ago

ah, so the problem here is that it's picking up the group below too high up... I can probably tweak this, will try to sort it out.

	Chris Lord [:cwiiis] Assignee
	Comment 13 • 10 years ago

Comment on attachment 8565472 [details]
[gaia] Cwiiis:bug1131039-fix-group-drag-first-row-highlight > mozilla-b2g:master

So wow, I don't know why I couldn't see what you were talking about before, but as soon as I realised, I could see how bad it was...

Anyway, this is, I think, the proper fix. Rather than remove the behaviour of highlighting on the first row of icons, this just now treats that as inserting at the top and the top indicator highlights accordingly - this makes it much easier to insert a group at the top of an expanded group.

I think behaviour is pretty much consistent now, let me know if you spot anything awry.

	Tiffanie Shakespeare [:tif]
	Comment 14 • 10 years ago

Seems good - thanks!

	Kevin Grandon :kgrandon
	Comment 15 • 10 years ago

Comment on attachment 8565472 [details]
[gaia] Cwiiis:bug1131039-fix-group-drag-first-row-highlight > mozilla-b2g:master

This is looking better to me now, thank you!

It still feels a bit inconsistent to me though when dragging over a large group as it turns grey, but when moving it over the original spot it turns blue. Possibly something that we might want to take a look at after this lands.

	Chris Lord [:cwiiis] Assignee
	Comment 16 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #15)
> Comment on attachment 8565472 [details]
> [gaia] Cwiiis:bug1131039-fix-group-drag-first-row-highlight >
> mozilla-b2g:master
> 
> This is looking better to me now, thank you!
> 
> It still feels a bit inconsistent to me though when dragging over a large
> group as it turns grey, but when moving it over the original spot it turns
> blue. Possibly something that we might want to take a look at after this
> lands.

I agree after more playing that I think it would make sense for groups to only light up when their position is going to change - but let's deal with that in another (polish) bug :)

	GitHub Autolander Bot
	Comment 17 • 10 years ago

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/870a6c17bea4dd47ab0261ed626072161e8fa4ee

	Chris Lord [:cwiiis] Assignee
	Comment 18 • 10 years ago

Comment on attachment 8565472 [details]
[gaia] Cwiiis:bug1131039-fix-group-drag-first-row-highlight > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: Some slightly confusing behaviour when dragging groups to the top of the container
[Testing completed]: Pretty thorough manual testing, grouping functionality reasonably well covered by unit and marionette tests.
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: None

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 10 years ago

v2.2: https://github.com/mozilla-b2g/gaia/commit/87df14ff6c4f14533411c8a7450cce635d3f1dee

	Jayme Mercado [:JMercado] (Inactive 3/4/2016)
	Comment 20 • 10 years ago

This issue is verified fixed on the latest Nightly 3.0 and 2.2 builds
	
Actual results:  When dragging a collapsed group above the top group, there is now an indicator that the group will be placed above the top one.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150226010233
Gaia: 7894b929f1b0394f3c997f72a6482bc7813e758d
Gecko: dd6353d61993
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20150226002503
Gaia: bf24aa57fa7760260ab05d1f53242c8d8ae59e83
Gecko: 363123044e61
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0