1196437 - Moving a sponsored tile in newtab breaks various things

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Justin Crawford [:hoosteeno] [:jcrawford]]

I'm attaching a screencast of some unusual behavior spotted in Nightly. 

Steps to reproduce:
1) Move a sponsored tile

What happens:
1) "Sponsored" tag above tile disappears
2) All tiles become immovable
3) Hover state of all tiles stops appearing when hovering over them

What should happen:
Moving sponsored tiles should work just like any other tile.


http://screencast.com/t/lpoKuS0j

Comment 1

[Ed Lee :Mardak]

You're pinning tiles when you drag them around. A pinned sponsored tile becomes a history tile because you made it part of your new tab page.

The hover states does seem to be a bug.

Comment 2

[Justin Crawford [:hoosteeno] [:jcrawford]]

(In reply to Ed Lee :Mardak from comment #1)
> You're pinning tiles when you drag them around. A pinned sponsored tile
> becomes a history tile because you made it part of your new tab page.

Pinning is a reasonable explanation for the "Sponsored" to disappear. I'm not sure I agree that dragging should equal pinning. I expected to click on the pin for that. Random UX feedback from a bug reporter. :)

> The hover states does seem to be a bug.

That and the other tiles becoming immovable.

Comment 3

[Ed Lee :Mardak]

(In reply to Justin Crawford [:hoosteeno] [:jcrawford] from comment #2)
> I'm not sure I agree that dragging should equal pinning.
That's a fair concern. It's been like that for quite some time now where dragging a tile means you want it pinned at the destination.

(In reply to Justin Crawford [:hoosteeno] [:jcrawford] from comment #2)
> (In reply to Ed Lee :Mardak from comment #1)
> > The hover states does seem to be a bug.
> That and the other tiles becoming immovable.
An initial look seems to indicate these are all related to newtab-grid staying as locked=true.

Comment 4

[Ed Lee :Mardak]

I would guess this is caused by bug 1145428.

Comment 5

[maxim zhilyaev]

Attachment: v1. nightly patch

Comment 6

[Marcos Caceres [:marcosc]]

Comment on attachment 8664472 [details] [diff] [review]
v1. nightly patch

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

LGTM

::: browser/base/content/newtab/sites.js
@@ +49,5 @@
>  
>    /**
>     * Pins the site on its current or a given index.
>     * @param aIndex The pinned index (optional).
> +   * @return true if link changed type after pin

Nit - Should be: 
* @return Boolean true if link changed type after pin.

We have a code checker on the remote-newtab side that will complain otherwise.

::: toolkit/modules/NewTabUtils.jsm
@@ +411,3 @@
>      this.links[aIndex] = aLink;
>      this.save();
> +    return changed;

Q: where does AllPages.update() happen now? Or does it not need to happen?

Comment 7

[maxim zhilyaev]

(In reply to Marcos Caceres [:marcosc] from comment #6)

> ::: toolkit/modules/NewTabUtils.jsm
> @@ +411,3 @@
> >      this.links[aIndex] = aLink;
> >      this.save();
> > +    return changed;
> 
> Q: where does AllPages.update() happen now? Or does it not need to happen?

it happens when site handles a regular pin, the drop will do update on its own:
https://bugzilla.mozilla.org/attachment.cgi?id=8664472&action=diff#a/browser/base/content/newtab/sites.js_sec4

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c2c326eff429

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2c326eff429

Comment 10

[maxim zhilyaev]

Attachment: 1196437.aurora.v1.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1196437 - Moving a sponsored tile in newtab breaks various things
[User impact if declined]:
Severe, see Bug 1210091 that is one of the possible failure cases of the conditions causing Bug 1196437
[Describe test coverage new/current, TreeHerder]:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9825fbeffc5
- manual testing
[Risks and why]: 
unable to find failures during manual tests, and treeherder shows no failures related to the changes.  Assume minimal risk - certainly much less then keeping this bug in the code  
[String/UUID change made/needed]:
None

Comment 11

[maxim zhilyaev]

Attachment: 1196437.beta.v1.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1196437 - Moving a sponsored tile in newtab breaks various things
[User impact if declined]:
Severe. See Bug 1210091 that is one of the possible failure cases of the conditions causing Bug 1196437.
[Describe test coverage new/current, TreeHerder]:
TreeHerder in beta seems to be quite falty, so I ran two trys: one with and one without the patch.  They seem to show same failures which make me think that the patch is not implicated. I also verified that test failures are not directly related to the patch (not new-tab tests).  Both links are below:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=80d0fc5e542a
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f00a0756697

Also tested manually - no apparent issues found.
[Risks and why]: 
unable to find failures during manual tests. treeHerder suggests that the patch does not introduce any extra test failures. I suggest moderate risk.
[String/UUID change made/needed]:

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8668810 [details] [diff] [review]
1196437.aurora.v1.patch

OK, the patch is pretty small. I am going to take that to aurora, wait a few days and take in beta if no regression.

Comment 13

[Saheda Reza [:Antora]]

I have successfully reproduced this bug on Firefox Nightly Version 43.0a1 (2015-08-19)

It's fixed and verified on Latest Nightly
Build ID 	20151002030204
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:44.0) Gecko/20100101 Firefox/44.0

Tested OS- Windows 8.1 32bit

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6bafad1876b5

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8668813 [details] [diff] [review]
1196437.beta.v1.patch

5 days with aurora with it, taking it in beta. Should be in 42 beta 5.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a341315c0c57

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a341315c0c57

Comment 18

[Mihai Boldan, Desktop QA [:mboldan]]

I was able to reproduce the issue on Windows 10 x86 using an affected build - Firefox 43.0a1 (20150818030209) and the STR from Comment 0.
The issue is no longer reproducible on Windows 10 x86, Mac OS X 10.10.4 and Ubuntu 14.04 x86, using Firefox 44.0a1 (20151013030225), Firefox 43.0a2 (20151014004030) and Firefox 42.0b6 (20151012151721).
I'm marking this bug VERIFED-FIXED, since the disappearing of the "Sponsored" tag above tile is not a bug.