Closed Bug 834190 Opened 12 years ago Closed 12 years ago

Work - Context App Bar messaging

Categories

(Firefox for Metro Graveyard :: General, defect)

x86
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sfoster, Assigned: sfoster)

References

Details

(Keywords: feature, platform-parity, Whiteboard: feature=work)

Attachments

(1 file)

Implement messaging between BrowserUI/ContextUI and the individual views to drive contextual action choices for selected tiles
Blocks: 831934
See evolution of this patch on https://bugzilla.mozilla.org/show_bug.cgi?id=831934 Apply this patch on top of the patch on https://bugzilla.mozilla.org/show_bug.cgi?id=834689 which provides the multi-selection aware richgrid. This patch puts in place the means to control the available contextual actions from the tile (and whatever generates/controls it). The right subset of actions should be presented when multiple tile selections have different context actions. Each tille (richgriditem) exposes a contextActions property. This is initially populated from the data-contextitems attribute on the richgriditem. The Appbar listens for selectionchanges on richgrids (e.g. by a contextmenu event from the tile). It maintains a activeTileset property. The activeTileset (richgrid) has an aggregate contextActions property - a Set - which is used to put up the right buttons in the appbar. When a contextual-action button is clicked, a 'context-action' event is dispatched on the activeTileset. The controller attached to this richgrid can handle the event to make it so. This patch includes the minimum code in TopSites to be able to complete the whole flow, including placeholder code to toggle pin/unpin of top sites (dosn't actually do anything - that work will happen in #831918) Known issues/qustions: * frynn mentioned an issue with xbl bindings being lost/reset if the element is ever moved/removed, and so to avoid storing state in the binding. Not sure if this applies here is is a concern? * There's a couple FIXMEs in there, for work that follows from this patch and is outside the scope of this ticket. * Eventually we might want to hide (w/ transition) the whole contextual action button tray if it is empty. I propose we KISS for now as the whole appbar is up for redesign. * AFAIK the existing pinButton is not at all the same as the contextual pin button - which will used to fix a tile in place. They are easily confused though. * Might want a drive-by review from Frank and Ally as this touches their worlds?
Attachment #709707 - Flags: review?(mbrubeck)
Comment on attachment 709707 [details] [diff] [review] tile and richrid contextActions set drive contextual action btns in appbar Review of attachment 709707 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Some minor JavaScript hints/nits below. It would be good to land now this so other patches don't add conflicts in the meantime, but I worry that exposing non-functional UI will be confusing to nightly testers. Can we land it "disabled" by commenting-out/disabling/hiding #contextualactions-tray or its contents? ::: browser/metro/base/content/appbar.js @@ +152,5 @@ > + let doc = document; > + Util.dumpLn("showContextualActions, show buttons for: " + aVerbs.join(', ')); > + > + // button element id to action verb lookup > + let buttonsMap = {}; Please remove the dumpLns before landing. You could use a Map here. @@ +167,5 @@ > + > + // hide/show buttons as appropriate > + let buttons = doc.querySelectorAll("#contextualactions-tray > toolbarbutton"); > + > + Array.forEach(buttons, function(aBtnNode){ for (let button of buttons) { ::: browser/metro/base/content/bindings/grid.xml @@ +102,5 @@ > + > + let tileNodes = Array.slice(this.selectedItems, 0); > + dump("selectedItems : " + (tileNodes.map(function(node){ > + return node.nodeName+"#"+node.getAttribute("value"); > + })).join(", ") +"\n"); Please remove the dump() before landing. You can avoid allocating an Array here if you change "tileNodes.forEach" below to "for (let node of this.selectedItems)". @@ +120,5 @@ > + } > + > + function setToArray(aSet) { > + return [v for (v of aSet)]; > + } It looks like this is unused and can be removed. ::: browser/metro/base/content/browser.xul @@ +316,5 @@ > + <hbox id="contextualactions-tray" flex="1"> > + <toolbarbutton id="delete-selected-button" hidden="true" oncommand="Appbar.dispatchContextualAction('delete')"/> > + <toolbarbutton id="restore-selected-button" hidden="true" oncommand="Appbar.dispatchContextualAction('restore')"/> > + <toolbarbutton id="pin-selected-button" hidden="true" oncommand="Appbar.dispatchContextualAction('pin')"/> > + <toolbarbutton id="unpin-selected-button" hidden="true" oncommand="Appbar.dispatchContextualAction('unpin')"/> We'll need icons for these, and labels for accessibility (for these and the other appbar buttons). We can get the icons in a separate bug -- we might want to wait for the redesign to start gelling first.
Attachment #709707 - Flags: review?(mbrubeck) → review+
Address js style issues above and added a hidden attribute to the contextualactions-tray element in browser.xul, to avoid confusion while the other moving parts of this user story are worked on. https://hg.mozilla.org/projects/elm/rev/35e219b514cc
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: