Let's start simple, see attached "Synced-Tabs-UX.pdf", we want to implement slides 9 to 12.

Ryan, do we want a full-featured sync status bar like the one on the hamburger menu or do we want a "lite" version of it?

Here's what I got so far

Looking good! I like that you added the device icon. We need to update those. I filed another bug for updated icons. Show All Synced Tabs should up updated. And yes, definitely want a duplicate of the error row. Do you have the synced tabs icons you need?

See bug 1201326 :)

does the product owners want to schedule this in iteration: 43.3 or 44.1

Just a quick update to say I'm still working on this (at a slow pace though). I got the duplicated full featured status bar working (see screenshot). There's still a lot of css bugs/js cleanup to do.

And the current state of the patch. I will definitely slice it in 2 parts, one for the "status bar" code, one for the synced tabs menu. I extracted the "status bar" code to a Javascript Module named SyncStatusBar where I separated state fetching and dom rendering. I'll probably split the big renderStatusBar function. The tests are not updated yet and there's still a LOT of work to do.

What do you think we should do about the translation strings Mark? Right now it's still embedded in the |.syncbar| xul code as attributes (e.g |unverifiedlabel|). We could move them to a properties file but we'll lose (correct me if I'm wrong) the ability to concatenate |&brandShortName;| for example.

(In reply to Edouard Oger [:eoger] from comment #9) > What do you think we should do about the translation strings Mark? > Right now it's still embedded in the |.syncbar| xul code as attributes (e.g > |unverifiedlabel|). > We could move them to a properties file but we'll lose (correct me if I'm > wrong) the ability to concatenate |&brandShortName;| for example. .properties files can have string substitutions - they are generally done with formatStringFromName() - to insert the "brand" name, you'd do one lookup in chrome://branding/locale/brand.properties then use formatStringFromName() passing the brand string. But yeah, .properties files make that a little more painful and needs to be setup with code rather than directly in the xul/html. The reverse is also true - if a .dtd file needs a value substituted at runtime, it's common to define the string with #1 and use javascript's .replace() at runtime (eg, aboutCertError.dtd) - which makes it just as inconvenient as .properties to use :) Why do you ask? ie, why wouldn't we just stick with the existing mechanism? While we can "move" the en-US strings, localizers probably will not be aware they were moved and will need to re-create them - which would be fine if there was a good reason. I think the general preference is to stick with .dtd files when the strings are mainly used directly in the xul/html but .properties files when code needs access to them in ways where it's not natural to pull the same "unverifiedlabel" trick we use. IOW, our l10n story sucks :( Please let me know if I haven't answered your question :)

Comment on attachment 8666529 [details] [diff] [review] bug-1201331.patch Review of attachment 8666529 [details] [diff] [review]: ----------------------------------------------------------------- This is a fairly quick drive-by look - sorry it's taken so long - but this is looking awesome :) There's still a fair bit of work to do though, as I'm sure you know. Are you still planning to keep working on this in the short-ish term? Another bit of good-news/bad-news is that the migration stuff is likely to change - the good part is that the migration complexity will be largely gone, but the bad news is that it will still make some of these bits stale. I'll help you keep this up-to-date with that though, and I'll CC you on the bug where this is happening. ::: browser/base/content/browser.css @@ +1150,5 @@ > @media (resolution: 2dppx) { > #PanelUI-recentlyClosedWindows > toolbarbutton > .toolbarbutton-icon, > #PanelUI-recentlyClosedTabs > toolbarbutton > .toolbarbutton-icon, > + #PanelUI-historyItems > toolbarbutton > .toolbarbutton-icon, > + #PanelUI-sync-othertabs-tabslist > toolbarbutton > .toolbarbutton-icon { Some other work (eg, awesomebar) is settling on using "remotetabs", so I think we should be consistent (ie, s/othertabs/remotetabs/) - unless there's a good reason to use "other", in which case we should change those other places :) ::: browser/components/customizableui/CustomizableWidgets.jsm @@ +8,5 @@ > this.EXPORTED_SYMBOLS = ["CustomizableWidgets"]; > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +#ifdef MOZ_SERVICES_SYNC > +Cu.import("resource://services-common/utils.js"); I think we are going to want a module in Sync that enumerates the remote tabs and is shared by this code and aboutSyncTabs.js - that should simplify the code here considerably. ::: browser/components/sync/SyncStatusBar.jsm @@ +20,5 @@ > + return Cu.import("resource://gre/modules/FxAccountsCommon.js", {}); > +}); > +XPCOMUtils.defineLazyModuleGetter(this, "EnsureFxAccountsWebChannel", > + "resource://gre/modules/FxAccountsWebChannel.jsm"); > + I think a few comment lines saying what this module does and how it works would be valuable. Just something like "This module tracks global Sync and Firefox Account state and reflects that state in UI components that are registered with this module." @@ +41,5 @@ > + return false; > + } > + }, > + > + get state() { Why not replace this.setState with a |set state()|? @@ +46,5 @@ > + return this._state; > + }, > + > + get topics() { > + // Do all this dance to lazy-load FxAccountsCommon. Note that there's no real value in attempting to lazy-load FxAccountsCommon here - as soon as the module is loaded we call .init(), which then references this.topics. IIRC, the idea is that gFxAccounts.init() is called on browser-delayed-starup notification, so that module doesn't want to suck in FxAccountsCommon (or this module) until that is done - but that doesn't apply here as this .init()'s method is called immediately on import. IOW, I think it is safe to Cu.import FxAccountsCommon, meaning this.topics could simply be a constant at the top of the file. @@ +112,5 @@ > + > + this._initialized = false; > + }, > + > + registerStatusBar: function (statusBarNode) { I suspect you will hate this idea, but IIUC, there's really only 2 public methods on this object - registerStatusBar and toggleSyncStatus (which is probably wrong too - see below) - so we might want a public object to only expose those public methods and an "internal" object with stuff we don't expose. We just don't need to go quite as far as FxAccounts takes this. @@ +122,5 @@ > + statusBars.add(statusBarNode); > + this.renderStatusBar(win, statusBarNode); > + }, > + > + toggleSyncStatus: function (active) { I doubt this should be public, but instead this module should track the status itself. The reason is that the callers of this are browser-fxaccounts, and there is one of them per window - so this will end up being called multiple times (once for each window) as the state changes. So it might make sense to reverse this - something like "isSyncRunning()" as a public method/getter consumers can query. @@ +127,5 @@ > + let newState = {syncActive: active}; > + this.setState(newState); > + }, > + > + setLastSyncTooltip: function (tooltipText) { I'd be inclined to have this managed internally too - the only caller is browser-syncui, and that already does FxA stuff. Indeed, I'd almost be inclined to merge browser-syncui and browser-fxaccounts. OTOH though, I guess we don't need to boil the ocean in the first cut, so I could be convinced to live with this for now. @@ +168,5 @@ > + renderStatusBar: function (win, statusBarNode) { > + let { status, userData, profile, syncActive, lastSyncTooltip } = this._state; > + let profileInfoEnabled = this.profileInfoEnabled; > + let rootEl = statusBarNode; > + let statusEl = statusBarNode.querySelector(".fxabar-status"); if we weren't trying to kill xbl, I'd almost suggest the status bar should be one ;) @@ +202,5 @@ > + labelEl.label = label; > + return; > + } > + > + if (status === "pendingUserData") { this looks a little like the status mechanism should use a promise - maybe something like this._state has a promiseStatus element? @@ +321,5 @@ > + this.setState(newState); > + return Promise.resolve(); > + } > + > + newState.status = "pendingUserData"; ie, we'd create the promise here and resolve it below @@ +368,5 @@ > + updateWithUserData(null); > + }); > + }, > + > + onMenuPanelCommand: function (e) { at first glance, I think it might be better to keep the event handler itself in browser-fxaccounts, which could explicitly call into here passing window. I'm not sure about that though

I adapted my patch to the changes the UX team made these last days. Since we no longer need a duplicated sync status bar, I removed the refactoring part of the previous patch. I'll open in the following weeks another bug to continue the sync status bar refactor work. TODO: - No "fetching" state message, we have no idea what's happening. - Externalize tabs enumeration code in another module. - Tests? I only work on this in my free time so I'm aware the iterations are really slow, if someone wants to pick this up I'll give him/her my blessing.

Mark, I moved the sync bar code refactor in bug 1214849.

(In reply to Edouard Oger [:eoger] from comment #12) > I only work on this in my free time so I'm aware the iterations are really > slow, if someone wants to pick this up I'll give him/her my blessing. Thanks Edouard, we totally understand and appreciate your work so far - it's really very close - but we probably will pick this up in the next week or so. Could you please verify the attached version is the latest you have, or attach a more recent version?

The current attachment the latest version, thank you for picking this up!

Attaching the current WIP. This patch adds a new broadcaster to simplify managing the animation for multiple sync buttons. Not requesting feedback yet as I don't think it will be controversial.

Zach, I took your module from the sidebar bug and changed it a little, so less state and complexity exists (eg, the maps don't seem to offer much value given we are just reaching into Sync's in-memory copy of the tabs). I think this should also work fine for you?

This is :eoger's patch with some tweaks. As discussed with rfeeley, we will "adopt" the existing Sync button instead of adding a new one. Gijs, this adds a new panel to the customizable UI for Sync. At this stage I'm only after very early feedback and without any urgency - there's still a fair bit to do, but I figured I might as well get feedback as early as possible given I'll probably be asking you for review :) Please feel free to pass this on to someone else if you prefer.

Comment on attachment 8683555 [details] [diff] [review] 0005-wip.patch Review of attachment 8683555 [details] [diff] [review]: ----------------------------------------------------------------- As a general thing, I thought we were going to upgrade about:sync-tabs to be nice and use that? Kind of confused at the duplication. Anyway, here's some review comments! :-) ::: browser/components/customizableui/CustomizableUI.jsm @@ +165,5 @@ > "history-panelmenu", > "fullscreen-button", > "find-button", > +#ifdef MOZ_SERVICES_SYNC > + "remotetabs-panelmenu", This should match the button ID, ie sync-button. also, this change will cause a lot of test failures (sync-button is used in the CUI tests) ::: browser/components/customizableui/CustomizableWidgets.jsm @@ +11,2 @@ > Cu.import("resource:///modules/CustomizableUI.jsm"); > Cu.import("resource://gre/modules/Services.jsm"); Why this change in order? @@ +313,5 @@ > + viewId: "PanelUI-remotetabs", > + defaultArea: CustomizableUI.AREA_PANEL, > + onBeforeCreated(aDocument) { > + // onBeforeCreated is called fairly early after startup, and we don't > + // want to force a Sync at this time. You don't need to have this callback :-) @@ +322,5 @@ > + }, > + onViewShowing(aEvent) { > + if (!this._tabsList) { > + let doc = aEvent.target.ownerDocument; > + this._tabsList = doc.getElementById("PanelUI-remotetabs-tabslist"); So |this| here is going to be the persistent widget thing. That's not good because now you stuck a DOM node from the window where we were first shown in here. The window will die but this widget might not (if the process isn't exiting) so we'll leak the window. @@ +323,5 @@ > + onViewShowing(aEvent) { > + if (!this._tabsList) { > + let doc = aEvent.target.ownerDocument; > + this._tabsList = doc.getElementById("PanelUI-remotetabs-tabslist"); > + Services.obs.addObserver(this, "services.sync.tabs.update", false); Similar leaky issues with the observer. @@ +388,5 @@ > + _showTabs() { > + // XXX - this is promise-based, but it's probably not necessary to > + // handle multiple in-flight calls as we clear and rebuild the list > + // after the promise is resolved, so multiple calls shouldn't cause > + // a problem in practice. We seem to only call clearTabList when we get the new results, which means the items displayed in the popup might change while they are already displayed. That seems like something we shouldn't do. I also wonder if we shouldn't just cache things a bit here. @@ +413,5 @@ > + Cu.reportError(err); > + }); > + }, > + _appendClient: function (client, attachNode) { > + let doc = attachNode.ownerDocument; Nit: attachNode is always a fragment so it should probably be called that. @@ +422,5 @@ > + let clientEnt = this._createClientElement(doc, client); > + attachNode.appendChild(clientEnt); > + > + for (let tab of this._sortFilterTabs(client.tabs)) { > + let tabEnt = this._createTabElement(doc, tab); Nit: the order in which this method and the other helpers are defined is a little hodgepodgy. Can we either order callers...callees or callees...callers? I don't mind which, but right now for a relationship of A calls B calls C the methods are ordered C A B, which is confusing and makes the code harder to follow than necessary. :-) ::: browser/components/customizableui/content/panelUI.inc.xul @@ +106,5 @@ > oncommand="PlacesCommandHook.showPlacesOrganizer('History'); CustomizableUI.hidePanelForNode(this);"/> > </panelview> > > +#ifdef MOZ_SERVICES_SYNC > + <panelview id="PanelUI-remotetabs" flex="1"> This should get the PanelUI-subview class like the other panels. @@ +110,5 @@ > + <panelview id="PanelUI-remotetabs" flex="1"> > + <label value="Synced Tabs" class="panel-subview-header"/> > + <!-- this widget has 3 body vboxes, but only 1 is ever visible --> > + <!-- When Sync is ready to sync --> > + <vbox class="panel-subview-body" observes="sync-syncnow-state"> Having 3 things with .panel-subview-body is going to be awkward: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.xml#413 Maybe consider sticking all 3 in a single container with that class? @@ +133,5 @@ > + oncommand="gSyncUI.openPrefs('menusubpanel')"/> > + </vbox> > + <!-- When Sync needs re-authentication --> > + <vbox class="panel-subview-body" observes="sync-reauth-state"> > + <toolbarbutton label="Sign in to continue syncing - TODO - real content?" You seem to have done strings for a number of other things, any reason these are still placeholders? @@ +137,5 @@ > + <toolbarbutton label="Sign in to continue syncing - TODO - real content?" > + oncommand="gSyncUI.openPrefs('menusubpanel')"/> > + </vbox> > + <!-- XXXX - need another for when Sync is configured but the tabs engine > + is not? What happens right now if this is the case? ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +600,5 @@ > } > > +#PanelUI-remotetabs-syncstatus, > +#PanelUI-remotetabs-syncnow-label { > + padding: 0px; Why is this necessary here specifically? Also, on what OSes have you tested this? @@ +1084,5 @@ > + min-width: 220px; > +} > + > +#PanelUI-remotetabs-tabslist > label[type="client"] { > + color: GrayText; This seems like it'll might break with "interesting" themes, and like it duplicates things. How are we styling the other labels in these subviews, and can we ensure that this just has the right class to benefit from that styling? @@ +1096,5 @@ > > #PanelUI-recentlyClosedWindows > toolbarbutton > .toolbarbutton-icon, > #PanelUI-recentlyClosedTabs > toolbarbutton > .toolbarbutton-icon, > +#PanelUI-historyItems > toolbarbutton > .toolbarbutton-icon, > +#PanelUI-remotetabs-tabslist > toolbarbutton > .toolbarbutton-icon { Here and in several other places it seems like there's no alphabetical order, and so you might as well just stick the new line above the existing line so we don't touch blame...

Thanks Gijs, I addressed most of your comments, but I'd like further feedback on a couple of things here. > As a general thing, I thought we were going to upgrade about:sync-tabs to be > nice and use that? Kind of confused at the duplication. We were, but UX always reserves the right to change their mind ;) > also, this change will cause a lot of test failures (sync-button is used in > the CUI tests) Yeah - I'll work on tests next, but wanted to make sure the direction was reasonable first. > ::: browser/components/customizableui/CustomizableWidgets.jsm > @@ +11,2 @@ > > Cu.import("resource:///modules/CustomizableUI.jsm"); > > Cu.import("resource://gre/modules/Services.jsm"); > > Why this change in order? qref failure :) Fixed. > So |this| here is going to be the persistent widget thing. That's not good > because now you stuck a DOM node from the window where we were first shown > in here. The window will die but this widget might not (if the process isn't > exiting) so we'll leak the window. Right. So this version uses onViewShowing() to setup the observer and this._tabsList, and onViewHiding() to remove the observer and null the element. This obviously implies that only one view can be visible at once, but I can't imagine how that wouldn't be true. The only reason we need to do this at all is because the observer tells us when new tabs are available. Do the changes I made here make sense? > We seem to only call clearTabList when we get the new results, which means > the items displayed in the popup might change while they are already > displayed. That seems like something we shouldn't do. I think we need to. If the user opens this panel before the first Sync, the list will initially be empty - but because we kick off a sync on open, it generally populates within a few seconds. Similarly, the user may have a tab open on a different device, then move back to this device and attempt to open it - and it may not appear because a sync hasn't been done for a while. I'm also hoping that the "spinning" of the Sync Now button will offer clues as to why the list is changing while it is open. IOW, I think it is important that this panel always have the most up-to-date list of tabs, even if the list changes while open due to a sync completing. > I also wonder if we shouldn't just cache things a bit here. I don't see much value in that as the list of tabs is already in memory - unless I misunderstand what you are suggesting here? > This should get the PanelUI-subview class like the other panels. CustomizableUI.jsm explicitly adds this class with |viewNode.classList.add("PanelUI-subView");| and I've verified it is added to this item. I'm a little confused by this as it is also added directly to some elements. Is there something I'm missing here? > You seem to have done strings for a number of other things, any reason these > are still placeholders? Because it's still very much a WIP - I'll add them soon. > @@ +137,5 @@ > > + <toolbarbutton label="Sign in to continue syncing - TODO - real content?" > > + oncommand="gSyncUI.openPrefs('menusubpanel')"/> > > + </vbox> > > + <!-- XXXX - need another for when Sync is configured but the tabs engine > > + is not? > > What happens right now if this is the case? You mean in about:sync-tabs? That just shows no tabs, which kinda sucks. > Why is this necessary here specifically? Also, on what OSes have you tested > this? Removed. > > @@ +1084,5 @@ > > + min-width: 220px; > > +} > > + > > +#PanelUI-remotetabs-tabslist > label[type="client"] { > > + color: GrayText; > > This seems like it'll might break with "interesting" themes, and like it > duplicates things. How are we styling the other labels in these subviews, > and can we ensure that this just has the right class to benefit from that > styling? The only other example I can find does something similar: #PanelUI-panic-actionlist-main-label { color: GrayText; font-size: 0.9em; } and the headers use: .panel-subview-header { margin: -4px -4px 4px; box-shadow: 0 -1px 0 hsla(210,4%,10%,.05) inset; color: GrayText; font-variant: small-caps; } so it looks like the CSS I added shouldn't have any more problems than those. > How are we styling the other labels in these subviews, and can we ensure > that this just has the right class to benefit from that styling? I can't find any other similar labels in subviews, and while .panel-subview-header exists, I can't see other rules that implies something already exists. I'd be happy to add, say, .panel-subview-subheader (sub-sub!) if you think that's worthwhile. All other comments were addressed - thanks for the (typically) thorough and timely review!

FTR, here's what the latest iteration looks like with my test account.

(In reply to Mark Hammond [:markh] from comment #21) > Created attachment 8685297 [details] > synced-tabs.png > > FTR, here's what the latest iteration looks like with my test account. Looks neat! (In reply to Mark Hammond [:markh] from comment #20) > > We seem to only call clearTabList when we get the new results, which means > > the items displayed in the popup might change while they are already > > displayed. That seems like something we shouldn't do. > > I think we need to. If the user opens this panel before the first Sync, the > list will initially be empty - but because we kick off a sync on open, it > generally populates within a few seconds. Similarly, the user may have a tab > open on a different device, then move back to this device and attempt to > open it - and it may not appear because a sync hasn't been done for a while. > I'm also hoping that the "spinning" of the Sync Now button will offer clues > as to why the list is changing while it is open. > > IOW, I think it is important that this panel always have the most up-to-date > list of tabs, even if the list changes while open due to a sync completing. I think I was unclear. I mean that we should clearTabList before the list becomes visible (or maybe just after the list is hidden). Right now we'll show the outdated list first, and then swap it out when we have the new one, which is the thing that I'm worried will cause unexpected results (ie opening the wrong tab or whatever) > > I also wonder if we shouldn't just cache things a bit here. > > I don't see much value in that as the list of tabs is already in memory - > unless I misunderstand what you are suggesting here? I think if the list of tabs is 10 seconds old there's not much point re-fetching it. I don't know what the threshold is here and if new sync has some way to do push - ie if I open a new tab, can we push just that delta out to all the other clients? Or is it strictly pull-based? > > This should get the PanelUI-subview class like the other panels. > > CustomizableUI.jsm explicitly adds this class with > |viewNode.classList.add("PanelUI-subView");| and I've verified it is added > to this item. I'm a little confused by this as it is also added directly to > some elements. Is there something I'm missing here? Err, I expect it's one of those optimizations for tpaint/ts_paint that we added, where dynamic DOM changes / restyles caused perf regressions compared with baked-in classes. Probably best to cargo-cult for now. > > @@ +137,5 @@ > > > + <toolbarbutton label="Sign in to continue syncing - TODO - real content?" > > > + oncommand="gSyncUI.openPrefs('menusubpanel')"/> > > > + </vbox> > > > + <!-- XXXX - need another for when Sync is configured but the tabs engine > > > + is not? > > > > What happens right now if this is the case? > > You mean in about:sync-tabs? That just shows no tabs, which kinda sucks. No, I meant, what does your patch currently show if this is the case? :-)

Comment on attachment 8685296 [details] [diff] [review] 0005-wip.patch Review of attachment 8685296 [details] [diff] [review]: ----------------------------------------------------------------- This generally looks fine. See some more notes below. ::: browser/components/customizableui/CustomizableUI.jsm @@ +166,5 @@ > "fullscreen-button", > "find-button", > +#ifdef MOZ_SERVICES_SYNC > + "sync-button", > +#endif See also my feedback in CustomizableUI.jsm - are we adding this to the menu panel by default? I am a bit saddened by the "button creep" here. ::: browser/components/customizableui/CustomizableWidgets.jsm @@ +310,5 @@ > + label: "remotetabs-panelmenu.label", > + tooltiptext: "remotetabs-panelmenu.tooltiptext", > + type: "view", > + viewId: "PanelUI-remotetabs", > + defaultArea: CustomizableUI.AREA_PANEL, Hm, this means it will now be automatically added to the panel. That doesn't seem right? Just omit for now, maybe? @@ +425,5 @@ > + item.setAttribute("targetURI", attrs.url); > + item.setAttribute("label", attrs.title != "" ? attrs.title : attrs.url); > + item.setAttribute("image", attrs.icon); > + item.addEventListener("command", e => { > + doc.defaultView.openUILink(attrs.url, e); Curious whether we should default to opening synced tabs in new tabs? Is that what this does by default (ie with no modifier keys pressed) ? ::: browser/components/customizableui/content/panelUI.inc.xul @@ +129,5 @@ > + fetchinglabel="&appMenuRemoteTabs.fetching.label;" > + notabslabel="&appMenuRemoteTabs.notabs.label;"/> > + </vbox> > + <!-- When Sync is not configured --> > + <vbox class="panel-subview-body" observes="sync-setup-state"> This and the bottom vbox still have .panel-subview-body ::: browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties @@ +6,5 @@ > # LOCALIZATION NOTE(history-panelmenu.tooltiptext2): %S is the keyboard shortcut > history-panelmenu.tooltiptext2 = Show your history (%S) > > +remotetabs-panelmenu.label = Synced Tabs > +remotetabs-panelmenu.tooltiptext = Show your synced tabs from other devices Nitpick: Seems to me this should either omit "[your] synced" or "from other devices". ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +1073,5 @@ > -moz-margin-end: auto; > color: GrayText; > } > > +/* only applies to the toolbar button */ Nit: only applies "when the subview is shown outside of the main menu panel" -- the button is always a toolbar button, and this is about the subview not the button. :-) @@ +1080,5 @@ > +} > + > +#PanelUI-remotetabs-tabslist > label[type="client"] { > + color: GrayText; > + font-size: 1.1em; Not super keen on this font-size thing because it's been historically "fun" to get fonts right here. Is it necessary and/or in the design and/or can you get ui-review? :-)

A newer version with: * Better tests, including a test-helper module that can be used by other tests. * No longer ignores tabs already open on the local device. * No longer excludes clients with zero tabs - the consumer can filter on that if they desire.

Thanks for the feedback Gijs. I'm hoping you can also help me with browser_876926_customize_mode_wrapping.js :) You landed this test a couple of years ago, and it's failing with my patches, but it looks like it's only indirectly related. At the top of the test we have: > const kXULWidgetId = "sync-button"; > const kAPIWidgetId = "feed-button"; And we are now failing in the 1 place where these 2 widgets are treated differently: > 38 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js | Widget sync-button should be in invisible palette in other window. - > Stack trace: > chrome://mochitests/content/browser/browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js:checkPalette:139 > chrome://mochitests/content/browser/browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js:MoveWidgetsInTwoWindows:158 I assume this is because the sync-button is now an "API Widget" (ie, removed from browser.xul and added to CustomizableWidgets) but I can't find another button that makes the test pass (eg, print-button seems to do what we need, but it fails in different ways). The test doesn't make it obvious why that button was chosen, nor why that special-case in the test exists. Can you give me any insights to save me dissecting that 2-year old bug in the morning?

(In reply to Mark Hammond [:markh] from comment #25) > Thanks for the feedback Gijs. > > I'm hoping you can also help me with > browser_876926_customize_mode_wrapping.js :) You landed this test a couple > of years ago, and it's failing with my patches, but it looks like it's only > indirectly related. > > At the top of the test we have: > > const kXULWidgetId = "sync-button"; > > const kAPIWidgetId = "feed-button"; > > And we are now failing in the 1 place where these 2 widgets are treated > differently: > > > 38 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js | Widget sync-button should be in invisible palette in other window. - > > Stack trace: > > chrome://mochitests/content/browser/browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js:checkPalette:139 > > chrome://mochitests/content/browser/browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js:MoveWidgetsInTwoWindows:158 > > I assume this is because the sync-button is now an "API Widget" (ie, removed > from browser.xul and added to CustomizableWidgets) but I can't find another > button that makes the test pass (eg, print-button seems to do what we need, > but it fails in different ways). The test doesn't make it obvious why that > button was chosen, nor why that special-case in the test exists. > > Can you give me any insights to save me dissecting that 2-year old bug in > the morning? I mean, the simple solution would be picking another existing xul widget that isn't about to be removed (so not tabview-button :-) ) and is in the palette by default. With this bug, no such widget will remain (print, new window and full screen all end up in the menu panel). So the test will need to add its own toolbarbutton to the palette in both windows for this to work well. So in the task, between these lines: yield otherWin.PanelUI.ensureReady(); ok(CustomizableUI.inDefaultState, "Should start in default state"); Add a function call that appends a XUL toolbar button like the sync button (new ID; it probably needs a class name, probably nothing else but might need an icon to make all the dragging work properly cross-platform?) *in both the window the test runs in and a new window* in the palette (gNavToolbox.palette) . Then update the ID for the XUL widget to that button's ID, and it should Just Work. Does that make sense? I'm sorry this is such a pain. :-(

I had a change of heart here. The SyncedTabs module does mock and use Sync itself, which I think is appropriate. However, tests that just use SyncedTabs don't really need to mock all the way down to the Sync level - just mocking that SyncedTabs module should be fine and simpler. So there's no longer a test module. Code that uses SyncedTabs (like the UI patch here) will just be able to set SyncedTabs._internal to some other object and simply change what the SyncedTabs module returns. The module itself is the same as the last version, just the test story changed.

Silly mistake in the last version :)

FTR, a new version of the "main" patch that Gijs previously gave feedback on - I'll probably ask for another feedback round soon but there are still a few obvious rough edges.

(In reply to :Gijs Kruitbosch from comment #26) > Add a function call that appends a XUL toolbar button like the sync button > (new ID; it probably needs a class name, probably nothing else but might > need an icon to make all the dragging work properly cross-platform?) *in > both the window the test runs in and a new window* in the palette > (gNavToolbox.palette) . Then update the ID for the XUL widget to that > button's ID, and it should Just Work. Does that make sense? It does, thanks. This patch fixes that test and another similar test - other failing tests are due to the functionality change rather than this simple use of sync-button as a placeholder for "generic xul button". If this looks OK, please let me know if you'd prefer me to roll it up in the "main" patch or leave it split out this way - I'd like to help make final reviews as painless as possible :)

Comment on attachment 8688298 [details] [diff] [review] 0006-Fix-browser_876926_customize_mode_wrapping.js-and-br.patch Review of attachment 8688298 [details] [diff] [review]: ----------------------------------------------------------------- Yup, this is more or less what I had in mind. Thanks! ::: browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js @@ +146,5 @@ > +function createXULButtonForWindow(win) { > + let button = document.createElement("toolbarbutton"); > + button.id = kXULWidgetId; > + button.setAttribute("style", "min-height: 100px"); > + win.gNavToolbox.palette.appendChild(button); Can you update the createDummyXULButton method in head.js (to take a window) and use that instead? :-) @@ +158,5 @@ > otherWin = yield openAndLoadWindow(null, true); > yield otherWin.PanelUI.ensureReady(); > + // Create the XUL button to use in the test in both windows. > + createXULButtonForWindow(window); > + createXULButtonForWindow(otherWin); The button should be removed at the end of the test. ::: browser/components/customizableui/test/browser_975719_customtoolbars_behaviour.js @@ +127,5 @@ > "Attribute should be gone in new window"); > yield promiseWindowClosed(newWindow); > > ok(CustomizableUI.inDefaultState, "Should be in default state after reset."); > + let syncButton = document.getElementById(kXULWidgetId); Nit: rename from 'syncButton' and update the test assertions further down. :-)

(In reply to Mark Hammond [:markh] from comment #30) > If this looks > OK, please let me know if you'd prefer me to roll it up in the "main" patch > or leave it split out this way - I'd like to help make final reviews as > painless as possible :) Either works for me. I presume that we need to fix other tests as well, but maybe I'm being too pessimistic? You could do all the test changes in a single patch, that'd wfm, or if there's not much else, roll it in with the rest of the changes. If you're very interested in making this painless for the reviewer, you could submit final reviews via mozreview! ;-)

I'm attaching a .zip file with screenshots to show the current state. There are 6 states we care about, and each of these states is shown when anchored to the hamburger panel and when anchored to a toolbar icon - so there are 12 images in total: For when anchored to the panel: > remotetabs-panel-signedout.png - when the user is not signed in to sync. > remotetabs-panel-needsreauth.png - when the user is signed in but needs to reauthenticate > remotetabs-panel-noclients.png - when the user is signed in and syncing, but this is the only device. > remotetabs-panel-tabsdisabled.png - when the user is signed in and syncing, but tabs are disabled. > remotetabs-panel-fetching.png - when the panel is opened immediately after starting the browser and we are currently fetching the list of tabs > remotetabs-panel-tabs.png - when we have tabs. The files with "-toolbar-" in the name instead of "-panel-" are the same states but when accessed via the toolbar menu. I'll attach 2 panorama images next, but I don't have a good tool for that, so each of the individual images were resized and thus is difficult to review. Ryan and Bryan, please note: * Bryan is still working on a different toolbar icon for this, which will show the existing sync spinner on a tab - that new icon obviously isn't reflected here. * The "client" rows (ie, the devices) have CSS "font-size: 1.1em;" so they are slightly bigger than the list of tab. Similarly, the labels in the "error-like" states (ie, the "Sign in to Firefox from your other devices..." label, "Open Sync Prefs" button, etc) use "font-size: 1.3em;". Gijs wanted to make sure that we *really* want these custom sizes to help themes. My take is "yes, we do", but yours may be different!

(In reply to :Gijs Kruitbosch from comment #32) > If you're very interested in making this painless for the reviewer, you > could submit final reviews via mozreview! ;-) I'll try, but last I tried, git and reviewboard don't really get on (and I just learned you can't request feedback via reviewboard). How much does it *actually* bother you? But in the meantime, I'll get the next round of feedback via splintr :(

Hey Drew, You've done a fair bit of reviewing of browser-syncui, so I'm hoping you can take a look at this for me. This patch leverages broadcasters to keep the various sync UI elements sane. Rather than adding yet another button ID to browser-syncui, we now just update the broadcaster and let all the elements automagically get the attributes.

Gijs, This patch is just the tests that used sync-button as a placeholder for a XUL button. I felt it better to keep the other test changes that are related to the functionality in the patch that actually changes the functionality. I think this is small enough it's ready for review.

Gijs, This is the main patch you've previously given feedback on. All tests pass (including new ones) and I think this is getting close to ready, pending UX review.

Gijs, This is a fairly small patch that arranges for the sync-button to be automatically placed in the panel with either a user signs in to Sync, plus a one-off check for users who are already signed into Sync (who otherwise wouldn't get any default placement for the button). Apologies for so many review requests (there's one more after this), but as usual, please feel free to delegate - I'm just picking on you because you have the most context here.

This patch replaces the "Tabs from other devices" entries in the history panel and menu with a "Synced Tabs" entry that opens the new panel.

Comment on attachment 8689366 [details] [diff] [review] 0007-Bug-1201331-part-5-replace-Tabs-from-other-devices-w.patch Got a bit ahead of myself here :)

I'm sorry, I likely won't get to this until Friday (tomorrow).

In IRC: * Bryan tells me that he and shorlander decided to not update the icon for the button - so we are sticking with the current existing Sync icon. * Ryan tells me the "sign in" button should be 200px and the image should be 180px, and specified the exact colors for that button. This patch makes those changes - it scales the illustration, but Bryan said he will supply me with either a svg or one that's exactly 180px wide + the @2x version.

This is how the "sign in" panel now looks

Comment on attachment 8689361 [details] [diff] [review] 0004-Bug-1201331-part-2-update-tests-that-use-the-sync-bu.patch Review of attachment 8689361 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks! I believe this can land independent of the other things. :-) ::: browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js @@ +147,5 @@ > + createDummyXULButton(kXULWidgetId, "test-button", win); > +} > + > +function removeXULButtonForWindow(win) { > + let button = win.gNavToolbox.palette.querySelector(`#${kXULWidgetId}`); Nice! I think technically you might want CSS.escape(kXULWidgetId) but it doesn't matter for the ID we've picked, and this is neater, so let's leave it like this. @@ +148,5 @@ > +} > + > +function removeXULButtonForWindow(win) { > + let button = win.gNavToolbox.palette.querySelector(`#${kXULWidgetId}`); > + button.parentNode.removeChild(button); Nit: button.remove() Probably don't even need the temp variable then. :-)

Comment on attachment 8689365 [details] [diff] [review] 0006-Bug-1201331-part-4-automatically-place-the-Synced-Ta.patch Review of attachment 8689365 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-fxaccounts.js @@ +233,5 @@ > }, > > + ensureSyncButtonPlaced() { > + if (CustomizableUI.getPlacementOfWidget("sync-button") == null) { > + CustomizableUI.addWidgetToArea("sync-button", CustomizableUI.AREA_PANEL); So after this, if a user uses CustomizableUI.reset() ("Restore Defaults" in Customize Mode), this button is going to go and be reverted to its default spot (ie the palette), and won't be put back again. I don't think we can really do anything about this except by making the button's default spot be the palette but hiding (setAttribute("hidden", "true") on the button) to avoid it being visible to non-sync-users. That would avoid having to migrate anything here, though... @@ +244,5 @@ > + // here we are handling a user already being logged in, so they missed that > + // placement when they did. > + checkSyncButtonMigration(getSignedInUser = fxAccounts.getSignedInUser /* for tests */) { > + const kPrefName = "browser.sync.checkedSyncButtonPlacement"; > + if (!Services.prefs.prefHasUserValue(kPrefName)) { I'm not an amazing fan of this kind of thing because now a distribution can't disable this, because they modify the default prefs, right (is that true? I'm not 100% sure, but it would stand to reason...) ? Can we also check the pref value itself? :-) @@ +245,5 @@ > + // placement when they did. > + checkSyncButtonMigration(getSignedInUser = fxAccounts.getSignedInUser /* for tests */) { > + const kPrefName = "browser.sync.checkedSyncButtonPlacement"; > + if (!Services.prefs.prefHasUserValue(kPrefName)) { > + return getSignedInUser().then(userData => { I think this is bound to fxAccounts by the copyObjectProperties thing, but it'd be good to check. Also, are we sure the fxaccounts object exists whenever this is called? ::: browser/base/content/test/general/browser.ini @@ +531,5 @@ > [browser_aboutTabCrashed.js] > skip-if = !e10s || !crashreporter > [browser_aboutTabCrashed_clearEmail.js] > skip-if = !e10s || !crashreporter > +[browser_syncbutton_autoplacement.js] Please put this in the CUI test directory instead. We really need to kill the dumping ground that browser/base/content/test/general/ has become. ::: browser/base/content/test/general/browser_syncbutton_autoplacement.js @@ +3,5 @@ > +const { ONVERIFIED_NOTIFICATION } = Cu.import("resource://gre/modules/FxAccountsCommon.js", {}); > + > +add_task(function* testPlacementDefault() { > + // Ensure the Sync button is placed in the pallete > + CustomizableUI.removeWidgetFromArea("sync-button"); This shouldn't be necessary. The test should just fail if other tests leave the button hanging about somewhere else. @@ +33,5 @@ > + // Ensure the Sync button is placed in the pallete > + CustomizableUI.removeWidgetFromArea("sync-button"); > + is(CustomizableUI.getPlacementOfWidget("sync-button"), null, "button starts with no placement"); > + > + const kPrefName = "browser.sync.checkedSyncButtonPlacement"; You should verify that it hasn't been written at this point because otherwise the check after the first move is not useful. I think because of how sync initialization works that might not actually be true at this point in time, so you may need to reset it beforehand like you do inbetween the other checks. :-) @@ +43,5 @@ > + // Should be no placement but with the pref written that we've done the check. > + is(CustomizableUI.getPlacementOfWidget("sync-button"), null, "didn't place it"); > + is(Services.prefs.getBoolPref(kPrefName), true, "pref was written"); > + > + // Do it again, but this time when a user *is* logged in, button is still in the pallete. Seems like this could be another subtest, right? (ie separate add_task()) @@ +52,5 @@ > + let placement = CustomizableUI.getPlacementOfWidget("sync-button"); > + is(placement.area, CustomizableUI.AREA_PANEL, "button was moved to the panel"); > + is(Services.prefs.getBoolPref(kPrefName), true, "pref was written"); > + > + // check we don't re-move it if already placed. Same.

(In reply to Mark Hammond [:markh] from comment #36) > How much does it > *actually* bother you? But in the meantime, I'll get the next round of > feedback via splintr :( I mean, I get better diffs and free proper interdiffs from mozreview, so it does make reviewing things like this (repeatedly!) a lot easier. But yes, if you're using git I don't think that the git-cinnabar or moz-git-tools stuff has proper integration with mozreview yet. Planned for this quarter, though, AIUI. Might be worth talking to mcote to check when that's supposed to be done.

Comment on attachment 8689899 [details] [diff] [review] 0005-Bug-1201331-part-3-add-a-Synced-Tabs-panel.-f-Gijs.patch Review of attachment 8689899 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_syncui.js @@ +52,5 @@ > }); > } > > function checkButtonTooltips(stringPrefix) { > + for (let butId of ["PanelUI-remotetabs-syncnow", "PanelUI-fxa-icon"]) { Looks like you forgot to update this? Or am I confused? ::: browser/components/customizableui/CustomizableWidgets.jsm @@ +311,5 @@ > + deckIndices: { > + DECKINDEX_TABS: 0, > + DECKINDEX_TABSDISABLED: 1, > + DECKINDEX_FETCHING: 2, > + DECKINDEX_NOCLIENTS: 3, Ewwwww. I forgot how much I hate decks. Rage against bug 390724. Not your fault. But ugh. @@ +343,5 @@ > + }, > + _tabsList: null, > + observe(subject, topic, data) { > + switch (topic) { > + case "services.sync.tabs.update": Does this fire when syncing tabs is disabled? @@ +354,5 @@ > + _showTabs(isFirstShow = false) { > + let doc = this._tabsList.ownerDocument; > + let deck = doc.getElementById("PanelUI-remotetabs-deck"); > + if (!SyncedTabs.isConfiguredToSyncTabs) { > + deck.selectedIndex = this.deckIndices.DECKINDEX_TABSDISABLED; because if not, this should probably happen inside onViewShowing so we only assign to the selectedIndex once. @@ +360,5 @@ > + } > + // XXX - this is promise-based, but it's probably not necessary to > + // handle multiple in-flight calls as we clear and rebuild the list > + // after the promise is resolved, so multiple calls shouldn't cause > + // a problem in practice. What if they don't resolve in FiFo order? @@ +367,5 @@ > + if (!this._tabsList) { > + return; > + } > + if (clients.length === 0 && isFirstShow) { > + // the "fetching tabs" deck is being shown - let's leave it there. Wait, is this just going to keep showing "Fetching synced tabs..." indefinitely while we're sure that we have no clients and are no longer fetching? That seems wrong... Why does isFirstShow == true need special treatment here? @@ +388,5 @@ > + let separator = doc.createElementNS(kNSXUL, "menuseparator"); > + fragment.appendChild(separator); > + } > + this._appendClient(client, fragment); > + firstClient = false; I wonder what you think of getting rid of the firstClient bool, which I find slightly icky, and doing something like: if (fragment.lastChild) { // add separator } instead. @@ +401,5 @@ > + }, > + _clearTabList () { > + let list = this._tabsList; > + while (list.lastChild) { > + list.removeChild(list.lastChild); Nit: list.lastChild.remove() @@ +437,5 @@ > + attachFragment.appendChild(tabEnt); > + } > + } > + }, > + _createTabElement(doc, attrs) { Nit: we're setting attributes here but the keys (property names) in this object don't correspond to the attribute names that we're using. Could just call the parameter "tabInfo" or something. @@ +440,5 @@ > + }, > + _createTabElement(doc, attrs) { > + let win = doc.defaultView; > + let item = doc.createElementNS(kNSXUL, "toolbarbutton"); > + item.setAttribute("type", "tab"); The type attribute on toolbarbutton has XBL meanings. It'd probably be a good idea to use a different attribute, even if it was just "itemtype" or whatever. @@ +445,5 @@ > + item.setAttribute("class", "subviewbutton"); > + item.setAttribute("targetURI", attrs.url); > + item.setAttribute("label", attrs.title != "" ? attrs.title : attrs.url); > + item.setAttribute("image", attrs.icon); > + item.addEventListener("click", e => { Why did you change this from "command" ? @@ +457,5 @@ > + const maxTabs = 15; > + for (let client of clients) { > + let tabs = client.tabs; > + tabs.sort((a, b) => b.lastUsed - a.lastUsed); > + client.tabs = tabs.slice(0, maxTabs); Are we OK modifying these objects? Nobody else relying on them etc. ? :-) (It seems SyncedTabs.jsm moved to a different bug, and it *seems* that we create new objects there every time we call getTabClients(), but it would be good to be sure.) ::: browser/components/customizableui/content/panelUI.inc.xul @@ +107,5 @@ > label="&appMenuHistory.showAll.label;" > oncommand="PlacesCommandHook.showPlacesOrganizer('History'); CustomizableUI.hidePanelForNode(this);"/> > </panelview> > > +#ifdef MOZ_SERVICES_SYNC OOI: who builds desktop without sync? Can we just kill this build flag? @@ +111,5 @@ > +#ifdef MOZ_SERVICES_SYNC > + <panelview id="PanelUI-remotetabs" flex="1" class="PanelUI-subView"> > + <label value="&appMenuRemoteTabs.label;" class="panel-subview-header"/> > + <vbox class="panel-subview-body"> > + <!-- this widget has 3 vboxes in the body, but only 1 is ever visible --> This comment is now out of date because 2 of them are hboxes. @@ +154,5 @@ > + <label class="PanelUI-remotetabs-instruction-label">&appMenuRemoteTabs.noclients.label;</label> > + <label class="PanelUI-remotetabs-mobile-promo"> > + &appMenuRemoteTabs.mobilePromo.start;<!-- We put these comments to avoid inserting white spaces > + --><label class="text-link remotetabs-promo-link" > + href="https://www.mozilla.org/firefox/android/?utm_source=firefox-browser&amp;utm_medium=firefox-browser&amp;utm_campaign=synced-tabs"><!-- It feels pretty yucky to have this URL hardcoded in here. My brain has temporarily melted and I forgot how we're supposed to do this. Maybe just assign from JS? Maybe use a pref? Unsure. TBH, the l10n stuff here would probably be clearer if we used a .properties file and a formatted string with replacement characters. That'd also avoid all the comment-based whitespace avoidance hacks. :-) @@ +182,5 @@ > + </hbox> > + <!-- When Sync needs re-authentication --> > + <hbox> > + <spacer flex="1"/> > + <vbox id="PanelUI-remotetabs-reauthsync" flex="1" observes="sync-reauth-state"> The content of this thing is identical to the sync-setup-state. Shouldn't the messaging be different? ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +683,5 @@ > +} > + > +.PanelUI-remotetabs-prefs-button { > + -moz-appearance: none; > + background-color: #0196dd; Note: this only passes WCAG AA contrast checks for large text, which would be >=14pt + bold or >=18pt (and normal font-weight), which the text isn't. It would be good if UX picked a darker shade for the background to alleviate this problem. Something like #016ea1 would work but I don't know if that looks ugly and/or UX wants to pick something else. See http://webaim.org/resources/contrastchecker/ for an easy-to-use tool to check this. @@ +688,5 @@ > + color: white; > + border-radius: 2px; > + margin: 10px; > + padding: 8px; > + border: 0px; Nit: none; Do you really need to set this? @@ +695,5 @@ > + max-width: 200px; > +} > + > +.PanelUI-remotetabs-prefs-button:hover, > +.PanelUI-remotetabs-prefs-button:active { We normally use :hover:active, I think. @@ +712,5 @@ > +} > + > +.fxaSyncIllustration { > + width: 180px; /* XXX - need a correctly scaled image */ > + list-style-image: url(chrome://browser/skin/fxa/sync-illustration.png) Nit: missing semicolon Note that we still need to hg add these images and to add them to jar.mn . @@ +717,5 @@ > +} > + > +@media (min-resolution: 1.1dppx) { > + .fxaSyncIllustration { > + list-style-image: url(chrome://browser/skin/fxa/sync-illustration@2x.png) Ditto. @@ +723,5 @@ > +} > + > +/* use a very specific rule for the remotetabs "prefs" button to override > + a text-align: left; without resorting to using !important */ > +panel .PanelUI-remotetabs-prefs-button > .toolbarbutton-text { Do you need the tag name descendant selector? Can't we just use the tagname of PanelUI-remotetabs-prefs-button? If that doesn't work, tbh I'd rather use !important with an explanation. @@ +729,5 @@ > + text-shadow: none; > +} > + > +#PanelUI-remotetabs[mainview] { /* panel anchored to toolbar button might be too skinny */ > + min-width: 220px; This might need to be in em? @@ +739,5 @@ > +#PanelUI-remotetabs[mainview] #PanelUI-remotetabs-setupsync, > +#PanelUI-remotetabs[mainview] #PanelUI-remotetabs-reauthsync, > +#PanelUI-remotetabs[mainview] #PanelUI-remotetabs-nodevicespane, > +#PanelUI-remotetabs[mainview] #PanelUI-remotetabs-tabsdisabledpane { > + min-height: 350px; Is this based on the length/wrapping of the label? If so it should be a localized entity and you should set it in the markup. You should also use em instead of px so it continues working if people use a larger text size. Anyway, localized because the label length will change in different locales and they will then have the same issue again. @@ +1161,2 @@ > #PanelUI-historyItems > toolbarbutton { > list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.png"); Is there no @2x equivalent to this rule? If not please add one; I'm pretty sure we do ship this icon in a @2x version everywhere.

(In reply to :Gijs Kruitbosch from comment #49) > Ewwwww. I forgot how much I hate decks. Rage against bug 390724. Not your > fault. But ugh. Yeah :( I meant to explicitly ask about this - given the .css needed to make it sane, I'm thinking I should just manage the visibility of the few boxes manually and skip the deck completely - what do you think?

(In reply to :Gijs Kruitbosch from comment #47) > So after this, if a user uses CustomizableUI.reset() ("Restore Defaults" in > Customize Mode), this button is going to go and be reverted to its default > spot (ie the palette), and won't be put back again. > > I don't think we can really do anything about this except by making the > button's default spot be the palette but hiding (setAttribute("hidden", > "true") on the button) to avoid it being visible to non-sync-users. That > would avoid having to migrate anything here, though... hmm - that's a great idea, thanks. But how to manage that state? It seems the choices are, basically: * browser-syncui does a PanelUI.js dance to find all widgets and updates the state - and it would need to do this at startup :( * A quick looks shows me might be able to treat widget.hidden the same as widget.disabled. The widget definition then grows a getter (which just checks the services.sync.username pref) and browser-syncui then only needs to track state changes (ie, do nothing special on startup) and even then just update the widget itself. Does that sound about right, and if so, which do you think the better option? > I'm not an amazing fan of this kind of thing because now a distribution > can't disable this, because they modify the default prefs, right (is that > true? I'm not 100% sure, but it would stand to reason...) ? Can we also > check the pref value itself? :-) If we take the above approach, this can all die IIUC.

(In reply to Mark Hammond [:markh] from comment #51) > (In reply to :Gijs Kruitbosch from comment #47) > > So after this, if a user uses CustomizableUI.reset() ("Restore Defaults" in > > Customize Mode), this button is going to go and be reverted to its default > > spot (ie the palette), and won't be put back again. > > > > I don't think we can really do anything about this except by making the > > button's default spot be the palette but hiding (setAttribute("hidden", > > "true") on the button) to avoid it being visible to non-sync-users. That > > would avoid having to migrate anything here, though... For clarity, I meant the *panel*, not the palette, for the default area for the button. > hmm - that's a great idea, thanks. But how to manage that state? It seems > the choices are, basically: > * browser-syncui does a PanelUI.js dance to find all widgets and updates the > state - and it would need to do this at startup :( I mean, it could just be hidden by default and be shown as soon as sync has finished deciding that it needs to be shown (ie push rather than pull updates). That will work in 99% of cases... I think the 1% might be starting up in customization mode or whatever, and/or being in the middle of dragging bits of the panel around... code there is going to get confused if the contents of the panel change mid-drag. Not worth worrying too much about, most likely. > * A quick looks shows me might be able to treat widget.hidden the same as > widget.disabled. The widget definition then grows a getter (which just > checks the services.sync.username pref) and browser-syncui then only needs > to track state changes (ie, do nothing special on startup) and even then > just update the widget itself. Right, I think this is more or less what I just said, maybe, but with the getter as sugar? You can already set the hidden attribute in widget.onCreated. :-) > Does that sound about right, and if so, which do you think the better option? Not too fussed. Pick whichever sounds best to you. > > I'm not an amazing fan of this kind of thing because now a distribution > > can't disable this, because they modify the default prefs, right (is that > > true? I'm not 100% sure, but it would stand to reason...) ? Can we also > > check the pref value itself? :-) > > If we take the above approach, this can all die IIUC. I believe that is correct.

Comment on attachment 8689899 [details] [diff] [review] 0005-Bug-1201331-part-3-add-a-Synced-Tabs-panel.-f-Gijs.patch Review of attachment 8689899 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the (typically) thorough review! Anything I didn't mention I've fixed. I'll be attaching a new patch shortly. ::: browser/base/content/test/general/browser_syncui.js @@ +52,5 @@ > }); > } > > function checkButtonTooltips(stringPrefix) { > + for (let butId of ["PanelUI-remotetabs-syncnow", "PanelUI-fxa-icon"]) { The sync-button no longer has its tooltip updated to reflect the most-recent sync - it always keeps the static "Show your Synced Tabs from other devices" - however, the "sync now" button inside the panel now has the dynamic tooltip - so this test change reflects that. ::: browser/components/customizableui/CustomizableWidgets.jsm @@ +360,5 @@ > + } > + // XXX - this is promise-based, but it's probably not necessary to > + // handle multiple in-flight calls as we clear and rebuild the list > + // after the promise is resolved, so multiple calls shouldn't cause > + // a problem in practice. heh - fair enough - I've changed that so the resolve in order. @@ +367,5 @@ > + if (!this._tabsList) { > + return; > + } > + if (clients.length === 0 && isFirstShow) { > + // the "fetching tabs" deck is being shown - let's leave it there. On startup before the first "normal" Sync, we will get an empty list of clients back. A background sync has started, so we can expect the observer to fire "soon". So when isFirstShow is true and we have no clients we assume that we simply haven't synced yet. If isFirstShow is false and we have no clients, we assume that means we *really* have no clients. BUT - on reflection, that's all too confusing and is a proxy for "has the tabs engine synced yet" - so I updated the patch for SyncedTabs.jsm (bug 1225690) to have an explicit flag we can query - that avoid us trying to guess, and I've updated this patch to use that. Does that make sense? @@ +445,5 @@ > + item.setAttribute("class", "subviewbutton"); > + item.setAttribute("targetURI", attrs.url); > + item.setAttribute("label", attrs.title != "" ? attrs.title : attrs.url); > + item.setAttribute("image", attrs.icon); > + item.addEventListener("click", e => { I added the comment: // We need to use "click" instead of "command" here so openUILink // respects different buttons (eg, to open in a new tab). @@ +457,5 @@ > + const maxTabs = 15; > + for (let client of clients) { > + let tabs = client.tabs; > + tabs.sort((a, b) => b.lastUsed - a.lastUsed); > + client.tabs = tabs.slice(0, maxTabs); Yeah, that's a good point - I'll add a comment here and to SyncedTabs to indicate part of the contract is that returned items are never shared. ::: browser/components/customizableui/content/panelUI.inc.xul @@ +107,5 @@ > label="&appMenuHistory.showAll.label;" > oncommand="PlacesCommandHook.showPlacesOrganizer('History'); CustomizableUI.hidePanelForNode(this);"/> > </panelview> > > +#ifdef MOZ_SERVICES_SYNC good question - but that flag has many occurrences in the tree, so is probably beyond the scope of this bug. I opened bug 1227361 for that. @@ +154,5 @@ > + <label class="PanelUI-remotetabs-instruction-label">&appMenuRemoteTabs.noclients.label;</label> > + <label class="PanelUI-remotetabs-mobile-promo"> > + &appMenuRemoteTabs.mobilePromo.start;<!-- We put these comments to avoid inserting white spaces > + --><label class="text-link remotetabs-promo-link" > + href="https://www.mozilla.org/firefox/android/?utm_source=firefox-browser&amp;utm_medium=firefox-browser&amp;utm_campaign=synced-tabs"><!-- Because there are links embedded in the string, I've always been under the impression that using .properties would require markup in the string definition, which is frowned upon? This URL is also used in the sync-prefs pane, which makes it doubly ugly, so I've moved the URLs to a preference. @@ +182,5 @@ > + </hbox> > + <!-- When Sync needs re-authentication --> > + <hbox> > + <spacer flex="1"/> > + <vbox id="PanelUI-remotetabs-reauthsync" flex="1" observes="sync-reauth-state"> Ryan Feeley says this is fine - they end up being taken to the same place. IOW, there's no real value in trying to explain the difference here - it will be clear once they click the button. I added a comment indicating this is intentional and that the different box exists so we can leverage the broadcasters to manage the hidden states. ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +683,5 @@ > +} > + > +.PanelUI-remotetabs-prefs-button { > + -moz-appearance: none; > + background-color: #0196dd; This is the same color scheme as, eg, the "pocket" button and as used on the FxA login pages, and was specified by UX - so I think this is an issue they should address in a followup (and work out how to change all affected UI at the same time). Sound OK? @@ +712,5 @@ > +} > + > +.fxaSyncIllustration { > + width: 180px; /* XXX - need a correctly scaled image */ > + list-style-image: url(chrome://browser/skin/fxa/sync-illustration.png) That image already exists and is used in sync preferences. However, I'm still waiting for one that's 180px wide or a .svg I can use in both places. @@ +723,5 @@ > +} > + > +/* use a very specific rule for the remotetabs "prefs" button to override > + a text-align: left; without resorting to using !important */ > +panel .PanelUI-remotetabs-prefs-button > .toolbarbutton-text { Nope - I couldn't make that work, so I just used !important. @@ +739,5 @@ > +#PanelUI-remotetabs[mainview] #PanelUI-remotetabs-setupsync, > +#PanelUI-remotetabs[mainview] #PanelUI-remotetabs-reauthsync, > +#PanelUI-remotetabs[mainview] #PanelUI-remotetabs-nodevicespane, > +#PanelUI-remotetabs[mainview] #PanelUI-remotetabs-tabsdisabledpane { > + min-height: 350px; The border and padding is also an issue - but note that the height there is bigger than actually necessary. I'm not too keen on making this more complex because (a) that bug will hopefully get fixed, (b) the consequences of it not being high enough are small - you just get a scroll-bar, (c) it's only a problem for users who customize the button to the toolbar and a final (d) it only affects the UI when the button isn't actually useful (ie, sync isn't configured, or there is only 1 client.) So I've just made that 30em, which leaves enough whitespace at the bottom for the text to take ~5 lines before the scroll-bar appears (it is 2 lines in English) @@ +1161,2 @@ > #PanelUI-historyItems > toolbarbutton { > list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.png"); apparently not! I added one.

Comment on attachment 8689365 [details] [diff] [review] 0006-Bug-1201331-part-4-automatically-place-the-Synced-Ta.patch Ryan Feeley tells me that the button should always be placed in the panel, even for users who don't have Sync configured - it is a good promo for Sync's features. Which means we don't need any of this (and I'll change the main patch to arrange for panel placement)

This version makes all requested changes, except as noted in comment 53.

This patch replaces "Tabs from Other Devices" with "Synced Tabs" that opens the new synced tabs UI. Note we no longer track a "disabled" state - if the item is visible it is enabled and those "bad" states are reflected in the main UI.

Comment on attachment 8691231 [details] [diff] [review] 0005-Bug-1201331-part-3-add-a-Synced-Tabs-panel.-f-Gijs.patch Review of attachment 8691231 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/CustomizableWidgets.jsm @@ +464,5 @@ > + item.setAttribute("itemtype", "tab"); > + item.setAttribute("class", "subviewbutton"); > + item.setAttribute("targetURI", tabInfo.url); > + item.setAttribute("label", tabInfo.title != "" ? tabInfo.title : tabInfo.url); > + item.setAttribute("image", attrs.icon); doh! Here and below |attrs| should be |tabInfo|

current screenshots taken on a machine with a DPI of 1. remotetabs-panel-tabsdisabled.png isn't centered correctly which I'm struggling to understand, but am working on it.

Comment on attachment 8691231 [details] [diff] [review] 0005-Bug-1201331-part-3-add-a-Synced-Tabs-panel.-f-Gijs.patch Review of attachment 8691231 [details] [diff] [review]: ----------------------------------------------------------------- Apologies for the slowness; I reviewed the interdiff, please request review next time, I think we're pretty much done here. ::: browser/components/customizableui/CustomizableUI.jsm @@ +169,5 @@ > "add-ons-button", > #ifndef MOZ_DEV_EDITION > "developer-button", > #endif > +#ifdef MOZ_SERVICES_SYNC Nit: per discussion elsewhere, drop the #ifdef. :-) ::: browser/components/customizableui/CustomizableWidgets.jsm @@ +380,5 @@ > + let deck = doc.getElementById("PanelUI-remotetabs-deck"); > + // XXX - this is promise-based, but it's probably not necessary to > + // handle multiple in-flight calls as we clear and rebuild the list > + // after the promise is resolved, so multiple calls shouldn't cause > + // a problem in practice. This comment is now out-of-date, right? @@ +386,5 @@ > + // The view may have been hidden while the promise was resolving. > + if (!this._tabsList) { > + return; > + } > + if (clients.length === 0 && !SyncedTabs.haveSyncedThisSession) { Hrm, I think my confusion is that getTabClients() returns a promise - wouldn't it make more sense to make that promise only resolve after the first sync is complete? ::: browser/components/customizableui/content/panelUI.inc.xul @@ +156,5 @@ > + &appMenuRemoteTabs.mobilePromo.start;<!-- We put these comments to avoid inserting white spaces > + --><label id="PanelUI-remotetabs-mobilepromo-android" > + class="text-link remotetabs-promo-link"><!-- > + -->&appMenuRemoteTabs.mobilePromo.androidLink;</label><!-- > + -->&appMenuRemoteTabs.mobilePromo.iOSBefore;<!-- androidLink and then iOSBefore? That doesn't sound right... As for markup in localized strings, you could keep the markup separately and then replace it into the target string with getFormattedString and friends. Something like: var foo = getString("androidLink"); var fooLink = document.createElement(...); // set attributes fooLink.textContent = foo; var html = fooLink.outerHTML; var completeStr = getFormattedString("promo-text", html); It wouldn't need to go into the localized strings itself (though that, too, is not unheard of). Another reason to do this is so that localizations can add text before the android link if that is required in their locale, etc. etc. ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css @@ +683,5 @@ > +} > + > +.PanelUI-remotetabs-prefs-button { > + -moz-appearance: none; > + background-color: #0196dd; You seem to have still adjusted this? :-) That works, though you can also keep it as-is and file a followup bug, if this style is identical to what we use elsewhere. I take it we can't share those styles?

Comment on attachment 8691233 [details] [diff] [review] 0006-Bug-1201331-part-4-replace-Tabs-from-other-devices-w.patch Review of attachment 8691233 [details] [diff] [review]: ----------------------------------------------------------------- f+ on code, I'm assuming someone else has said "yes, we're killing about:sync-tabs" ? ::: browser/base/content/browser-syncui.js @@ +314,5 @@ > + openSyncedTabsPanel() { > + let placement = CustomizableUI.getPlacementOfWidget("sync-button"); > + let area = placement ? placement.area : CustomizableUI.AREA_NAVBAR; > + let anchor = document.getElementById("sync-button") || > + document.getElementById("PanelUI-menu-button"); I mean, f+ on the approach here, though obviously we're losing keyboard a11y assuming the in-content page was keyboard-accessible (seems to have been, on Windows, though it was very clumsy - AFAICT you need to use the context menu). Can you file a followup bug on that? ::: browser/components/customizableui/CustomizableWidgets.jsm @@ -254,5 @@ > } else { > tabsFromOtherComputers.setAttribute("hidden", true); > } > - > - if (PlacesUIUtils.shouldEnableTabsFromOtherComputersMenuitem()) { I don't see you using or removing this method. Does it need to live? Especially now we're no longer disabling things here? ::: browser/locales/en-US/chrome/browser/aboutSyncTabs.dtd @@ -1,5 @@ > <!-- This Source Code Form is subject to the terms of the Mozilla Public > - License, v. 2.0. If a copy of the MPL was not distributed with this > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > > -<!-- LOCALIZATION NOTE (tabs.otherDevices.label): Keep this in sync with syncTabsMenu2.label from browser.dtd --> Err, I presume we're going to just kill this page, if we're removing all the UI entrypoints? Is there some reason not to?

(In reply to Mark Hammond [:markh] from comment #54) > > Ryan Feeley tells me that the button should always be placed in the panel, > even for users who don't have Sync configured - it is a good promo for > Sync's features. Got confirmation from Madhava that this is okay, although he wants us to use a different icon. Bryan Bell is on that (NIing him). I think it might end up just being a tab icon.

(In reply to :Gijs Kruitbosch from comment #60) > Err, I presume we're going to just kill this page, if we're removing all the > UI entrypoints? Is there some reason not to? The reason we aren't killing it completely is "cloud sync" - we can't kill cloud sync completely (bug 1184003) but the new SyncedTabs module doesn't support cloud sync (as no one seems to know how to set it up or test it :/). So the compromise is to leave about:synced-tabs in the tree for CloudSync users to open manually.

Comment on attachment 8691233 [details] [diff] [review] 0006-Bug-1201331-part-4-replace-Tabs-from-other-devices-w.patch Review of attachment 8691233 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/CustomizableWidgets.jsm @@ -254,5 @@ > } else { > tabsFromOtherComputers.setAttribute("hidden", true); > } > - > - if (PlacesUIUtils.shouldEnableTabsFromOtherComputersMenuitem()) { PlacesUIUtils.shouldEnableTabsFromOtherComputersMenuitem() is removed in this patch - am I missing something?

(In reply to :Gijs Kruitbosch from comment #59) > Apologies for the slowness; lol - I don't think anyone could accuse you of being tardy with feedback - thanks! > Hrm, I think my confusion is that getTabClients() returns a promise - > wouldn't it make more sense to make that promise only resolve after the > first sync is complete? I'm not particularly comfortable with that - at this stage I prefer the approach of the notification being the canonical representation of "tabs are available". Plus, Sync usually will have done its thing - so I hope you can live with this :) > As for markup in localized strings, you could keep the markup separately and > then replace it into the target string with getFormattedString and friends. > Something like: Cool, thanks. I made that change here, but left the existing Sync prefs alone. > It wouldn't need to go into the localized strings itself (though that, too, > is not unheard of). I remember Gavin telling me off for trying it once ;) > > + background-color: #0196dd; > > You seem to have still adjusted this? :-) It's now #0096dd; - I suspect the 01 was someone's color picker lieing to them :) This is the same as pocket, and you'll see I opened bug 1227844. > this style is identical to what we use elsewhere. I take it we can't share > those styles? Not that I can see (pocket and hello are somewhat special snowflakes ;) accounts.firefox.com uses the same scheme too :/ I also simplified the markup, added a .svg for the scalable illustration and added a bit of padding around it - rfeeley gave a thumbs-up over IRC - I'll upload screenshots. Per comment 61, it still needs a new icon, but I hope you can still review it now.

Comment on attachment 8691233 [details] [diff] [review] 0006-Bug-1201331-part-4-replace-Tabs-from-other-devices-w.patch You'll see I opened the a11y bug, so I think this is ready to go too?

Latest screenshots taken on windows with DPI of 1.0.

Comment on attachment 8691836 [details] [diff] [review] 0005-Bug-1201331-part-3-add-a-Synced-Tabs-panel.-r-Gijs.patch Review of attachment 8691836 [details] [diff] [review]: ----------------------------------------------------------------- r+, but still some questions to sort out below. ::: browser/components/customizableui/CustomizableWidgets.jsm @@ +331,5 @@ > + androidLink.setAttribute("class", "text-link remotetabs-promo-link"); > + let iosLink = doc.createElement("label"); > + iosLink.textContent = bundle.getString("appMenuRemoteTabs.mobilePromo.ios"); > + iosLink.setAttribute("href", Services.prefs.getCharPref("identity.mobilepromo.ios") + "synced-tabs"); > + iosLink.setAttribute("class", "text-link remotetabs-promo-link"); I am nittypicky: let formatArgs = ["android", "ios"].map(os => { let link = doc.createElement("label"); link.textContent = bundle.getString(`appMenuRemoteTabs.mobilePromo.${os}`) link.href = Services.prefs.getCharPref(`identity.mobilepromo.${os}`) + "synced-tabs"; link.className = "text-link remotetabs-promo-link"; return link.outerHTML; }); looks a little neater and saves you some repetition here and in passing to the getFormattedString call. :-) @@ +333,5 @@ > + iosLink.textContent = bundle.getString("appMenuRemoteTabs.mobilePromo.ios"); > + iosLink.setAttribute("href", Services.prefs.getCharPref("identity.mobilepromo.ios") + "synced-tabs"); > + iosLink.setAttribute("class", "text-link remotetabs-promo-link"); > + // Put it all together... > + let full = bundle.getFormattedString("appMenuRemoteTabs.mobilePromo", [ nit: s/full/contents/ ::: browser/components/customizableui/content/panelUI.inc.xul @@ +121,5 @@ > + closemenu="none"/> > + <toolbarbutton id="PanelUI-remotetabs-opensidebar" > + class="subviewbutton" > + label="&appMenuRemoteTabs.openSidebar.label;" > + hidden="true"/> Err, is interdiff lying at me or did these buttons swap around? If so, why? :-) @@ +186,5 @@ > + flex="1" > + align="center" > + class="PanelUI-remotetabs-instruction-box" > + observes="sync-reauth-state"> > + <hbox pack="center"> The nesting here is really confusing. I'm not convinced you need the toplevel hbox - can't you just use the vbox as the container and give that the padding? You have an hbox. Then you have a vbox with flex, so that will fill the whole width of the hbox, and because the hbox's align and pack attributes are the default, it will also fill the whole height (both minus padding of the outer hbox). Then you set align="center" on the vbox. If I'm reading MDN right, that will mean the children of the element are horizontally centered. Then you have more hboxes which have pack="center", which again, if I'm reading MDN right, will mean the children of the element are horizontally centered. Aren't those unnecessary? Am I missing something? ::: browser/themes/shared/fxa/sync-illustration.svg @@ +1,5 @@ > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" preserveAspectRatio="xMidYMid" width="320" height="280" viewBox="0 0 320 280"> > + <defs> > + <style> > + .cls-1 { > + fill: #cdcdce; So I posit that no human can see the difference between #cdcdce and #cdcdcd, and that the difference here is not intentional, just like that #01... vs #00... that we ran into before. Just make all of them be filled with the same color (#cdcdcd because that's easier to type), and: - add a license header; - remove all the class attributes; - remove the id attributes; - if we need the fill-rule, just stick it on path {} instead of a double class selector; - remove the extra level of nested <g> which is not doing anything, afaict - in fact, I bet you can remove both of them with no loss in appearance; - remove the unused xlink namespace declaration; See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines for our guidelines for SVG. :-) ::: browser/themes/shared/jar.inc.mn @@ +79,5 @@ > skin/classic/browser/fxa/logo.png (../shared/fxa/logo.png) > skin/classic/browser/fxa/logo@2x.png (../shared/fxa/logo@2x.png) > skin/classic/browser/fxa/sync-illustration.png (../shared/fxa/sync-illustration.png) > skin/classic/browser/fxa/sync-illustration@2x.png (../shared/fxa/sync-illustration@2x.png) > + skin/classic/browser/fxa/sync-illustration.svg (../shared/fxa/sync-illustration.svg) Can we replace the png versions everywhere? Can be a followup bug.

Comment on attachment 8691233 [details] [diff] [review] 0006-Bug-1201331-part-4-replace-Tabs-from-other-devices-w.patch Review of attachment 8691233 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-syncui.js @@ +323,5 @@ > + PanelUI.showSubView("PanelUI-remotetabs", anchor, area); > + }).catch(Cu.reportError); > + } else { > + // It is placed somewhere else - just try and show it. > + PanelUI.showSubView("PanelUI-remotetabs", anchor, area); We need to unify this approach here sometime, UX complains about how we anchor all of these differently. Not here, but if you want to poke at that, I would love someone to straighten that out (literally!). ::: browser/components/customizableui/CustomizableWidgets.jsm @@ -254,5 @@ > } else { > tabsFromOtherComputers.setAttribute("hidden", true); > } > - > - if (PlacesUIUtils.shouldEnableTabsFromOtherComputersMenuitem()) { I was missing something - sorry!

Some UX issues: Sync icon and animation is not required in states depicted in: remotetabs-panel-fetching.png remotetabs-panel-noclients.png remotetabs-panel-tabs.png remotetabs-panel-tabsdisabled.png remotetabs-toolbar-fetching.png remotetabs-toolbar-noclients.png remotetabs-toolbar-tabs.png remotetabs-toolbar-tabsdisabled.png "Fetching Synced Tabs…" is not required in states depicted in images: remotetabs-panel-fetching.png remotetabs-toolbar-fetching.png "Sync now" option is not required in states depicted in: remotetabs-panel-tabsdisabled.png remotetabs-toolbar-tabsdisabled.png Black text should match the SVG colour (#CDCDCD) in states depicted in images: remotetabs-panel-needsreauth.png remotetabs-panel-noclients.png remotetabs-panel-signedout.png remotetabs-panel-tabsdisabled.png remotetabs-toolbar-needsreauth.png remotetabs-toolbar-noclients.png remotetabs-toolbar-signedout.png remotetabs-toolbar-tabsdisabled.png Devices lacking open tabs should simply not appear in states depicted images: remotetabs-panel-tabs.png remotetabs-toolbar-tabs.png Device names should be styled like "No open tabs" that is due for removal in states depicted in images: remotetabs-panel-tabs.png remotetabs-toolbar-tabs.png Perhaps "Open Sync Preferences" is too long and simply "Sync Preferences" will do? remotetabs-panel-tabsdisabled.png remotetabs-toolbar-tabsdisabled.png Thoughts?

(In reply to Ryan Feeley [:rfeeley] from comment #69) > Black text should match the SVG colour (#CDCDCD) in states depicted in > images: > remotetabs-panel-needsreauth.png > remotetabs-panel-noclients.png > remotetabs-panel-signedout.png > remotetabs-panel-tabsdisabled.png > remotetabs-toolbar-needsreauth.png > remotetabs-toolbar-noclients.png > remotetabs-toolbar-signedout.png > remotetabs-toolbar-tabsdisabled.png Hardcoding text color like that is not something we can reasonably do, because there is no guarantee what the background color is on different OSes/OS themes (cf. windows high contrast mode and friends). We could use GrayText, which might be closer, but still won't be black. The contrast might then be too low again, though, because the background of these things is also grey-ish.

Whatever the grey colour we use for "Paste" when the clipboard is empty, is fine.

I had a quick chat with Ryan in IRC to point out that we have already discussed and iterated on some of these issues. He also agreed it was more important to land this in 45 and do some iteration there than to miss 45 completely (which is likely if we spend too much more time), and that I should use my own judgement - so I did :) > Sync icon and animation is not required in states depicted in: > remotetabs-panel-fetching.png > remotetabs-panel-noclients.png > remotetabs-panel-tabs.png > remotetabs-panel-tabsdisabled.png > remotetabs-toolbar-fetching.png > remotetabs-toolbar-noclients.png > remotetabs-toolbar-tabs.png > remotetabs-toolbar-tabsdisabled.png Note that in all cases above (except fetching), Sync is configured and working correctly - it's just that we have no tabs to show. If we remove the Sync Now button, then we will have removed the ability for users configured this way to perform a "Sync Now" from the panel/toolbar UI. If we did that, then we'd probably want to reinstate another button just for "Sync Now" - but I think that would be a backwards step. > "Fetching Synced Tabs…" is not required in states depicted in images: > remotetabs-panel-fetching.png > remotetabs-toolbar-fetching.png This has been there since I took the patch over from Edouard, and if we remove it, then panel has nothing other than the "Sync Now" button - and if we took the suggestion above the panel would be completely empty in this state - which seems wrong. Note also that this will only be seen when the user interacts with the menu before Sync has run for the first time, so hopefully this will be rare. > "Sync now" option is not required in states depicted in: > remotetabs-panel-tabsdisabled.png > remotetabs-toolbar-tabsdisabled.png As above, Sync is working correctly here, and this is the only way to reach "Sync Now" for those users. > Black text should match the SVG colour (#CDCDCD) in states depicted in > images: As per Gij's comment, I've made this GrayText which looks very close to me eyes. However, we should test with high-contrast mode etc, and see if we also want to use CSS to adjust the SVG color. > Devices lacking open tabs should simply not appear in states depicted images: > remotetabs-panel-tabs.png > remotetabs-toolbar-tabs.png We agreed to show the devices with no tabs and the label "No open tabs" to handle the state where tabs are enabled, there are devices connected, but none of those devices have tabs. We can't show either the "tabs disabled" state nor the "no clients" state, so we just end up presenting the user with an empty list. Personally I also think it is reassuring for the user to see their devices listed there in all cases to head-off the "why does a device appear sometimes and sometimes not" questions. From IRC: > 10:02 AM <markh> so one extra state me might need - user has tabs enabled to sync, user *does* have other devices connected, but still *does not* have any remote tabs. That state is likely if the user just enabled tab syncing, but those other devices are yet to sync their tabs. > 10:02 AM <markh> IOW - we display "Turn on Tab syncing to view your tabs", the user then goes and enabled tabs - but when they come back there are still no tabs as the other devices are yet to sync them > 10:03 AM <markh> otherwise we take then to the "connect your other devices" page - but they *have* other devices connected - just no tabs. > 10:04 AM <rfeeley> i think it'd be pretty rare > 10:04 AM <rfeeley> although perhaps it should show the device name… > 10:05 AM <markh> that's actually an interesting question - currently we hide devices with no tabs. The user might prefer to see the device names even if they don't have tabs open on them > 10:05 AM <rfeeley> like "Ryan Feeley’s iPhone 6" and under that in grey "No open tabs" > 10:05 AM <markh> yeah > 10:05 AM <rfeeley> i'll mock that up > Device names should be styled like "No open tabs" that is due for removal in > states depicted in images: > remotetabs-panel-tabs.png > remotetabs-toolbar-tabs.png I've styled the device name so it is only GrayText - no longer bold and no longer 1.1em - although it is now identical to the (still remaining and indented) "No open tabs" line it looks fine IMO. > Perhaps "Open Sync Preferences" is too long and simply "Sync Preferences" > will do? Yeah, agreed, I've made that change.

(In reply to :Gijs Kruitbosch from comment #67) > r+, but still some questions to sort out below. Thanks! I'm just re-requesting review for the svg changes as I wasn't 100% confident in what you asked for. > I am nittypicky: > > let formatArgs = ["android", "ios"].map(os => { ... done. > ::: browser/components/customizableui/content/panelUI.inc.xul > @@ +121,5 @@ > > + closemenu="none"/> > > + <toolbarbutton id="PanelUI-remotetabs-opensidebar" > > + class="subviewbutton" > > + label="&appMenuRemoteTabs.openSidebar.label;" > > + hidden="true"/> > > Err, is interdiff lying at me or did these buttons swap around? If so, why? > :-) I did move them because I thought the new placement was more logical - that that "open sidebar" element isn't ever made visible in this patch, so I just remove the element (and string) completely - we can do that in the patch that lands the sidebar. > The nesting here is really confusing. Yeah, sorry about that - I earlier got myself into a confused state and didn't completely undo my hacks. This has removed all the unnecessary boxes. > - remove all the class attributes; > - if we need the fill-rule, just stick it on path {} instead of a double > class selector; > - remove the extra level of nested <g> which is not doing anything, afaict - > in fact, I bet you can remove both of them with no loss in appearance; We don't seem to need the fill-rule, but obviously do need the "fill" itself. We could also remove both <g> elements, but I ended up keeping one so I could add the fill to that and avoid it on each of the paths - so we now have: <g fill="#cdcdcd"> <path d="M46.352,148.919 ..."/> ... </g> But I'm not sure that's what you had in mind? > Can we replace the png versions everywhere? Can be a followup bug. Bug 1228478 - the colors are intentionally different (by design - the "prefs" pane uses a blueish tinge) - so it seems that fixing that bug might mean reintroducing styles to the svg, but that sounds OK to me - this is landing the simplest possible and we can add complexity as it is actually needed. Sound ok? This patch also reflects the changes I mentioned in comment 72.

Comment on attachment 8693059 [details] [diff] [review] interdiff.txt Review of attachment 8693059 [details] [diff] [review]: ----------------------------------------------------------------- Please let me know if the interdiff misses something, but this looks fine to me. Note that your browser.xul and panelUIOverlay.inc.css changes have bitrotted.

Comment on attachment 8689359 [details] [diff] [review] 0003-Bug-1201331-part-1-use-a-broadcaster-to-manage-the-S.patch Review of attachment 8689359 [details] [diff] [review]: ----------------------------------------------------------------- Good idea.

Comment on attachment 8689359 [details] [diff] [review] 0003-Bug-1201331-part-1-use-a-broadcaster-to-manage-the-S.patch Thanks Drew, and welcome back!

Comment on attachment 8689359 [details] [diff] [review] 0003-Bug-1201331-part-1-use-a-broadcaster-to-manage-the-S.patch Review of attachment 8689359 [details] [diff] [review]: ----------------------------------------------------------------- Good idea. ::: browser/base/content/browser-sets.inc @@ +183,5 @@ > + <!-- Sync broadcasters --> > + <!-- A broadcaster of a number of attributes suitable for "sync now" UI - > + A 'syncstatus' attribute is set while actively syncing, and the label > + attribute which changes from "sync now" to "syncing" etc. --> > + <broadcaster id="sync-status"/> Might be a little confusing or error-prone that this is named sync-status and it has an attribute called syncstatus, dunno. You might call it sync-status-broadcaster or something if you agree. ::: services/sync/locales/en-US/sync.properties @@ +25,5 @@ > sync.eol.learnMore.label = Learn more > sync.eol.learnMore.accesskey = L > + > +syncnow.label = Sync Now > +syncing.label = Syncing Are the values of these strings totally new, or are they replacing some older strings somewhere that maybe should be removed now?

Thanks for the quick review! (In reply to Drew Willcoxon :adw from comment #78) > Might be a little confusing or error-prone that this is named sync-status > and it has an attribute called syncstatus, dunno. You might call it > sync-status-broadcaster or something if you agree. Yeah :( On the flip-side, we have: <broadcaster id="sync-status"/> <broadcaster id="sync-setup-state"/> <broadcaster id="sync-syncnow-state" hidden="true"/> <broadcaster id="sync-reauth-state" hidden="true"/> and changing just one of them to have a trailing -broadcaster seems just as confusing. If it's OK with you I'll leave it as is for now and open a new bug to rename them all. > ::: services/sync/locales/en-US/sync.properties > @@ +25,5 @@ > > sync.eol.learnMore.label = Learn more > > sync.eol.learnMore.accesskey = L > > + > > +syncnow.label = Sync Now > > +syncing.label = Syncing > > Are the values of these strings totally new, or are they replacing some > older strings somewhere that maybe should be removed now? Out l10n story sucks - there is another "Sync Now" string but that's in a DTD as one of the menus references it as an entity.

These are test-only changes for tests that made various assumptions about the default panel layout. I opened bug 1229236 to try and make these tests more resilient to such changes in the future. I'll roll it into the "part 3" patch before landing.

This patch changes the animated sync icon to a static "tab-like" button. Note that the "sync now" buttons in the hamburger menu and inside the new panel remain the old animated versions. This patch touches the "sprite" files we use for icons - the new icon is at the end (ie, each image is now wider) - the old sync icon remains in the sprite. Bryan Bell made the images for me with review of the images done by shorlander. I've also manually eyeballed each image and they all look fine to me (for whatever that's worth ;). I've tested this patch on Windows 7 (with DPI of 1.1 and 1.0), OSX (non-retina) and Linux and all work as expected. A full try run with this patch is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d07f646987ac&selectedJob=14287748, so if anyone wants to grab those builds and verify, just grab them from there. Gijs, thanks for taking on these reviews, but as always, feel free to hand this off to someone more appropriate.

Comment on attachment 8693974 [details] [diff] [review] 0004-Bug-1201331-part-3.5-fixup-tests-which-make-assumpti.patch Review of attachment 8693974 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/test/browser_876944_customize_mode_create_destroy.js @@ +38,5 @@ > ok(elem, "There should be an item"); > ok(wrapper, "There should be a wrapper"); > is(wrapper.firstChild.id, kTestWidget2, "Wrapper should have test widget"); > is(wrapper.parentNode, panel, "Wrapper should be in panel"); > + expectedPlaceholders = 3 + (isInDevEdition() ? 1 : 0); Heh, there will never be 4 placeholders (we won't create an extra row of 3), so this will go orange on aurora. You want: isInDevEdition() ? 1 : 3

Comment on attachment 8693990 [details] [diff] [review] 0006-Bug-1201331-part-5-Use-a-new-non-animated-icon-for-t.patch Review of attachment 8693990 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/menupanel.inc.css @@ +59,5 @@ > } > > #sync-button[cui-areatype="menu-panel"], > toolbarpaletteitem[place="palette"] > #sync-button { > + -moz-image-region: rect(0px, 1024px, 32px, 992px); I'm confused. Why can't we just reuse the existing portion of the spritesheet? Nothing else is using the old image in there, right?

(In reply to :Gijs Kruitbosch from comment #83) > Comment on attachment 8693990 [details] [diff] [review] > 0006-Bug-1201331-part-5-Use-a-new-non-animated-icon-for-t.patch > > Review of attachment 8693990 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/themes/shared/menupanel.inc.css > @@ +59,5 @@ > > } > > > > #sync-button[cui-areatype="menu-panel"], > > toolbarpaletteitem[place="palette"] > #sync-button { > > + -moz-image-region: rect(0px, 1024px, 32px, 992px); > > I'm confused. Why can't we just reuse the existing portion of the > spritesheet? Nothing else is using the old image in there, right?

Oops - I did miss the retina icon for mac. (In reply to :Gijs Kruitbosch from comment #83) > I'm confused. Why can't we just reuse the existing portion of the > spritesheet? Nothing else is using the old image in there, right? TBH I was expecting the new assets would replace that icon, but they were delivered to me with the icon appended. Given it's a large chunk of work for UX to generate new sprites (there should be a bug on that too!) and the lack of problems having a "currently unused" image in the sprite causes, I didn't bother asking them to regenerate them again. If you feel strongly that it *is* a problem, how would you feel about having this done in a followup?

Comment on attachment 8694535 [details] [diff] [review] 0006-Bug-1201331-part-5-Use-a-new-non-animated-icon-for-t.patch Review of attachment 8694535 [details] [diff] [review]: ----------------------------------------------------------------- code-wise this looks fine. Tested on OSX hidpi, works fine. So you get r+. TBH I am surprised that there is no indication of sync on the new icon. If we're going to show this by default in the menupanel, I think just showing a tab graphic is going to be confusing to most users, especially if we're doing it in the same release where we remove panorama... We could just use the circly arrows or even <=> inside the tab to indicate that this is specifically about sync... but all that's just me, and I'm not a visual designer, so...

(In reply to :Gijs Kruitbosch from comment #86) > but all that's just me, and I'm not a visual > designer, so... I agree. IIUC, Madhava asked for this specific change. Madhava, any thoughts here for a followup bug?

https://hg.mozilla.org/integration/fx-team/rev/d0d6d5d4ebfa https://hg.mozilla.org/integration/fx-team/rev/549690eaf36d https://hg.mozilla.org/integration/fx-team/rev/b48a56ce709d https://hg.mozilla.org/integration/fx-team/rev/5a286e996755 https://hg.mozilla.org/integration/fx-team/rev/661a531f5dee

The issues Madhava and others brought up: 1. UX believes that the sync icon is confusable with other loading/refresh icons in the awesome bar, so sticking with the sync icon means it'll never be suitable to be put up there by default. 2. The compound icon I originally made (and Bryan improved) does not work in smaller sizes. 3. Sync icon is already used beside username but has different function (refresh only).

https://hg.mozilla.org/mozilla-central/rev/d0d6d5d4ebfa https://hg.mozilla.org/mozilla-central/rev/549690eaf36d https://hg.mozilla.org/mozilla-central/rev/b48a56ce709d https://hg.mozilla.org/mozilla-central/rev/5a286e996755 https://hg.mozilla.org/mozilla-central/rev/661a531f5dee

Release Note Request (optional, but appreciated) [Why is this notable]: Cool new feature, exposed by default [Suggested wording]: New menu to easily access your synced tabs [Links (documentation, blog post, etc)]:

Nick, if this feature deserves a release note, can you please write one?

Added to the release notes with "Synced Tabs button in button bar" as wording.

Performed Regression and Exploratory Testing on Firefox 45 Beta 10 (20160225145837) across the following platforms: Windows 10 (x64), Ubuntu 14.04 (x32) and Mac OS X 10.11. There were no new issues found during sign off, just a few already known, low-impact bugs. - Test Plan: https://wiki.mozilla.org/QA/Synced_Tabs - Detailed test Results: https://goo.gl/KSCJWV Based on the test results, I am marking this bug as verified since the other issues are tracked separately.