Closed Bug 1028330 Opened 6 years ago Closed 4 years ago

[Vertical Homescreen] Use center coordinates for drag/drop

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: jlorenzo, Assigned: freddy03h)

References

Details

(Whiteboard: [systemsfe])

Attachments

(3 files, 4 obsolete files)

STR
1. Long press the leftmost empty space of a group (which is not the last)
2. Install a new Smart Collection
3. Verify that the Smart Collection is installed where you taped
4. Delete the Smart Collection
5. Long press at the same place in step 1
6. Reinstall the same Smart Collection

Expected result
Smart Collection is installed where you taped as in step 3

Actual
The Smart Collection is place in the other group. See video for details.

Video Link: http://mzl.la/1yt9R2x
Inconsistent UX.
blocking-b2g: --- → 2.0?
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+]
If you add a different collection does the issue reproduce?
Keywords: qawanted
Does UX consider this as a blocker or nice to have?
Flags: needinfo?(firefoxos-ux-bugzilla)
Attached patch 1028330.patch (obsolete) — Splinter Review
Use the center of an item to calculate the nearest item index (getNearestItemIndex) instead of use the x and y

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=3371f2de6d8c
Attachment #8444083 - Flags: review?(kgrandon)
Attached patch 1028330.patch (obsolete) — Splinter Review
fix a regression bug when we move an app on edit mode

https://github.com/mozilla-b2g/gaia/pull/20859
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=4cb7884c8fc2
Attachment #8444127 - Flags: review?(kgrandon)
Duplicate of this bug: 1028693
Comment on attachment 8444127 [details] [diff] [review]
1028330.patch

Review of attachment 8444127 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, it would be better to obsolete one patch and upload a new one. I'll take a look, thanks!
Attachment #8444127 - Flags: review?(kgrandon)
Attached file Pull request (obsolete) —
Oh, it looks like there's a github pull request? Let's just review that, thanks!
Attachment #8444083 - Attachment is obsolete: true
Attachment #8444127 - Attachment is obsolete: true
Attachment #8444083 - Flags: review?(kgrandon)
Attachment #8444171 - Flags: review?(kgrandon)
Comment on attachment 8444171 [details] [review]
Pull request

Clearing review for now due to the unit test failures. I think we would rather be testing this with integration tests, so perhaps this would be a good time to remove the failing test if we can achieve the same coverage with integration tests.

This is the test that's failing: https://github.com/mozilla-b2g/gaia/blob/master/apps/sharedtest/test/unit/elements/gaia_grid/grid_dragdrop_test.js#L49


https://travis-ci.org/mozilla-b2g/gaia/jobs/28164625
Attachment #8444171 - Flags: review?(kgrandon)
Freddy, do you have some time to fix the failing unit tests ?

You can find a tutorial to write unit tests at https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests

If you look at https://tbpl.mozilla.org/?tree=Gaia-Try&rev=4cb7884c8fc2 there is an orange 'G'. When you click on it, you will see the name of the unit tests that are failing. 

I suggest that you fix the test locally, following the tutorial I linked. Every time you will edit the test file and save the change, the test runner will re-rerun the test of this file for you (if you have launched ./bin/gaia-test).

The 'Gu' test that is failing is not your fault, so you can ignore this one.

Kevin, I suggest that we fix those unit tests, and open a followup for the integration tests. Is that OK for you ?
Flags: needinfo?(freddy03h)
Assignee: nobody → freddy03h
Flags: needinfo?(freddy03h)
Oups. Still need the needinfo from Freddy.
Flags: needinfo?(freddy03h)
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+] → [VH-FL-blocking-][VH-FC-blocking?]
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #10)
> Kevin, I suggest that we fix those unit tests, and open a followup for the
> integration tests. Is that OK for you ?

I think it's fine by me, it's likely the new usage of clientWidth/clientHeight that is throwing off desktop. I looked through the test code and tbh I don't think it is covering that much useful stuff, so I thought it may be easier to write a new integration test than save it.

Let's see if Freddy has any luck with the unit test, if not we can probably rip it out as long as we file a follow-up to add a better integration test.
Flagging Jacqueline on the UX questions.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(jsavory)
Attached file Pull Request
Use gridItemWidth and gridItemHeight layout properties in the getNearestItemIndex method instead of clientWidth and clientHeight

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=8fd9530fbdd4
Attachment #8444859 - Flags: review?(kgrandon)
Flags: needinfo?(freddy03h)
Attachment #8444171 - Attachment is obsolete: true
Comment on attachment 8444859 [details] [review]
Pull Request

Thank you for your patch. I tried this on a device today and I'm noticing a less-than-optimal experience when icons reflow around the currently dragged icon. I'm not sure exactly what the cause is, but my assumption is that we need to adjust for the xAdjust/yAdjust numbers somewhere.

Will attach a screenshot of what I'm seeing.
Attachment #8444859 - Flags: review?(kgrandon)
The reporters bug does NOT repro on: Flame 2.1 Master, Flame 2.0, OpenC 2.1 master, OpenC 2.0, Buri 2.1, Buri 2.0

Actual Results: Adding then deleting and re-adding a collection will place the reinstalled collection in the expected location.



Environmental Variables:
Device: Flame Master
Build ID: 20140624090716
Gaia: 2944695a89c3281d49dbe5ff9c0cd26c8318e2ba
Gecko: 215b93e07e1d
Version: 33.0a1 (Master)
Firmware Version: v122
-----------------------------------------
Environmental Variables:
Device: Flame 2.0
Build ID: 20140624093150
Gaia: 7260d58fb2b4665ebe614f94d822b8407bd95f58
Gecko: 23457787dfa1
Version: 32.0a2 (2.0)
Firmware Version: v122
-----------------------------------------
Environmental Variables:
Device: Open_C Master
Build ID: 20140624090716
Gaia: 2944695a89c3281d49dbe5ff9c0cd26c8318e2ba
Gecko: 215b93e07e1d
Version: 33.0a1 (Master)
Firmware Version: P821A10V1.0.0B06_LOG_DL
-----------------------------------------
Environmental Variables:
Device: Open_C 2.0
Build ID: 20140624093150
Gaia: 7260d58fb2b4665ebe614f94d822b8407bd95f58
Gecko: 23457787dfa1
Version: 32.0a2 (2.0)
Firmware Version: P821A10V1.0.0B06_LOG_DL
----------------------------------------
Environmental Variables:
Device: Buri Master
Build ID: 20140624090716
Gaia: 2944695a89c3281d49dbe5ff9c0cd26c8318e2ba
Gecko: 215b93e07e1d
Version: 33.0a1 (Master)
Firmware Version: v1.2device.cfg
-----------------------------------------
Environmental Variables:
Device: Buri 2.0
Build ID: 20140624133052
Gaia: 7260d58fb2b4665ebe614f94d822b8407bd95f58
Gecko: 55155b4af80d
Version: 32.0a2 (2.0)
Firmware Version: v1.2device.cfg
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking?][QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Contact: croesch
(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #2)
> If you add a different collection does the issue reproduce?

I was unable to repro with any collection.
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?][QAnalyst-Triage?] → [VH-FL-blocking-][VH-FC-blocking?][QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Johan - Can you retest here? The QAnalysts can't reproduce the bug here.
Flags: needinfo?(jlorenzo)
Not able to reproduce either anymore.
Status: NEW → RESOLVED
blocking-b2g: 2.0? → ---
Closed: 6 years ago
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?][QAnalyst-Triage+] → [VH-FL-blocking-][VH-FC-blocking-][QAnalyst-Triage+]
Flags: needinfo?(jlorenzo)
Resolution: --- → WORKSFORME
Removing flag as bug is resolved
Flags: needinfo?(jsavory)
The original report talk about a bug for a re-installation. I didn't see anything like this in the source code.

But while testing, I found that when we press at the bottom of a placeholder, the smart collection isn't created in the right place (see attachment).

It's because the "getNearestItemIndex" function take into account X and Y of the gridItem. And this origin point is located on the top left corner of the item.

Doing the center of the element the origin point, solve the bug I describe and I think it's what the user expect.

But it causes other bugs for the drag and drop I've tried to solve.
Thanks Freddy - I agree that fixing this should improve the drag/drop usability slightly. It would be good to fix this, though we need to fix the icon reflowing issue that I mentioned above.

Re-opening to track this effort, and renaming the bug to match what we're doing here.
Blocks: vertical-home-next
No longer blocks: 1015336
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: [Vertical Homescreen] App is not at the same place after a direct reinstallation → [Vertical Homescreen] Use center coordinates for drag/drop
Assignee: freddy03h → chrislord.net
Attachment #8451527 - Flags: review?(kgrandon)
Sorry, I confused some of my bugmail and took this mistakenly - Assigning it back. If that's ok, treat my pull request as a feedback comment - the patch is almost identical :)
Assignee: chrislord.net → freddy03h
Comment on attachment 8451527 [details] [review]
Use the center of the item when calculating the nearest grid item

I'd like to fold in any updates to Freddy's original PR here if possible, as they are quite similar in nature.

(I'm still seeing the reflowing regression with this pull request as well)
Attachment #8451527 - Attachment is obsolete: true
Attachment #8451527 - Flags: review?(kgrandon)
A tip with the missing rearranging, GridDragDrop offsets the coordinates by an amount, I assume to counteract this bug(?) Search for 'Testing with some extra offset' in grid_dragdrop.js - if we don't want to get rid of that, we can at least compensate for it in positionIcon, where the call to getNearestItemIndex is made.
Sorry about that, I think this is leftover from the prototyping days :)

I believe the idea was that you want to adjust the icon so you can see it better as you drag it around. I think we probably need to continue to make those adjustments, but take that adjustment into account for the center position calculation.
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-][QAnalyst-Triage+] → [VH-FL-blocking-][VH-FC-blocking-][QAnalyst-Triage+][lead-review+]
Mass update: Resolve wontfix all issues with legacy homescreens.

As of 2.6 we have a new homescreen and having these issues open is confusing. All issues will block bug 1231115 so we can use that to re-visit any of these if needed.
Blocks: 1231115
Status: REOPENED → RESOLVED
Closed: 6 years ago4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.