1008144 - Avoid reflows in dragdrop

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Comment 1

[Cristian Rodriguez (:crdlc)]

Attachment: Github pull request

Thanks in advance mate

Comment 2

[Kevin Grandon :kgrandon]

Hey Cristian - No objections to most of this. I would just like to have some understanding as to why we are reflowing - I feel like we weren't before, and it's a new thing? I think it may be because we update the textContent of the element now, is that correct?

I was just wondering if we should do that instead with some setter and leave the render function alone. I'm starting to wonder if our methods might get confusing if we now have all of: render, setPosition, transform, and now rearrange. (I know most of this is certainly not your fault!)

I think we added this here: https://github.com/crdlc/gaia/commit/75f40537a998f071a585c843ece9625cb79d3d9a#diff-d612e45a7d0193d39a053f21292d53d6R97

Should we consider adding this to the update() call instead?

Comment 3

[Kevin Grandon :kgrandon]

Attachment: Alternative approach, thoughts?

Hey Cristian - here is an alternative approach, wanted to see what you thought. I think we can still put some of your optimizations in regarding array vs objects, but perhaps going with this more simple one would be an option?

I'm not tied to any approach, but if you think creating a new method is better, we can go with that as well.

Comment 4

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8420210 [details] [review]
Alternative approach, thoughts?

It only works for bookmarks sorry

Comment 5

[Cristian Rodriguez (:crdlc)]

I gonna modify my pr with the update method instead of rearrange

Comment 6

[Cristian Rodriguez (:crdlc)]

You can review my changes

1) I have followed your suggestion removing the rearrange method, I agreed with you!
2) The title is updated in "update" method as you suggested
3) Avoid "document.body.dataset.cols = layout.perRow" when we are dragging because it is just needed after zoom and so we avoid a reflow
4) This patch moves icons which are between origin and target of the movement. I mean, for example, if you drag the icon 3rd to 6th position, only icons placed in 3rd, 4th, 5th and 6th positions are translated. With this code we save a lot of time when we have tons of dividers, icons, etc... trying to move all of them when it is not needed
5) Coordinates were an object and now are an array, no significant but better performance

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8420057 [details]
Github pull request

Hey Cristian - This is looking really good, the only thing I noticed is that implementing the from/to has had some regression when moving icons over icons in the last row. I think what happens is that we don't animate the icons in time, they then get stuck in a "stale" position, and if you drag back up, they stay in their new position and do not revert.

Here's some basic STR:
1 - Create a new group at the bottom with only a single icon.
2 - Scroll up.
3 - Drag icon from the top of a page down to the bottom.
4 - Move the icon over the last icon, notice that it shifts to the right.
5 - Quickly drag your finger up the screen to leave the bottom divider.
6 - Notice that the bottom icon remains shifted to the right, and does not return to the proper left position.

Sorry, this one is a bit difficult to explain and I can get a video of the phone if necessary. One option may be to remove the from/to handling and do this in another patch.

Comment 8

[Cristian Rodriguez (:crdlc)]

OK, I gonna remove the from/to parameters

Comment 9

[Cristian Rodriguez (:crdlc)]

OK done, I can work on it during this week, I mean from/to parameters in another bug

Comment 10

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8420057 [details]
Github pull request

thanks for the good catch

Comment 11

[Kevin Grandon :kgrandon]

Thanks - I'll take a look soon. Another thing we need to do is implement some delay before the rearrange happens I think. E.g., we should wait some 200-300 ms for the user to hold over some spot before we perform a re-arrange. Perhaps this could be part of the same work?

Comment 12

[Cristian Rodriguez (:crdlc)]

OK, I can look that too, 10x

(In reply to Kevin Grandon :kgrandon from comment #11)
> Thanks - I'll take a look soon. Another thing we need to do is implement
> some delay before the rearrange happens I think. E.g., we should wait some
> 200-300 ms for the user to hold over some spot before we perform a
> re-arrange. Perhaps this could be part of the same work?

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8420057 [details]
Github pull request

Thanks!!

Comment 14

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/1f7fe039e57e130e90c51f6497c1e59fcae1a88c

Comment 15

[Kevin Grandon :kgrandon]

Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.