1239845 - Remove Fetching Synced Tabs row from Synced Tabs

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P3)

Description

[Ryan Feeley [:rfeeley]]

Attachment: fetching.png

UX requests that the row and copy for "Fetching Synced Tabs" be removed both in the Syncing… and Sync Now states.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0004-Bug-1239845-part-1-remove-Fetching-Synced-Tabs-row-f.patch

This patch removes the "Fetching Synced Tabs" deck pane. It also removes the menuseparator which normally appears above this item as otherwise it is "orphaned" (ie, it's the last element displayed). This means the code that populates the client list needs to add a new separator before the first client, and the other deck elements also get a separator that appears above the sync illustration - which sadly involved adding a new vbox, otherwise the new separator would be vertically centered (ie, there may be unwanted space above it)

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0005-Bug-1239845-part-2-remove-hasSyncedThisSession-prope.patch

This patch removes the hasSyncedThisSession property from SyncedTabs.jsm and the tabs engine. No callers remain so removing this just keeps the complexity level down.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8714207 [details] [diff] [review]
0004-Bug-1239845-part-1-remove-Fetching-Synced-Tabs-row-f.patch

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

I think there's a simpler solution than to add another box, see review comments, which would also help with the fact that now the IDs live on non-kids of the deck, which breaks some of the rest of this patch, I think.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +130,5 @@
>              </vbox>
>              <!-- Sync is ready to Sync but the "tabs" engine isn't enabled-->
> +            <vbox>
> +              <menuseparator/>
> +              <hbox id="PanelUI-remotetabs-tabsdisabledpane" pack="center" flex="1">

Can't it be in here with align="start" on this hbox ? Ditto for the other one.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +743,5 @@
>  
>  /* Collapse the non-active vboxes in the remotetabs deck to use only the
>     height the active box needs */
>  #PanelUI-remotetabs-deck:not([selectedIndex="1"]) > #PanelUI-remotetabs-tabsdisabledpane,
> +#PanelUI-remotetabs-deck:not([selectedIndex="2"]) > #PanelUI-remotetabs-nodevicespane {

These selectors are broken now, because these items are no longer direct children of the parent. I think there might be a few other cases where we rely on that being the case right now.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #3)
> Comment on attachment 8714207 [details] [diff] [review]
> 0004-Bug-1239845-part-1-remove-Fetching-Synced-Tabs-row-f.patch
> 
> Review of attachment 8714207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think there's a simpler solution than to add another box, see review
> comments, which would also help with the fact that now the IDs live on
> non-kids of the deck, which breaks some of the rest of this patch, I think.
> 
> ::: browser/components/customizableui/content/panelUI.inc.xul
> @@ +130,5 @@
> >              </vbox>
> >              <!-- Sync is ready to Sync but the "tabs" engine isn't enabled-->
> > +            <vbox>
> > +              <menuseparator/>
> > +              <hbox id="PanelUI-remotetabs-tabsdisabledpane" pack="center" flex="1">
> 
> Can't it be in here with align="start" on this hbox ? Ditto for the other
> one.

I'm not sure what you mean here. Putting it inside the hbox means the separator and the sibling vbox (with the illustration and button) would be placed horizontally (ie, next to each other) - we want the separator and the following box to be vertical.

Can you please clarify?

> These selectors are broken now, because these items are no longer direct
> children of the parent. I think there might be a few other cases where we
> rely on that being the case right now.

Doh - yes, thanks!

Comment 5

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > Comment on attachment 8714207 [details] [diff] [review]
> > 0004-Bug-1239845-part-1-remove-Fetching-Synced-Tabs-row-f.patch
> > 
> > Review of attachment 8714207 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think there's a simpler solution than to add another box, see review
> > comments, which would also help with the fact that now the IDs live on
> > non-kids of the deck, which breaks some of the rest of this patch, I think.
> > 
> > ::: browser/components/customizableui/content/panelUI.inc.xul
> > @@ +130,5 @@
> > >              </vbox>
> > >              <!-- Sync is ready to Sync but the "tabs" engine isn't enabled-->
> > > +            <vbox>
> > > +              <menuseparator/>
> > > +              <hbox id="PanelUI-remotetabs-tabsdisabledpane" pack="center" flex="1">
> > 
> > Can't it be in here with align="start" on this hbox ? Ditto for the other
> > one.
> 
> I'm not sure what you mean here. Putting it inside the hbox means the
> separator and the sibling vbox (with the illustration and button) would be
> placed horizontally (ie, next to each other) - we want the separator and the
> following box to be vertical.

Oh, um, that's dumb of me. I totally missed that. So many boxes. :-\

I guess putting it in the nested vbox inside the hbox (with class="PanelUI-remotetabs-instruction-box") makes it too narrow?

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #5)
> I guess putting it in the nested vbox inside the hbox (with
> class="PanelUI-remotetabs-instruction-box") makes it too narrow?

That vbox is vertically centered due to the pack="center" on the containing <hbox id="PanelUI-remotetabs-tabsdisabledpane" pack="center" flex="1"> - so the separator ends up closer to the illustration than to the items above it (ie, there's a gap between the "Sync now" element and the separator) which looks very strange.

Comment 7

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > I guess putting it in the nested vbox inside the hbox (with
> > class="PanelUI-remotetabs-instruction-box") makes it too narrow?
> 
> That vbox is vertically centered due to the pack="center" on the containing
> <hbox id="PanelUI-remotetabs-tabsdisabledpane" pack="center" flex="1"> - so
> the separator ends up closer to the illustration than to the items above it
> (ie, there's a gap between the "Sync now" element and the separator) which
> looks very strange.

Hmm. Would it just be easier to keep the separator where it is now and hide it with styling for the views in the deck (just 1 of 3, right?) where we don't want it?

Comment 8

[Mark Hammond [:markh] [:mhammond]]

I've been rethinking this. Apart from the comments above, that approach has real problems managing the state of the panel and sidebar - my approach tried to remove the "loading" state completely. I'm now of the opinion that we need to keep that state, but just not show the words. IOW, I think this patch should be trivial - just the removal of the string (so that state shows a completely blank panel)

Unassigning as I hope our new hire will be able to take this on as a good early patch.

Comment 9

[Thom Chiovoloni [:tcsc] (ex-moco)]

I believe

Comment 10

[Thom Chiovoloni [:tcsc] (ex-moco)]

Sorry, didn't realize the "save changes" icon above also submitted this comment.

Anyway, I believe I've implemented this fix, however I'm unsure about how to go about testing it. This is a panel that shows up for a very brief amount of time, right?

Comment 11

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: MozReview Request: Bug 1239845 - Replace fetching synced tabs text with blank panel r?markh

Review commit: https://reviewboard.mozilla.org/r/43577/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43577/

Comment 12

[Thom Chiovoloni [:tcsc] (ex-moco)]

Sorry, it actually showed for longer than I had initially thought. Testing it ended up not being a problem after I knew what I was looking for.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8736865 [details]
MozReview Request: Bug 1239845 - Replace fetching synced tabs text with blank panel r?markh

https://reviewboard.mozilla.org/r/43577/#review40201

Looks good! We should also do the same for the synced tabs sidebar (sidebar.xhtml) - ie, add the same comment and remove the string itself.

::: browser/components/customizableui/content/panelUI.inc.xul:143
(Diff revision 1)
>                  </hbox>
>                </vbox>
>              </hbox>
>              <!-- Sync is ready to Sync but we are still fetching the tabs to show -->
>              <vbox id="PanelUI-remotetabs-fetching">
> -              <label>&appMenuRemoteTabs.fetching.label;</label>
> +              <!-- Show intentionally blank panel, see bug 1239845 -->

dxr.mozilla.org tells me this is the only usage of that string, so we should kill the string too.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Attachment: remote-tabs-state-scratchpad.js

FYI, this hacky little script is something I use to test the Synced Tabs menu/panel - you can set a state and see what the panel looks like in that state - it's useful for these "short term" or obscure states that are difficult to manually test. It doesn't help test the sidebar, but I think we can live with just manual testing there.

Comment 15

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8736865 [details]
MozReview Request: Bug 1239845 - Replace fetching synced tabs text with blank panel r?markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43577/diff/1-2/

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8736865 [details]
MozReview Request: Bug 1239845 - Replace fetching synced tabs text with blank panel r?markh

https://reviewboard.mozilla.org/r/43577/#review40783

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44acd8ef646c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44acd8ef646c

Comment 19

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OS X 10.9.5 using latest Nightly 48.0a1 (buildID: 20160413030239) - "Fetching Synced Tabs" row is removed from Synced Tabs.