1129171 - Update Remote Tabs home panel split pane styling for landscape tablets

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

This is a follow-up to Bug 1063742.

The code is mostly in place, but I'd like to get closer to the mock in https://bugzilla.mozilla.org/attachment.cgi?id=8427449.

Namely:

* Add top margin whitespace to push page down a little, like the mock;
* Split clients and tabs 40%/60% like the mock;
* Include left and right margin whitespace around clients list; (use the panel margin sizes here);
* Include left margin whitespace in each tab; (use the bookmarks panel item margin sizes here);
* Increase the vertical size of client records;
* Make the selected/unselected state of client records be "gray background"/"white background", both with black text.

Many small commits, I expect.

Comment 1

[Nick Alexander :nalexander [he/him]]

antlam: this is squarely your domain.  I'll attach a screencap for you to see current.  Minor changes to yuan's mock could work.

mcomella: this impacts tablet landscape, which is squarely your domain now.  Any thoughts?

Comment 2

[Nick Alexander :nalexander [he/him]]

I would like this track Fennec 38.

Comment 3

[Nick Alexander :nalexander [he/him]]

Soon to be landed code looks like:

https://people.mozilla.org/~nalexander/screenshots/Synced.Tabs.In.Future.png

(Ignore the last synced time in the future -- clocks are hard.)

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Nice! You're right, just getting those changes in place to get closer to the mock would be a great start. I don't think Yuan or I actually spec'd this out but I can do that for you.

In the mean time, maybe closer to 32% - 68% would be better. More to come in a spec :)

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Nick Alexander :nalexander from comment #1)
> mcomella: this impacts tablet landscape, which is squarely your domain now. 
> Any thoughts?

Nope - with New Tablet landed, this should be straightforward.

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Hey Nick,

Could you post a screenshot of the progress once you've made those changes? would help me visualize this a bit more. Thanks!

Comment 7

[Vivek Balakrishnan[:vivek]]

Attachment: 1129171.patch

Split plane landscape mode UI changes:

* padding added to tabs and clients list
* selected client background handled.

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_tab_syncpanel_landscape.png

Here's a higher fidelity mock of the early stage one we talked about earlier. Focusing on what comes below the Panel header, I made the least amount of changes I could and I think it's pretty extensible. It's basically a third and two thirds design.

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_tab_syncpanel_landscape2.png

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8566914 [details] [diff] [review]
1129171.patch

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

Sorry for the delayed review!  This has issues with the values/values-land dimens:

E GeckoCrashHandler(6820)     >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
E GeckoCrashHandler(6820)     java.lang.UnsupportedOperationException: Can't convert to dimension: type=0x1
E GeckoCrashHandler(6820)     	at android.content.res.TypedArray.getDimensionPixelSize(TypedArray.java:463)
E GeckoCrashHandler(6820)     	at android.view.View.<init>(View.java:3345)
E GeckoCrashHandler(6820)     	at android.view.ViewGroup.<init>(ViewGroup.java:427)
E GeckoCrashHandler(6820)     	at android.widget.LinearLayout.<init>(LinearLayout.java:176)
E GeckoCrashHandler(6820)     	at android.widget.LinearLayout.<init>(LinearLayout.java:172)
E GeckoCrashHandler(6820)     	at java.lang.reflect.Constructor.constructNative(Native Method)
E GeckoCrashHandler(6820)     	at java.lang.reflect.Constructor.newInstance(Constructor.java:417)
E GeckoCrashHandler(6820)     	at android.view.LayoutInflater.createView(LayoutInflater.java:587)
E GeckoCrashHandler(6820)     	at com.android.internal.policy.impl.PhoneLayoutInflater.onCreateView(PhoneLayoutInflater.java:56)
E GeckoCrashHandler(6820)     	at android.view.LayoutInflater.onCreateView(LayoutInflater.java:660)
E GeckoCrashHandler(6820)     	at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:685)
E GeckoCrashHandler(6820)     	at android.view.LayoutInflater.rInflate(LayoutInflater.java:746)
E GeckoCrashHandler(6820)     	at android.view.LayoutInflater.rInflate(LayoutInflater.java:749)
E GeckoCrashHandler(6820)     	at android.view.LayoutInflater.inflate(LayoutInflater.java:489)
E GeckoCrashHandler(6820)     	at android.view.LayoutInflater.inflate(LayoutInflater.java:396)
E GeckoCrashHandler(6820)     	at org.mozilla.gecko.home.RemoteTabsSplitPlaneFragment.onCreateView(RemoteTabsSplitPlaneFragment.java:59)
E GeckoCrashHandler(6820)     	at android.support.v4.app.Fragment.performCreateView(Fragment.java:1786)
E GeckoCrashHandler(6820)     	at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:947)
E GeckoCrashHandler(6820)     	at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1126)
E GeckoCrashHandler(6820)     	at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1108)
E GeckoCrashHandler(6820)     	at android.support.v4.app.FragmentManagerImpl.dispatchActivityCreated(FragmentManager.java:1917)
E GeckoCrashHandler(6820)     	at android.support.v4.app.Fragment.performActivityCreated(Fragment.java:1800)
E GeckoCrashHandler(6820)     	at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:967)
E GeckoCrashHandler(6820)     	at android.support.v4.app.FragmentManagerImpl.attachFragment(FragmentManager.java:1302)
E GeckoCrashHandler(6820)     	at android.support.v4.app.BackStackRecord.run(BackStackRecord.java:729)
E GeckoCrashHandler(6820)     	at android.support.v4.app.FragmentManagerImpl.execPendingActions(FragmentManager.java:1489)
E GeckoCrashHandler(6820)     	at android.support.v4.app.FragmentManagerImpl$1.run(FragmentManager.java:454)
E GeckoCrashHandler(6820)     	at android.os.Handler.handleCallback(Handler.java:615)
E GeckoCrashHandler(6820)     	at android.os.Handler.dispatchMessage(Handler.java:92)
E GeckoCrashHandler(6820)     	at android.os.Looper.loop(Looper.java:137)
E GeckoCrashHandler(6820)     	at android.app.ActivityThread.main(ActivityThread.java:4745)
E GeckoCrashHandler(6820)     	at java.lang.reflect.Method.invokeNative(Native Method)
E GeckoCrashHandler(6820)     	at java.lang.reflect.Method.invoke(Method.java:511)
E GeckoCrashHandler(6820)     	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:786)
E GeckoCrashHandler(6820)     	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
E GeckoCrashHandler(6820)     	at dalvik.system.NativeStart.main(Native Method)

::: mobile/android/base/resources/layout/home_remote_tabs_split_plane_panel.xml
@@ +20,5 @@
>  
>          <LinearLayout android:layout_width="match_parent"
>                        android:layout_height="match_parent"
> +                      android:orientation="horizontal"
> +                      android:paddingTop="@dimen/home_remote_tabs_split_plane_top_margin">

These dimens are only defined in values-land.  I get crashes on my devices when rotating.

Comment 11

[Vivek Balakrishnan[:vivek]]

Attachment: 1129171.patch

A new patch to fix the missing dimensions error during fragment loading after orientation change.

Upon orientation change from landscape to portrait mode, the split plane fragment that is in the back-stack would be made visible during onStart callback. However, this would throw an error as the dimensions defined in value-land file are not available in portrait mode. To fix this, split plane fragment can be safely removed from back-stack during onActivityCreated callback if the current orientation is now in portrait mode.

@Nick: Can you please help me test this with a tablet.

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8570231 [details] [diff] [review]
1129171.patch

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

I don't understand why this would work in all cases. How do you know that having mCurrentFragment set implies that we always have the wrong configuration in the back stack?  Is it not possible to have the same configuration across other configuration changes, like language changes?

::: mobile/android/base/home/RemoteTabsPanel.java
@@ +79,5 @@
>      @Override
>      protected void loadIfVisible() {
>          // Force load the child fragment if fragment is displayed in Tablet with a valid account.
>          if (canLoad() && HardwareUtils.isTablet() && (mCurrentFragment instanceof RemoteTabsBaseFragment)) {
> +            // The Fragment in the back-stack is not for the current configuration.

What if this is the first load?  I.e., not after a configuration change?

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

An idea that was brought to my attention by Robin: How hard would it be to use the same layout styles for these items in landscape mode (i.e. make "devices" list style the same as "links" so they align), but keep what we already use in Portrait mode? I know we had concerns about creating one too many styles but this wouldn't necessarily be doing that and it would align things nicely.

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Anthony Lam (:antlam) from comment #13)
> An idea that was brought to my attention by Robin: How hard would it be to
> use the same layout styles for these items in landscape mode (i.e. make
> "devices" list style the same as "links" so they align), but keep what we
> already use in Portrait mode? I know we had concerns about creating one too
> many styles but this wouldn't necessarily be doing that and it would align
> things nicely.

I do not think it would be hard.  In fact, it may simplify things.  vivek, can you comment?

Comment 15

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Just checking but, we should also plan on using this "split pane" UI in our "Bookmarks" panel as well.

Comment 16

[Vivek Balakrishnan[:vivek]]

Attachment: 1129171.patch

Updated UI styling according to antlam's latest mock

@Nick: This is patch was developed on top of my patch for bug 1129168

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8573533 [details] [diff] [review]
1129171.patch

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

vivek: thanks for sticking with this.  Looks great!

Comment 18

[Nick Alexander :nalexander [he/him]]

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d0e907163a24

Comment 20

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8573533 [details] [diff] [review]
1129171.patch

Approval Request Comment
[Feature/regressing bug #]: none.
[User impact if declined]: ugly layout in Synced Tabs panel.
[Describe test coverage new/current, TreeHerder]: manual testing by nalexander.
[Risks and why]: very low.
[String/UUID change made/needed]: none.

Comment 21

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I think we're missing the vertical divider from the mock in comment 9. Can we add that in?

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8573533 [details] [diff] [review]
1129171.patch

approving for aurora uplift, i see that a separate bug was filed for the vertical divider.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa343bcd66bd