876033 - Change - After undoing the deletion of tiles, the undeleted tiles should be selected

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: App Bar, defect, P3)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

STR:
  1) Select 1 or more tiles
  2) Delete them by tapping the deletion icon
  3) Undo the deletion by pressing the undo button

At this point, I would expect to be in the exact same state as I was at step 1. I should have the same set of tiles selected and the same options available in the app bar.

Comment 1

[Brian R. Bondy [:bbondy]]

p=1

Comment 2

[Sam Foster [:sfoster] (he/him)]

I concur with the p=1. I'll take a look at this

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: Part 1 of 3: Add arranged event for richgrid, event cleanup

Patch 1 of 3. As arrangeItems can be async, raising an "arranged" event to explicitly mean the grid is done arranging turns out to be v. useful.

Comment 4

[Sam Foster [:sfoster] (he/him)]

Attachment: Part 2 of 3: Re-select topsites when we undo deletion

Makes use of the arranged richgrid event to reset selection on restored tiles, and ensure the context appbar is displayed as required

Comment 5

[Sam Foster [:sfoster] (he/him)]

Attachment: Part 3 of 3: Added arrranged event test for richgrid, refactor TopSites tests and test restore functionality

I found and fixed an issue in the TopSites tests, which we running before the UI was finished setting up. Along the way I refactored all the helpers into an object, similar to bookmarks tests. The delete/restore test is new, all pass locally.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 776693 [details] [diff] [review]
Part 3 of 3: Added arrranged event test for richgrid, refactor TopSites tests and test restore functionality

Needs parts 1, 2 in your queue to apply and for tests to pass.

Comment 7

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 776693 [details] [diff] [review]
Part 3 of 3: Added arrranged event test for richgrid, refactor TopSites tests and test restore functionality

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

Nice refactoring! Looking good, just some nits.

::: browser/metro/base/tests/mochitest/browser_topsites.js
@@ +138,5 @@
> +  updatePagesAndWait: function th_updatePagesAndWait() {
> +    let deferredUpdate = Promise.defer();
> +    let updater = {
> +      update: function() {
> +        dump("updatePagesAndWait: updater callback\n");

Use info instead.

@@ +177,5 @@
>    desc: "load and display top sites",
>    setUp: function() {
> +    yield TopSitesTestHelper.setup();
> +    let grid = document.getElementById("start-topsites-grid");
> +    let arrangedPromise = waitForEvent(grid, "arranged");

This is not being used, you're re-assigning to arrangedPromise below.

@@ +185,2 @@
>  
> +    let arrangedPromise = waitForEvent(document.getElementById("start-topsites-grid"), "arranged");

You can use the grid variable above or remove that declaration.

@@ +285,2 @@
>      yield waitForCondition(function(){
>        return !grid.controller.isUpdating;

Can this one be replaced by a wait for arranged event too? Same for https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/browser_topsites.js#311

@@ +323,1 @@
>      } catch(e) {

Nit: you don't need to try/catch on setup anymore, head.js will do that for you.

@@ +490,5 @@
> +    // those sites are blocked and their tiles have been removed from the grid?
> +    ok( !grid.querySelector('richgriditem[value="'+brianSite.value+'"]'));
> +    ok( !grid.querySelector('richgriditem[value="'+dougalSite.value+'"]'));
> +    ok( NewTabUtils.blockedLinks.isBlocked(brianSite), "Brian site was blocked" );
> +    ok( NewTabUtils.blockedLinks.isBlocked(dougalSite), "Dougal site was blocked" );

Should grid.selectedItems.length be 0? Probably a good check to add.

Comment 8

[Sam Foster [:sfoster] (he/him)]

I think I addressed all review comments. Double-checked tests as had to merge a couple times since. Landed on inbound: 

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/92a4fbaff6c4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d46e861a4203
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/df2ffeb51fc0

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92a4fbaff6c4
https://hg.mozilla.org/mozilla-central/rev/d46e861a4203
https://hg.mozilla.org/mozilla-central/rev/df2ffeb51fc0

Comment 10

[Manuela Muntean [Away]]

Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20130729 Firefox/25.0

Verified as fixed on the latest Nightly build, following the steps from the description. After the "undo" operation, the same set of tiles selected and the same options are available in the app bar.