Work - Hide and restore sites from startUI

RESOLVED FIXED in Firefox 23

Status

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: ally, Assigned: sfoster)

Tracking

Trunk
Firefox 23
x86
Windows 8.1
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Assignee: nobody → ally
Depends on: 800996

Updated

7 years ago
Whiteboard: [metro-mvp] [LOE:?][metro-it2] → [metro-mvp] [LOE:1][metro-it2]
(Reporter)

Updated

7 years ago
Whiteboard: [metro-mvp] [LOE:1][metro-it2] → [metro-mvp] [LOE:1][metro-it3]
(Reporter)

Updated

6 years ago
Whiteboard: [metro-mvp] [LOE:1][metro-it3] → [metro-mvp] [LOE:1]
(Assignee)

Updated

6 years ago
Assignee: ally → sfoster
Blocks: 831918
OS: Mac OS X → Windows 8 Metro
Summary: remove sites from startUI → Hide and restore sites from startUI
(Assignee)

Updated

6 years ago
Blocks: 832105

Updated

6 years ago
Whiteboard: [metro-mvp] [LOE:1] → [metro-mvp] [LOE:1] feature=work
Summary: Hide and restore sites from startUI → Work - Hide and restore sites from startUI
Whiteboard: [metro-mvp] [LOE:1] feature=work → feature=work
No longer blocks: 747789
(Assignee)

Updated

6 years ago
No longer depends on: 800996
(Assignee)

Comment 1

6 years ago
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.
No longer blocks: 832105
No longer depends on: 794028

Updated

6 years ago
Component: General → Firefox Start
Version: unspecified → Trunk
(Assignee)

Comment 2

6 years ago
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?
Attachment #727668 - Flags: review?(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.
Attachment #727668 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 4

6 years ago
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.
Attachment #727668 - Attachment is obsolete: true
Attachment #732792 - Flags: review?(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.
Attachment #732792 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 6

6 years ago
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)
https://hg.mozilla.org/mozilla-central/rev/e4b06af6ce99
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.