Closed Bug 831934 Opened 11 years ago Closed 11 years ago

Story - Access the Context App Bar for interacting with Firefox Start tiles

Categories

(Tracking Graveyard :: Metro Operations, defect, P2)

Product:
Tracking Graveyard
Component:
Metro Operations
Platform:
x86
Windows 8.1
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: asa, Assigned: sfoster)

References

Details

(Whiteboard: feature=story c=firefox_start u=metro_firefox_user p=8)

Attachments

(2 files, 4 obsolete files)

Firefox Start screen context app bar
4.08 KB, text/html
Details
context bar open even after page loaded
450.36 KB, image/png
Details
Attached file UC-76 Access the context app bar (obsolete) — 
      No description provided.
Priority: -- → P2
Whiteboard: c=Context_app_bar u= p= → c=Context_app_bar u= p=0 feature=story status=for_testing
Whiteboard: c=Context_app_bar u= p=0 feature=story status=for_testing → c=Context_app_bar u=metro_firefox_user p=0 feature=story status=for_testing
Priority: P2 → P5
Assignee: nobody → sfoster
Priority: P5 → P2
Whiteboard: c=Context_app_bar u=metro_firefox_user p=0 feature=story status=for_testing → c=Context_app_bar u=metro_firefox_user p=7 feature=story status=story
OS: Windows 8 → Windows 8 Metro
Summary: Access the context app bar → Story - Access the Context App Bar
Whiteboard: c=Context_app_bar u=metro_firefox_user p=7 feature=story status=story → feature=story status=for_sprint c=Context_app_bar u=metro_firefox_user p=7
Status: NEW → ASSIGNED
No longer blocks: 800996
Depends on: 800996, 834190
Hardware: x86_64 → x86
I intend to break this out, write a few tests and land the parts seperately but I wanted to get feedback so far. 

I've implementd the contextual appbar for TopSites, but you'll see that there's a lot there that will be common to all *Views. I could use a mixin to decorate the prototype of each. Or actual prototypal inheritance from a super View.

Functionality-wise no TopSites or other tiles are actually pinned/deleted/manhandled, this patch is just about messaging and getting the appbar to do its thing. 

Similarly I've not implmented pinned/hidden state at all for TopSites, just a mechanism by the contextual actions might work (the data-verbs attribute)

Right now there's a potential disconnect between the selectionchange and the buttons that result, and that action being applied to a different selection somehow - I don't keep any state. Can I pass the view object in the notifyObservers call? that would help.
Attachment #706180 - Flags: review?(mbrubeck)
Attachment #706180 - Flags: review?(ally)
Attachment #706180 - Flags: review?(mbrubeck)
Attachment #706180 - Flags: review?(ally)
Attachment #706180 - Flags: feedback?(mbrubeck)
Attachment #706180 - Flags: feedback?(ally)
Depends on: 834689
Comment on attachment 706180 [details] [diff] [review]
WIP with messaging, richgrid selectionchange events, contextual appbar buttons, topsites implementation

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

After seeing all how all the parts of this interact (thanks!), I'm thinking that Appbar makes sense as the central "switchboard" for most of the selection/context-action dispatching.  I'll add some comments inline to try to illustrate.

::: browser/metro/base/content/TopSites.js
@@ +89,5 @@
> +
> +    // republish as v notification, with a ref to this view
> +    Util.dumpLn("TopSites#onTileGroupSelectionChange notifyObservers");
> +    let subj = null; // could pass a ref to the view?
> +    Services.obs.notifyObservers(subj, "contextual-actions-change", null);

Instead of a notification here, how about a direct call to an Appbar method, something like "Appbar.showContextualActionsFor(tileGroup)" (and then Appbar can look at the tileGroup and its selectedItems to decide which actions to show).

::: browser/metro/base/content/appbar.js
@@ +125,5 @@
> +  dispatchContextualAction: function(aActionName){
> +    dump("ME: Appbar dispatchContextualAction of name: " + aActionName + "\n");
> +    let obs = Cc["@mozilla.org/observer-service;1"].
> +                  getService(Ci.nsIObserverService);
> +    obs.notifyObservers(null, "contextual-tile-action", aActionName);

...and here, since Appbar would know which tileGroup is currently selected, it can call a method or dispatch an event directly to that tileGroup, rather than broadcasting a notification to all of them.

@@ +130,5 @@
> +  },
> +
> +  showContextualActions: function(aVerbs){
> +    // hide them all, then show the ones we need
> +    let buttons = aVerbs.map( this._getContextButtonForVerb.bind(this) ).filter(Boolean);

".filter(Boolean)" -- nice idiom, I'll have to steal this. :)

::: browser/metro/base/content/bindings/grid.xml
@@ +29,5 @@
>          <body>
>            <![CDATA[
> +            // 'selection' and 'selected' are confusingly overloaded here
> +            // as richgrid is adopting multi-select behavior, but select/selected are already being
> +            // used to describe triggering the default action of a tile

We should probably change the latter to "activeItem" or something.

@@ +37,5 @@
> +            }
> +            let selectedChildren = this.querySelectorAll("richgriditem[selected='true']");
> +            for(let i=0; i<selectedChildren.length; i++){
> +              selectedChildren[i].selected = false;
> +            }

You could just use "this.selectedItems" here.

@@ +48,4 @@
>          <body>
>            <![CDATA[
> +            anItem.selected = !anItem.selected;
> +            this._fireOnSelectionChange();

If we can avoid the need for a global broadcast of selection changes (see below), then we can replace this event with a direct method call to the controller.

::: browser/metro/base/content/browser-ui.js
@@ +1081,5 @@
> +      for(let i=0; i<tileGroups.length; i++){
> +        let tileGroup = tileGroups[i];
> +        if(tileGroup !== aEvent.target) {
> +          // clear selections on groups other than the current one
> +          tileGroup.clearSelection(); // no events fire as a result

Instead of looping through all tile groups, let's just keep track of the currently "selected" group.  Then we only have to notify that one group.
Attachment #706180 - Flags: feedback?(mbrubeck) → feedback+
Sounds good. I overlooked the .controller property on the richgrid so struggled to see how to call methods on the associatd view without a bunch of events going back and forth. 
Your suggesions also remove the need for the View superclass - at least for now - which is a good thing.
(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> Instead of a notification here, how about a direct call to an Appbar method,
> something like "Appbar.showContextualActionsFor(tileGroup)"

...or if you prefer looser coupling, it can dispatch an event from the tileGroup and Appbar can listen for the event bubbling up to the window.  I don't care about the mechanism; I mostly want to remove BrowserUI as a third-party broker between the tile sets and the appbar.
Apply on top of richgrid patch on #834689

I've reworked this to get rid of the notifications. 

The views (richgrid controller) are still responsible for managing their own selectedTileActions property. Views can also implement clearTileSelection. When contextual buttons are clicked, the view has its doActionOnSelectedTiles method called. 

We'll need to work out how to handle when the Appbar is dismissed/shown. What if the selection happes when it is already open - does clicking a button still result in it being dismissed? 

I think to land this I'll break out the TopSites stuff so we just get the mechanism itself in one patch. Then a reference implementation in TopSites.js in another - which might need more work as we dig into the behavior wanted there
Attachment #706180 - Attachment is obsolete: true
Attachment #706180 - Flags: feedback?(ally)
Attachment #708126 - Flags: feedback?(mbrubeck)
Comment on attachment 708126 [details] [diff] [review]
Reworked Appbar and TopSites to simplify contextual-actions

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

::: browser/metro/base/content/TopSites.js
@@ +46,5 @@
>  
> +  get selectedTileActions() {
> +    // list of verbs/actions is updated via selection changes in the view's tile group
> +    return this._verbsApplicableToSelection;
> +  },

Most of the added code here in TopSitesView.prototype is actually generic code that can be used by all grids, so I'd like to move it into either the <richgrid> binding or appbar.js.

Rationale: Since the grids all share a binding, putting common code/APIs into the binding rather than the view is an easier way to share code and enforce a common interface.  I don't want to start depending on an unspecified interface for these "view" objects.

I'm hoping this will reduce/simplify the code a bit; if it doesn't, please feel free to push back. :)

@@ +67,5 @@
> +
> +  clearTileSelection: function(){
> +    this._set.clearSelection();
> +    this._verbsApplicableToSelection = [];
> +  },

Instead of adding this new method to the "view" object, Appbar can call clearSelection directly on the <richgrid>.

@@ +71,5 @@
> +  },
> +
> +  onTileGroupSelectionChange: function _onTileGroupSelectionChange(aEvent){
> +    // handle changes in selection state on our tile group/grid
> +

The view shouldn't need to listen for selectionchange events; Appbar already listens for them and can request or calculate the new set of context items whenever the selection changes.

@@ +86,5 @@
> +  },
> +
> +  doActionOnSelectedTiles: function(aActionName){
> +    let tileGroup = this._set;
> +    let selectedTiles = tileGroup.selectedItems;

Instead of a method on the view, let's have Appbar dispatch a "context-action" event to the <richgrid>, and the view (or anyone else) can listen for that event.

@@ +163,5 @@
>    destruct: function destruct() {
>      // remove the observers here
>    },
> +
> +  _getActionsForTiles: function(aTileNodes){

This function isn't specific to TopSites; lets move it into the <richgrid> binding or Appbar.

Also, for the case where all tiles in a grid support the same actions, we should probably have a way to indicate this on the <richgrid> itself, e.g. <richgrid data-verbs="delete,pin"> and then just return that from the "getActionsForTiles" method.

@@ +190,5 @@
> +        var overlap = aValues.filter(function(val){
> +          return aCommon.indexOf(val) > -1;
> +        });
> +        return overlap;
> +      }, lists.shift());

lists.shift() is unnecessary here - just return lists.reduce(function() { ... }).

::: browser/metro/base/content/appbar.js
@@ +8,5 @@
>    get pinButton()     { return document.getElementById('pin-button'); },
>    get moreButton()    { return document.getElementById('more-button'); },
>  
> +  // track selected/active view - the context for contextual action buttons
> +  activeView: null,

Rather than keep track of the active view, I'd like appbar to keep track of the active <richgrid> element.  It can call methods directly on the grid, and/or dispatch elements to it which controller/view code can listen for.

@@ +131,5 @@
>        window.openDialog("chrome://browser/content/shell.xul", "_blank",
>                          "all=no,scrollbars=yes,resizable=yes,dialog=no");
>    },
>  
> +  dispatchContextualAction: function(aActionName){

style nit: space before "{" (throughout this patch - purely for consistency)

@@ +147,5 @@
> +  },
> +
> +  showContextualActions: function(aVerbs){
> +    // hide them all, then show the ones we need
> +    let buttons = aVerbs.map( this._getContextButtonForVerb.bind(this) ).filter(Boolean);

Optional style hint: If you want to avoid the .bind(this) rigamarole, maybe just define the getContextButton function as a nested function here, rather than a method?
Attachment #708126 - Flags: feedback?(mbrubeck) → feedback+
Blocks: 831671
Reworked to address feedback: 
* richgrid now has a contextActions property which the appbar uses to put up the right buttons
* appbar tracks selected richgrid via an activeTileset property and talks directly to it instead of the View
* TopSitesView has what I think it the minimum boilerplate. It listens for the context-action event on its _set (richgrid) and does the thing indicated on the richgrid's selectedItems
Attachment #708126 - Attachment is obsolete: true
Attachment #708686 - Flags: feedback?(mbrubeck)
Attachment #708686 - Attachment is patch: true
Comment on attachment 708686 [details] [diff] [review]
Revised appbar/richgrid/*View relationship

Latest patch still needs to be applied on top of the grid.xml patch on https://bugzilla.mozilla.org/show_bug.cgi?id=834689
Comment on attachment 708686 [details] [diff] [review]
Revised appbar/richgrid/*View relationship

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

Nice, thanks!

::: browser/metro/base/content/bindings/grid.xml
@@ +119,5 @@
> +                var overlap = aValues.filter(function(val) {
> +                  return aCommon.indexOf(val) > -1;
> +                });
> +                return overlap;
> +              }, lists.shift());

I still think you can omit the lists.shift() argument here.

@@ +416,5 @@
>                item.setAttribute("value", aValue);
> +
> +            // copy over the richgrid's data-verbs as each child is created
> +            if (this.hasAttribute("data-verbs"))
> +              item.setAttribute("data-verbs", this.getAttribute("data-verbs"));

Instead of copying this onto each child, and then looping through all the children again in the contextActions getter, we should just return early from the contextActions getter if (this.hasAttribute("data-verbs")).
Attachment #708686 - Flags: feedback?(mbrubeck) → feedback+
(In reply to Matt Brubeck (:mbrubeck) from comment #9)
> Instead of copying this onto each child, and then looping through all the
> children again in the contextActions getter, we should just return early
> from the contextActions getter if (this.hasAttribute("data-verbs")).

Saw your reply in IRC -- I hadn't really thought about the fact that the verbs would be changing frequently on items as they are pinned, unpinned, etc.  That means my idea of a per-grid value isn't actually useful as an optimization.

Instead of a string atttribute, I'd consider using a .contextActions property on the richgriditem binding.  The property would be an array of strings (and we might also include some add/remove methods to manipulate the array, too).
Blocks: metrov1it1
Comment on attachment 708686 [details] [diff] [review]
Revised appbar/richgrid/*View relationship

This patch migrated south to bug 834190.
Attachment #708686 - Attachment is obsolete: true
No longer blocks: metrov1it1
Blocks: metrov1backlog
Whiteboard: feature=story status=for_sprint c=Context_app_bar u=metro_firefox_user p=7 → feature=story c=Context_app_bar u=metro_firefox_user p=7
Depends on: 842203
We've changed course here and the context app bar will be a part of the Firefox app bar. That story is bug 831899. We've discontinued this separate app bar approach.
No longer blocks: 831671, metrov1backlog
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
No longer depends on: 800996, 842203
Resolution: --- → WONTFIX
Whiteboard: feature=story c=Context_app_bar u=metro_firefox_user p=7
Oops, we still need this for context actions on Firefox Start, maybe? Was I hasty in closing?
Status: RESOLVED → REOPENED
Depends on: 800996, 842203
Resolution: WONTFIX → ---
Yeah, UX-wise it's all folded into a single bar, but it still exists as a thing. At least for now, we may find down the road that we can consolidate code and flatten things out.
Component: General → Metro Operations
Product: Firefox for Metro → Tracking
Version: unspecified → ---
New story coming for the the context/command app bar for Firefox Start. The context app bar for web pages is folding in with Bug 845137 - (newUI) Epic - NewUI - Main UI Reorganization
Blocks: 747789
No longer depends on: 842203
updated story.
Attachment #703502 - Attachment is obsolete: true
No longer blocks: metrov1onhold
Blocks: metrov1planning
Whiteboard: feature=story c=firefox_start u=metro_firefox_user p=0
Sam can you provide a point value for this story?
The only pieces of this I see which aren't done are: 
* cross-slide gesture to select tiles - this is a part of #831918 (Pin Topsites) and probably other tile-based stories too
* the context appbar is a part of the appbar redesign. My understanding is that will include all transitions.
* the topsites/bookmarks -appropriate contextual icons and behavior are all specified under their own stories. 

So I think this is either:
* done (demonstrably once patches on #808770 land), 
* invalid, 
* or, not-done but needing no work specific to this story
Hi Asa, please see Sam's comment #18 for your opinion on how to proceed.  Thanks.
Flags: needinfo?(asa)
Let's call this done. We've mangled this a bit with the re-design that combines the app bars so I'd rather any additional work be attached to stories that are more contemporary than this.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: needinfo?(asa)
Resolution: --- → WORKSFORME
No longer blocks: metrov1planning
Thanks Asa.  Will you add the additional work to the other stories or should Sam?  Also, does anything need to be edited in the current user story before it is assigned to Juan for testing?
Flags: needinfo?(asa)
(In reply to Marco Mucci [:MarcoM] from comment #21)
> Thanks Asa.  Will you add the additional work to the other stories or should
> Sam?  Also, does anything need to be edited in the current user story before
> it is assigned to Juan for testing?

I've moved bug 800996 to block bug 835623 - Story - Combine page contextual items (URL bar, bookmark star, etc.) into single app bar. 

With that, we can push this to for_testing.
No longer depends on: 800996
Flags: needinfo?(asa)
Blocks: metrov1legacy
Flags: needinfo?(jbecerra)
QA Contact: jbecerra
Asa, could you edit the story to reflect the changes to the story? I just want to know what is the main success scenarios, because, for example, long-tap on bookmark tiles selects them, but doesn't bring up a context menu or app infobar.
Flags: needinfo?(jbecerra) → needinfo?(asa)
I need some with the definition of the story in order to verify it.
Flags: needinfo?(mmucci)
(In reply to juan becerra [:juanb] from comment #23)
> Asa, could you edit the story to reflect the changes to the story? I just
> want to know what is the main success scenarios, because, for example,
> long-tap on bookmark tiles selects them, but doesn't bring up a context menu
> or app infobar.


I think this story is not actually completed. We have the context app bar working for the Top Site tiles but none of the other Firefox Start tile groups. 

If we want to call this complete, we'll need to change it to "Context app bar for Top Site tiles" and need to create another story for Bookmarks and History. 

If we do that, the success guarantee for this story is "the long pressed Top Sites tile was selected and the context app bar showed."

We'll also want a change or defect (or story?) for implementing the drag selection and replacing our long press selection.  We'll also want a work item, change, or defect for getting the right actions into the context app bars for each kind of Start Page tile group.
Flags: needinfo?(asa)
Hi Asa, what is your preference:

1.  Reopen and move this to the Story Backlog for development in a future iteration until all the requirements are completed.

2.  Edit the existing story and create a new change story for the remaining requirements.

Marco
Flags: needinfo?(mmucci)
This story is being moved to 'On Hold' after further discussion with Asa for point estimation for completing the outstanding component(s).
Blocks: metrov1onhold
No longer blocks: metrov1legacy
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Depends on: 841228
This story is still accurate. We should bring it out from on hold and get dependency work bugs hooked up to it for the remaining pieces.
No longer blocks: metrov1onhold
Summary: Story - Access the Context App Bar → Story - Access the Context App Bar for interacting with Firefox Start tiles
Not all the start screen grids will have all their applicable contextual actions populated in the context app bar - they are being addressed in other stories. But, the defaults at minimum (e.g. select none) should work at completion of this story
Whiteboard: feature=story c=firefox_start u=metro_firefox_user p=0 → feature=story c=firefox_start u=metro_firefox_user p=8
Blocks: metrov1it6
No longer blocks: metrov1planning
Whiteboard: feature=story c=firefox_start u=metro_firefox_user p=8 → feature=story c=firefox_start u=metro_firefox_user p=8
Status: REOPENED → ASSIGNED
All dependent bugs now in m-c, this should be good to go. See comment 29 for caveats - each start tile group will eventually implement its own contextual actions to remove/pin etc. At minimum, all tiles will show a "clear selection" button if you select more than one in a group which should be sufficient to test the feature. Top Sites implement a full complement of contextual actions
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Depends on: 867179
Depends on: 867523
Depends on: 867525
This feels like it should be reopened because the only thing we can do right now, that we expect, is for us to bring up the context app bar for these tiles, but the functions within the context app bar don't work as expected. These have been captured at least in part by the bugs above.

The story doesn't say anything about these options working, but it seems like that should be called out in the story.

Can I mark this as verified or should I re-open?
Flags: needinfo?(mmucci)
Hi Juan, thanks for the update.  Let's bring Bug 831934 up in the review meeting today to see if we should just reopen the original story fully.
Flags: needinfo?(mmucci)
I want to make sure this isn't just a symptom of bug 867543, i.e that without labels the function of the icons is ambiguous (at best). Also, as seen in attachment 726419 [details], the other page-scoped appbar buttons should not be visible which is also contributing to confusion here I think? I've filed that as bug 867616
(In reply to Sam Foster [:sfoster] from comment #33)
> I want to make sure this isn't just a symptom of bug 867543, i.e that
> without labels the function of the icons is ambiguous (at best). Also, as
> seen in attachment 726419 [details], the other page-scoped appbar buttons
> should not be visible which is also contributing to confusion here I think?
> I've filed that as bug 867616

I find this confusing as well. I'm never sure what the buttons do or which one to press. Maybe we could create a keyboard shortcut / hot press area on the app bar that displays labels on buttons?
No longer depends on: 867179
No longer depends on: 867523
Off of today's m-c, I find that I cannot dismiss any context bar. It even stays open after you leave the start screen and load a bookmark/topsites/history item!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 876062
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
No longer depends on: 876062
Depends on: 877875
Depends on: 886652
Depends on: 891182
Depends on: 867543
Depends on: 894949
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130826074752
Built from http://hg.mozilla.org/mozilla-central/rev/14b1e8c2957e

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in user story and got expected result.
No longer depends on: 877875
OS: Windows 8 Metro → Windows 8.1
Product: Tracking → Tracking Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: