1241141 - Synced tabs panel height changes when it is accessed from History (Panel Menu)

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P2)

Description

[Vasilica Tamas, QA [:vtamas]]

Reproducible on: Firefox 46.0a1 and Firefox 45.0a2 across all platforms

Prerequisites: Use a Firefox account which have a few synced tabs.

STR
1.Launch Firefox with a clean profile.
2.Click menu [≡] → Sign in to Sync.
3.Log in using the Firefox account.
4.Open the Panel Menu → History → Synced tabs.

ER
Synced tabs panel has the same height as the Panel Menu.
See screenshot: http://i.imgur.com/WkVHjMs.jpg


AR
Accessing Synced tabs from History (Panel Menu) opens a smaller panel. 
See screenshot: http://i.imgur.com/c6IpKTB.jpg


Additional notes:
- This issue is reproducible on Firefox 46.0a1 (2016-01-19) and Firefox 45.0a2 (2015-01-20) under Windows 10 64-bit, Mac OS X 10.10.4 and Ubuntu 14.04 32-bit.

Comment 1

[Chris Karlof [:ckarlof]]

Attachment: history panel.jpg

I can verify that the history panel with little data does not display this problem.

Comment 2

[Chris Karlof [:ckarlof]]

I'm assuming that this fix is pretty straightforward, so let's prioritize it.

Comment 3

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

This looks tricky. I *think* the issue is that the code for showing the panel is getting confused in this context - we are entering https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.xml#220 when this._transitioning is already true, meaning there are 2 transitionend observers on the panel and the height adjustment is getting confused because of it.

Comment 4

[Lina Butler (ex-Mozilla)]

Attachment: bug.gif

Comment 5

[Lina Butler (ex-Mozilla)]

Attachment: queued-transitions.gif

Comment 6

[Lina Butler (ex-Mozilla)]

This is an interesting one, because it shows a subview (Synced Tabs) when we already have a subview (History) open. AFAICT, this is the first time we're using this UI pattern.

Here's what I think happens: clicking "Synced Tabs" dismisses the app menu (https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/components/customizableui/CustomizableUI.jsm#1701 hides the panel, and https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/components/customizableui/content/panelUI.xml#319-324 transitions to the main view).

Then, the `oncommand` handler [https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#90] calls https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/base/content/browser-syncui.js#318-332, which calls `PanelUI.show()`.

At that point, `PanelUI.panel.state == "closed"`, but the panel is still transitioning. When the `show()` promise resolves, the transition is still in progress, causing `PanelUI.showSubView()` to calculate and set the wrong height here: https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/components/customizableui/content/panelUI.xml#208-209

Attachment 8717553 [details] shows the bug.

It's possible to queue up calls to `showMainView` and `showSubView` in `panelUI.xml`, and run them when the `transitionend` event fires. We can also set `closemenu="single"` on the "Synced Tabs" menu item. This fixes the height calculation, but the transition is awkward: clicking "Synced Tabs" first slides the app menu back in, then shows the panel. See attachment 8717554 [details].

I think the first step is to figure out how we want to show the nested panel. We should also make sure that fixing this case doesn't break others. I can think of four:

1. Menu bar (shown by default on OS X, or if "Alt" is pressed on Windows) > History > Synced Tabs.
2. Nav bar > Synced Tabs. Not by default, but possible if you add the "Synced Tabs" button to the nav bar in Customize view.
3. Nav bar > History > Synced Tabs.
4. App (hamburger) menu > Synced Tabs.

...And several button combinations, depending on how the nav bar is customized.

Comment 7

[Lina Butler (ex-Mozilla)]

Gijs, do you have any thoughts on how we should handle the "subview of a subview" case (when we bring up Synced Tabs from the History panel)? I didn't see any other examples of this in the panel UI. The "queued transitions" fix seems better than the current behavior, but I think it's still a little jarring.

Comment 8

[:Gijs (he/him)]

(In reply to Kit Cambridge [:kitcambridge] from comment #7)
> Gijs, do you have any thoughts on how we should handle the "subview of a
> subview" case (when we bring up Synced Tabs from the History panel)? I
> didn't see any other examples of this in the panel UI. The "queued
> transitions" fix seems better than the current behavior, but I think it's
> still a little jarring.

Specifically for this case, we could write code that replaces the loaded subview without transitioning back to the main view or closing the popup. I don't know how hard that would be, but it seems like the most correct solution.

Comment 9

[Edwin Wong [:edwong]]

triage: moving this to P2 - it may be difficult to encounter this bug for most users.

Comment 10

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

As Kit mentioned above, there are 2 main ways this could look to the user:

When the user clicks "Show Synced Tabs":
1) the panel immediately changes to a new subview without any transitions.
2) the panel transitions back to the main view then to the new sub-view. This is roughly what kit uploaded as attachment 8717554 [details].

Option (2) looks a little strange, but the more I think about it, the more I think it is actually appropriate. It gives the user some context on what is happening and makes it clear that the "History" panel is gone and the "Synced Tabs" panel is being shown. If we did (1) the user may be confused and think they are still in the "History" panel.

OTOH though, it's hard to argue that it isn't at least a *little* strange.

I think the choice made here will determine the implementation strategy, so Ryan, what do you think? From a UX perspective would you prefer (1) or (2)?

Comment 11

[Edwin Wong [:edwong]]

triage: this is not a blocker

Comment 12

[Ryan Feeley [:rfeeley]]

(In reply to Mark Hammond [:markh] from comment #10)
> As Kit mentioned above, there are 2 main ways this could look to the user:
> 
> When the user clicks "Show Synced Tabs":
> 1) the panel immediately changes to a new subview without any transitions.
> 2) the panel transitions back to the main view then to the new sub-view.
> This is roughly what kit uploaded as attachment 8717554 [details].

I think 2 makes more sense. Not a blocker for me. If it was less work hide the Synced Tabs option in the History toolbar menu, I would be fine with that. I am guessing very very few users will experience this.

Comment 13

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

Attachment: 0001-experiments.patch

This is fairly subtle due to various event handlers, but what's happening here is:

* History panel is open - user clicks on "Synced Tabs".
* oncommand on the "Synced Tabs" element ends up in openSyncedTabsPanel() - panel state is currently open.
* openSyncedTabsPanel() calls PanelUI.show(), which resolves its ensureReady() promise - that is already resolved, but still spins an event loop before getting into the body of .show()
* While waiting for the .ensureReady() promise, the click handler in CustomizableUI fires, which calls maybeAutoHidePanel(), which hides the panel
* PanelUI.show()'s promise is resolved - body is entered and finds panel state as "closed" - re-opens.
* PanelUI.show() resolves its promise on popupshowing - but the panel is still transitioning open
* openSyncedTabsPanel() calls PanelUI.showSubView()
* we try and transition the subview, but the main view is still transitioning, things get upset.

A relatively simple fix is attached - we just wait for the transition to finish before PanelUI.show()'s promise is resolved. It still leaves us with the panel fully-closing then fully-opening which kinda sucks - but fixing that gets complicated, as we'd end up in the situation where History->SyncedTabs sometimes wants to auto-close the panel, but sometimes does not.

Anyway, time to get feedback - Gijs, what do you think?

Comment 14

[:Gijs (he/him)]

Comment on attachment 8723967 [details] [diff] [review]
0001-experiments.patch

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

TBH I would prefer:

(In reply to Ryan Feeley [:rfeeley] from comment #12)
> (In reply to Mark Hammond [:markh] from comment #10)
> > As Kit mentioned above, there are 2 main ways this could look to the user:
> > 
> > When the user clicks "Show Synced Tabs":
> > 1) the panel immediately changes to a new subview without any transitions.
> > 2) the panel transitions back to the main view then to the new sub-view.
> > This is roughly what kit uploaded as attachment 8717554 [details].
> 
> I think 2 makes more sense. Not a blocker for me. If it was less work hide
> the Synced Tabs option in the History toolbar menu, I would be fine with
> that. I am guessing very very few users will experience this.


Doing this, ie removing the item. Would that be OK? This change seems fairly scary to me, and removing the item seems like much the simplest solution to the issue.

Note that the transitionend handler needs to be cleverer anyway, if you really want to push this, because IIRC there can be multiple transitions (in some other cases in the panel multiview we wait for a specific one, I think...).

Comment 15

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

Attachment: 0001-Bug-1241141-remove-Synced-Tabs-item-from-history-pan.patch

Yeah, removing the item WFM!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1a6147470794

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a6147470794

Comment 18

[Camelia Badau [:cbadau], Desktop Test Engineering]

I've tested on Windows 7 64bit, Mac OSX 10.9.5 and Ubuntu 13.10 32bit using latest DevEdition 47.0a2 (buildID: 20160322004033) and the Synced Tabs item is removed from History (Panel Menu). 

Marking VERIFIED FIXED since this is the expected behaviour, based on comments above.