867163 - Defect - Restore tile button doesn't work when context app bar is visible on a second time

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Browser, defect, P1)

Description

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20130429 Firefox/23.0

1. Select a tile/group of tiles from Top Sites
2. Remove that tile by clicking the "Hide" button from context app bar
3. Right click on Awesome screen to make context app bar disappear
4. Right click again to re-make context app bar visible
5. Click on "Restore" button

Expected result:
Tile/group of tile should be restored.

Found while verifying bug 831918.

Comment 1

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

I think the bug here is that once the context app bar is dismissed the first time, the restore button should go away. The intention is that you just get the one shot at the "undo hide" action.

Comment 2

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

I had the 'restore not going away' issue when working on bug 831916 too. I fixed it by sending a "MozContextActionsChange" event when the appbar dismisses. I'm not sure if that's the best way or if the appbar should be smarter about it. See https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bookmarks.js?force=1#361

Comment 3

[Matt Brubeck (:mbrubeck)]

I think the appbar should automatically clear itself.

Comment 4

[Marco Mucci [:MarcoM]]

Matt's Point Estimate = 1.

Comment 5

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

Point estimate = 1

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Any time the appbar dismisses, automatically clear the selection and the context actions.

Comment 8

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

Comment on attachment 745973 [details] [diff] [review]
patch

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

Looks good and works well. Thanks for picking this up.

::: browser/metro/base/content/appbar.js
@@ +210,5 @@
>      });
>    },
> +
> +  clearContextualActions: function() {
> +    this.showContextualActions([]);

Its a bit weird that we have to send a magical empty array to clear contextual actions (I know, I did this) but it works and we can refine later if the need arises

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c39614efd263

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Marco Mucci [:MarcoM]]

For testing and verification.

Comment 12

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" and everything passed without any issues during Testing/Verification. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-05-16-03-10-04-mozilla-central/

- Selected several tiles from "Top Sites" and removed them. Selected "Restore" and ensured that all the tiles that have been removed have been restored correctly
- Selected all of the tiles from "Top Sites" and removed them. Selected "Restore" and ensured that all the tiles that have been removed have been restored correctly
- Removed several tiles from "Top Sites" and then dismissed the "App Bar", ensured that you cannot restore the tiles as per Comment 1
- Ensured that if you have a large list of tiles under "Top Sites", the removed ones are replaced with others without any issues
- Ensured that if you remove all of the tiles from "Top Sites" and they are replaced with other tiles, selecting "restore" will restore the other ones without any issues
- Ensured that when you remove several tiles from "Top Sites" and then close Firefox Metro, the removed tiles are not reloaded when re-opening Firefox Metro

Comment 13

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130819030205
Built from http://hg.mozilla.org/mozilla-central/rev/c8c9bd74cc40

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment12 and got expected result except for one. I have filed bug 906910.