Update Remote Tabs home panel split pane styling for landscape tablets

RESOLVED FIXED in Firefox 38



Firefox for Android
3 years ago
3 years ago


(Reporter: nalexander, Assigned: vivek, Mentored)


Firefox 39
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed, fennec38+)



(2 attachments, 3 obsolete attachments)

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.


* 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.
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?
Flags: needinfo?(alam)
I would like this track Fennec 38.
tracking-fennec: --- → ?
Flags: needinfo?(michael.l.comella)
Soon to be landed code looks like:


(Ignore the last synced time in the future -- clocks are hard.)
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 :)
(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.
Flags: needinfo?(michael.l.comella)
tracking-fennec: ? → 38+
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!
Flags: needinfo?(nalexander)

Comment 7

3 years ago
Created attachment 8566914 [details] [diff] [review]

Split plane landscape mode UI changes:

* padding added to tabs and clients list
* selected client background handled.
Attachment #8566914 - Flags: review?(nalexander)
Created attachment 8570018 [details]

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.
Flags: needinfo?(alam)
Assignee: nobody → vivekb.balakrishnan
Created attachment 8570021 [details]
Attachment #8570018 - Attachment is obsolete: true
Comment on attachment 8566914 [details] [diff] [review]

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.
Attachment #8566914 - Flags: review?(nalexander)

Comment 11

3 years ago
Created attachment 8570231 [details] [diff] [review]

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.
Attachment #8566914 - Attachment is obsolete: true
Attachment #8570231 - Flags: review?(nalexander)
Comment on attachment 8570231 [details] [diff] [review]

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?
Attachment #8570231 - Flags: review?(nalexander)
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.
(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?
Flags: needinfo?(nalexander) → needinfo?(vivekb.balakrishnan)
Just checking but, we should also plan on using this "split pane" UI in our "Bookmarks" panel as well.

Comment 16

3 years ago
Created attachment 8573533 [details] [diff] [review]

Updated UI styling according to antlam's latest mock

@Nick: This is patch was developed on top of my patch for bug 1129168
Attachment #8570231 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8573533 - Flags: review?(nalexander)
Comment on attachment 8573533 [details] [diff] [review]

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

vivek: thanks for sticking with this.  Looks great!
Attachment #8573533 - Flags: review?(nalexander) → review+
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Comment on attachment 8573533 [details] [diff] [review]

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.
Attachment #8573533 - Flags: approval-mozilla-aurora?
I think we're missing the vertical divider from the mock in comment 9. Can we add that in?
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)


3 years ago
Blocks: 1141665


3 years ago
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
Comment on attachment 8573533 [details] [diff] [review]

approving for aurora uplift, i see that a separate bug was filed for the vertical divider.
Attachment #8573533 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox38: --- → affected
status-firefox38: affected → fixed
You need to log in before you can comment on or make changes to this bug.