Closed Bug 876033 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 25

People

(Reporter: TimAbraldes, Assigned: sfoster)

References

Details

(Whiteboard: feature=change c=firefox_start u=metro_firefox_user p=1)

Attachments

(3 files)

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.
Whiteboard: feature=change c=firefox_start u=metro_firefox_user p=0
Priority: -- → P3
I concur with the p=1. I'll take a look at this
QA Contact: sfoster
Whiteboard: feature=change c=firefox_start u=metro_firefox_user p=0 → feature=change c=firefox_start u=metro_firefox_user p=1
Assignee: nobody → sfoster
QA Contact: sfoster
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.
Attachment #776684 - Flags: review?(mbrubeck)
Makes use of the arranged richgrid event to reset selection on restored tiles, and ensure the context appbar is displayed as required
Attachment #776690 - Flags: review?(mbrubeck)
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.
Attachment #776693 - Flags: review?(mbrubeck)
Attachment #776684 - Flags: review?(mbrubeck) → review+
Attachment #776690 - Flags: review?(mbrubeck) → review+
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.
Attachment #776693 - Flags: review?(mbrubeck) → review?(rsilveira)
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.
Attachment #776693 - Flags: review?(rsilveira) → review+
Blocks: metrov1it11
No longer blocks: metrov1defect&change
Status: NEW → ASSIGNED
QA Contact: jbecerra
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
https://hg.mozilla.org/mozilla-central/rev/92a4fbaff6c4
https://hg.mozilla.org/mozilla-central/rev/d46e861a4203
https://hg.mozilla.org/mozilla-central/rev/df2ffeb51fc0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
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.
Status: RESOLVED → VERIFIED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.