812291 - Work - Hide and restore sites from startUI

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect)

Description

[Allison Naaktgeboren :ally]

per Asa's user stories, users should be able to hide embarrassing or undesirable sites, at the very least from their recent history or topsites, on the startUI

Comment 1

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

We will ultimately need the tilegroup widget (794028) but we can fake it for now. Most of the work is the code behind the interactions which stores the hidden-ness, and an option (prefs/options?) to unhide all hidden sites - which I believe was what we agreed on rather than per-site unhide.

Comment 2

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

Attachment: Hook up hide/restore of Top Sites

Apply on top of the latest patch from https://bugzilla.mozilla.org/show_bug.cgi?id=812291, this patch implements the hideSite and restoreSite methods of our TopSites singleton. As tiles are being removed/added I don't use the dirty/refresh mechanism but call populateGrid to rebuild the grid. 

Note that we don't have UI currently for restore site. I believe the plan is to make this available somehow in the options panel? You can do it manually by editing the value in browser.newtabpage.blocked. The tests verify that TopSites.restoreSite does work however.

This patch can't land before that of #812291. I think it validates the approach taken for pin/unpin somewhat?

Comment 3

[Matt Brubeck (:mbrubeck)]

Comment on attachment 727668 [details] [diff] [review]
Hook up hide/restore of Top Sites

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

(In reply to Sam Foster [:sfoster] from comment #2)
> Note that we don't have UI currently for restore site. I believe the plan is
> to make this available somehow in the options panel?

The "restore" button should appear in the context appbar after hiding a site, according to extension *.a in attachment 703488 [details].  We can add that in a follow-up if you want.

Comment 4

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

Attachment: Hook up hide/restore of Top Sites

I've reworked this patch to include the restore behavior. 
Needs the MozAppbarDismissing event from the patch on #855590. 

I refactored a little to have the appbar populate the context actions toolbar in response to MozContextActionsChange events. The thing where selection drives context actions was just one case, as blocked tiles are removed and don't feature in the grid's selectedItems. This way, our undo/restore sites action can publish a MozContextActionsChange event, with the list of actions as a '.actions' payload, and the right thing happens. 

We have new contextual actions graphics on #836387, which I'll update seperately and get clarification on which icon to use for restore. 

Tests updated and pass for me. You still need to remove the hidden flag in browser.xul to test.

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 732792 [details] [diff] [review]
Hook up hide/restore of Top Sites

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

Looks good; see below for some minor cleanup suggestions.

::: browser/metro/base/content/TopSites.js
@@ +177,4 @@
>  
>      switch (aActionName){
>        case "delete":
> +        let removed = this._lastSelectedSites || (this._lastSelectedSites = []);

Is the "removed" local needed, or can we just use "this._lastSelectedSites"?

@@ +201,5 @@
>          Array.forEach(selectedTiles, function(aNode) {
>            let site = TopSites._linkFromNode(aNode);
>            let index = Array.indexOf(aNode.control.children, aNode);
> +          aNode.contextActions.delete('pin');
> +          aNode.contextActions.add('unpin');

Note that this change does *not* go through the setter, so it doesn't update the data-contextactions attribute, so it can get blown away by the next refresh() call.  I'm not sure if we need to worry about this in practice -- maybe not.

@@ +215,5 @@
>          });
>          break;
>        // default: no action
>      }
> +    if (needUndo) {

We could get rid of the "needUndo" variable and just do "if (nextContextActions.size)"

::: browser/metro/base/content/bindings/grid.xml
@@ +108,5 @@
>        <property name="contextActions">
>          <getter>
>            <![CDATA[
>              // return the subset of verbs that apply to all selected tiles
>              // use cached list, it'll get cleared out when selection changes

Remove the "used cached list" comment.

@@ +599,5 @@
>          <setter>
>            <![CDATA[
> +            let actions = ("string" == typeof val) ? actions.split(/[,\s]+/) : val;
> +            this._contextActions = new Set(actions);
> +            this.setAttribute("data-contextactions", [...this._contextActions].join(','));

Since I don't think we use this setter, I wonder if we should just remove it (making the property read-only)...  This code looks fine though if you have a use case for keeping it.

Comment 6

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

I removed the richgriditem contextActions setter and addressed the other comments. Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b06af6ce99

(oops, no r=mbrubeck in the commit msg)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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