1673429 - When sponsored tiles are present, dragging and dropping tiles adds duplicates, empty spaces, messes up the order

Status: VERIFIED FIXED

(Firefox :: Top Sites, defect, P1)

Description

[Pablo]

Attachment: orderbug.jpg

Topsites shows Duplicates and empty spaces and wrong order
when i try to re organize them

settings
bucket : main-preview
Dump: false
region: US
useremotesettings: true

Tested on
Windows10 64bit

Firefox
Nightly 84.0a1 and Beta 83.0b4

Steps to Reproduce
(see screenshot attached)
-launch firefox (beta or nightly)

• go to about:preferences#home and set row 3 for topsites
• open new tab
• Remove the reddit topsite
• Add new topsite "playstation" , "www.playstation.com" >IT gets added as 3rd position
• Move SONY topsite to first place > firefox moves sony to 3rd place
• Move SONY topsite to first place again > firefox makes first place empty
• Move empty 1st place to last position

Expected result
The order of the topsites should be what the user wants it to be without duplicates
being shown or without empty spaces.

Actual result
When i reorder the topsites i see empty spaces, or duplicates being shown
this seems to be intermitent. When i restart i see the sony topsite on the last position +1, with empty spaces,
and the playstation topsite on another position that was not the one that it showed previous to the restart

Comment 1

[Pablo]

added Dao to cc list.

Comment 2

[Pablo]

Severity would be S2

Comment 3

[Julien Cristau [:jcristau]]

Dao, do you have an update on this? We're about to build the last beta for 83.

Comment 4

[Dão Gottwald [:dao]]

Attachment: Bug 1673429 - Fix drag & drop of and around sponsored tiles. r=mikedeboer

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84f00e5c4201
Fix drag & drop of and around sponsored tiles. r=mikedeboer

Comment 6

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=84f00e5c420186016a527e4a65387a8e5e574e6b&selectedTaskRun=G4FHhQZpSISZih9lrSEtKw.0&searchStr=Linux%2C18.04%2Cx64%2Copt%2CNode%2Ctests%2Csource-test-node-newtab-unit-tests%2Cnewtab

Failure log: https://treeherder.mozilla.org/logviewer?job_id=320840326&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/1876c61824ff0a35b31a08436250553595e07779

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a56dd6b0b57
Fix drag & drop of and around sponsored tiles. r=mikedeboer

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4a56dd6b0b57

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 9185719 [details]
Bug 1673429 - Fix drag & drop of and around sponsored tiles. r=mikedeboer

Beta/Release Uplift Approval Request

• User impact if declined: See comment 0
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Patch is fairly big, but the upside is that there's little risk that it would change anything when there are no sponsored tiles which is the default for 83.
• String changes made/needed:

Comment 11

[Pascal Chevrel:pascalc]

Comment on attachment 9185719 [details]
Bug 1673429 - Fix drag & drop of and around sponsored tiles. r=mikedeboer

We have no beta left, this patch just landed on Nightly, was not verified by QA yet, has no automated tests and is a sizable change. This feels too risky to take directly in our RC build, sorry.

Comment 12

[Pascal Chevrel:pascalc]

Comment on attachment 9185719 [details]
Bug 1673429 - Fix drag & drop of and around sponsored tiles. r=mikedeboer

Let's posptpone the decision about it on QA work on this on a try build on beta.

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9185719 [details]
Bug 1673429 - Fix drag & drop of and around sponsored tiles. r=mikedeboer

Approved for RC, thanks.

Comment 14

[Adrian Florinescu [:aflorinescu]]

(In reply to Pascal Chevrel:pascalc from comment #12)

Let's posptpone the decision about it on QA work on this on a try build on beta.

As previously stated in the #search-qa, QA tested this uplift in an 83 beta try-build while considering if the above requested uplift improves user experience as compared with the pre-uplift version and also comparing with the current behavior as observed for a topsites experiment enrolled user. In this regard, QA asses that we are better off with the uplifted version, at least for the current status qvo: topsites modernization pref'd off by default.

The remaining issues identified during today's beta 83 trybuild test run have been broken down and logged separately and the next steps would be: investigation, triage and fixing (see #see also).

Based on the above, marking 83 as verified, while for 84 pending additional follow-up on the remaining bugs.

:dao, given that we are uplifting this bug, for 84 do you prefer that the newly logged bugs are handled on their own, or should they actually block this one?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/ff9e93fc2370

Comment 16

[Dão Gottwald [:dao]]

(In reply to Adrian Florinescu [:aflorinescu] from comment #14)

:dao, given that we are uplifting this bug, for 84 do you prefer that the newly logged bugs are handled on their own, or should they actually block this one?

Using See Also is fine.

Comment 17

[Adrian Florinescu [:aflorinescu]]

Re-verified and smoke tested a bit around this fix with 83 RC2 (83.0 2020-11-12) on Windows 10, Mac 10.14 and Ubuntu 20.04 and with the noted bugs in the see also, I didn't see anything different that on the original tests on the 83b try build.

Given the decision to move as-is with Fx83 due to the top sites experiments and given that for Fx84 is uncertain at this point what shall be fixed, I'm going to mark this bug as verified, the follow-ups making more sense to be followed-up separately.

Note: The 84 fixed and not verified flag is by design, hoping that this would make some sense.