Closed Bug 1008144 Opened 6 years ago Closed 6 years ago

Avoid reflows in dragdrop

Categories

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

All
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.0, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: crdlc, Assigned: crdlc)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files)

No description provided.
Assignee: nobody → crdlc
Status: NEW → ASSIGNED
Attached file Github pull request
Thanks in advance mate
Attachment #8420057 - Flags: review?(kgrandon)
Whiteboard: [systemsfe]
Target Milestone: --- → 2.0 S1 (9may)
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?
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.
Attachment #8420210 - Flags: review?(crdlc)
Comment on attachment 8420210 [details] [review]
Alternative approach, thoughts?

It only works for bookmarks sorry
Attachment #8420210 - Flags: review?(crdlc) → review-
I gonna modify my pr with the update method instead of rearrange
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 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.
Attachment #8420057 - Flags: review?(kgrandon)
Flags: needinfo?(crdlc)
OK, I gonna remove the from/to parameters
Flags: needinfo?(crdlc)
OK done, I can work on it during this week, I mean from/to parameters in another bug
Comment on attachment 8420057 [details]
Github pull request

thanks for the good catch
Attachment #8420057 - Flags: review?(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?
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 on attachment 8420057 [details]
Github pull request

Thanks!!
Attachment #8420057 - Flags: review?(kgrandon) → review+
Landed: https://github.com/mozilla-b2g/gaia/commit/1f7fe039e57e130e90c51f6497c1e59fcae1a88c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
feature-b2g: --- → 2.0
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
You need to log in before you can comment on or make changes to this bug.