Moving a sponsored tile in newtab breaks various things

VERIFIED FIXED in Firefox 42

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: hoosteeno, Assigned: mzhilyaev)

Tracking

(Blocks 1 bug)

Trunk
Firefox 44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 verified, firefox43 verified, firefox44 verified)

Details

Attachments

(3 attachments)

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
Blocks: 1176364
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.
(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.
(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.
I would guess this is caused by bug 1145428.
Blocks: 1145428
Blocks: 1189754
No longer blocks: 1176364, 1145428
Blocks: 1197340
No longer blocks: 1189754
Assignee: nobody → mzhilyaev
Attachment #8664472 - Flags: review?(mcaceres)
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?
Attachment #8664472 - Flags: review?(mcaceres) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/c2c326eff429
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
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
Attachment #8668810 - Flags: approval-mozilla-aurora?
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]:
Attachment #8668813 - Flags: approval-mozilla-beta?
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.
Attachment #8668810 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
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
QA Whiteboard: [testday-201501002]
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.
Attachment #8668813 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.