1366680 - (photon) Tab tray UI refresh and consistency

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Hi jwu
I saw bug 1372486 is landed. I guess I don't need https://github.com/idiotmax/gecko/tree/photon_zoo?
I got some build error when building the github feature branch.

0:26.28 Error running mach:
0:26.28
0:26.28     ['--log-no-times', 'artifact', 'install']
0:26.28
0:26.28 The error occurred in code that was called by the mach command. This is either
0:26.28 a bug in the called code itself or in the way that mach is calling it.
0:26.28
0:26.29 You should consider filing a bug for this issue.
0:26.29
0:26.29 If filing a bug, please include the full output of mach, including this error
0:26.29 message.
0:26.30
0:26.30 The details of the failure are as follows:
0:26.30
0:26.30 IndexError: list index out of range
0:26.30
0:26.30   File "/Users/nechen/Desktop/mozilla-central/python/mozbuild/mozbuild/mach_commands.py", line 1598, in artifact_install
0:26.30     return artifacts.install_from(source, self.distdir)
0:26.30   File "/Users/nechen/Desktop/mozilla-central/python/mozbuild/mozbuild/artifacts.py", line 1149, in install_from
0:26.30     return self.install_from_recent(distdir)
0:26.30   File "/Users/nechen/Desktop/mozilla-central/python/mozbuild/mozbuild/artifacts.py", line 1110, in install_from_recent
0:26.30     return self._install_from_hg_pushheads(hg_pushheads, distdir)
0:26.30   File "/Users/nechen/Desktop/mozilla-central/python/mozbuild/mozbuild/artifacts.py", line 1089, in _install_from_hg_pushheads
0:26.30     for trees, hg_hash in hg_pushheads:
0:26.30   File "/Users/nechen/Desktop/mozilla-central/python/mozbuild/mozbuild/artifacts.py", line 990, in _find_pushheads
0:26.30     candidate_pushheads = self._pushheads_from_rev(last_revs[0].rstrip(),
0:26.31 make[4]: *** [recurse_artifact] Error 1
0:26.31 make[3]: *** [artifact] Error 2
0:26.31 make[2]: *** [default] Error 2
0:26.31 make[1]: *** [realbuild] Error 2
0:26.31 make: *** [build] Error 2
0:26.32 0 compiler warnings present.


Can you also let me know where is the photon colors you previous mentioned?
e.g. #272727 or #F9F9FA in the attachment.
Or should I create my own in colors.xml ?

Comment 2

[Jing-wei Wu [:jwu]]

It looks like an artifact build error, maybe clear files in `$HOME/.mozbuild/package-frontend` and try again?

Also the colors for Photon aren't landed yet, you can use commits in https://github.com/idiotmax/gecko/tree/photon_zoo for temporary.

Comment 3

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1366680 - Refresh Tab Tray UI.

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

Comment 4

[Nevin Chen(Not active on Bugzilla)]

Hi Carol
If the update design is done, please let me know :) Thanks!

Comment 5

[Carol Huang [:Carol]]

Hi Nevin,
I just updated the spec to the Gdrive! 
v 0.4 Revised TAB TRAY UI -p.11 , p.12

please take a look and let me know if you have any other question :)

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Thanks~!

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

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

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

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

Comment 9

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156902/diff/3-4/

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156902/diff/4-5/

Comment 11

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156902/diff/5-6/

Comment 12

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156902/diff/6-7/

Comment 13

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156902/diff/7-8/

Comment 14

[Julian Chu [:walkingice]]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

https://reviewboard.mozilla.org/r/156902/#review166634

::: mobile/android/app/src/australis/res/values/colors.xml:111
(Diff revision 8)
>  
> +  <!-- Tab Tray -->
> +  <color name="tab_item_normal_bg">#F9F9FA</color>
> +  <color name="tab_item_private_bg">#38383D</color>
> +  <color name="tab_item_normal_highlight_bg">#0a84ff</color>
> +  <color name="tab_item_normal_highlight_bg_80">#CC0a84ff</color>

I don't understand what does '80' mean

::: mobile/android/app/src/australis/res/values/colors.xml:113
(Diff revision 8)
> +  <color name="tab_item_normal_bg">#F9F9FA</color>
> +  <color name="tab_item_private_bg">#38383D</color>
> +  <color name="tab_item_normal_highlight_bg">#0a84ff</color>
> +  <color name="tab_item_normal_highlight_bg_80">#CC0a84ff</color>
> +  <color name="tab_item_private_highlight_bg">#AC39FF</color>
> +  <color name="tab_item_private_highlight_bg_80">#CCAC39FF</color>

same here

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java:54
(Diff revision 8)
>      }
>  
> +    private void setPadding() {
> +        final float scale = getResources().getDisplayMetrics().density;
> +        final float sizeInDp = getResources().getDimension(R.dimen.tab_panel_list_item_top_padding);
> +        int dpAsPixels = (int) (sizeInDp * scale + 0.5f);

nit: final

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java:55
(Diff revision 8)
>  
> +    private void setPadding() {
> +        final float scale = getResources().getDisplayMetrics().density;
> +        final float sizeInDp = getResources().getDimension(R.dimen.tab_panel_list_item_top_padding);
> +        int dpAsPixels = (int) (sizeInDp * scale + 0.5f);
> +        setPadding(0, dpAsPixels, 0, dpAsPixels);

what if we explicit say `super.setPadding` ?

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsPanel.java:121
(Diff revision 8)
>      private IconTabWidget mTabWidget;
>      private View mMenuButton;
>      private ImageButton mAddTab;
>  
> +    // Tab Tray
> +    @Nullable private ThemedImageButton normalTabsPanel;

this naming is inconsistent with other members in this class

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsPanel.java:122
(Diff revision 8)
>      private View mMenuButton;
>      private ImageButton mAddTab;
>  
> +    // Tab Tray
> +    @Nullable private ThemedImageButton normalTabsPanel;
> +    @Nullable private ThemedImageButton privateTabsPanel;

same here

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsPanel.java:172
(Diff revision 8)
>          });
>  
>          mTabWidget = (IconTabWidget) findViewById(R.id.tab_widget);
>  
> -        mTabWidget.addTab(R.drawable.tabs_normal, R.string.tabs_normal);
> -        final ThemedImageButton privateTabsPanel =
> +        final View tabNormal = mTabWidget.addTab(R.drawable.tabs_normal, R.string.tabs_normal);
> +        normalTabsPanel = tabNormal instanceof ThemedImageButton ? ((ThemedImageButton) tabNormal) : null;

the original implementation already assumes it should be an ThemedImageButton. If it is not, we can easily catch it in development stage. So I am not sure Type and Null checking are necessary or not. But it won't hurt anyone so I am still good with current implementation.

Comment 15

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156902/diff/8-9/

Comment 16

[Jing-wei Wu [:jwu]]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

https://reviewboard.mozilla.org/r/156902/#review167212

::: mobile/android/app/src/australis/res/values/colors.xml:114
(Diff revision 9)
> +  <color name="tab_item_private_bg">#38383D</color>
> +  <color name="tab_item_normal_highlight_bg">#0a84ff</color>
> +  <color name="tab_item_normal_highlight_bg_80">#CC0a84ff</color>
> +  <color name="tab_item_private_highlight_bg">#AC39FF</color>
> +  <color name="tab_item_private_highlight_bg_80">#CCAC39FF</color>
> +  <color name="tab_item_thumbnail_default_bg">#E4E4E4E4</color>

Should this be #E4E4E4(three "E4"s) instead of #E4E4E4E4(four "E4"s)?

::: mobile/android/app/src/photon/res/values/colors.xml:203
(Diff revision 9)
> +    <color name="tab_item_private_bg">#38383D</color>
> +    <color name="tab_item_normal_highlight_bg">#0a84ff</color>
> +    <color name="tab_item_normal_highlight_bg_80">#CC0a84ff</color>
> +    <color name="tab_item_private_highlight_bg">#AC39FF</color>
> +    <color name="tab_item_private_highlight_bg_80">#CCAC39FF</color>
> +    <color name="tab_item_thumbnail_default_bg">#E4E4E4E4</color>

Same question as above.

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java:52
(Diff revision 9)
> +        final float scale = getResources().getDisplayMetrics().density;
> +        final float sizeInDp = getResources().getDimension(R.dimen.tab_panel_list_item_top_padding);
> +        final int dpAsPixels = (int) (sizeInDp * scale + 0.5f);

Why not use `getDimensionPixelSize()`?, The result is rounded, which means +0.5f might not be needed.

https://developer.android.com/reference/android/content/res/Resources.html#getDimensionPixelSize(int)

Comment 17

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156902/diff/9-10/

Comment 18

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156902/diff/10-11/

Comment 19

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

https://reviewboard.mozilla.org/r/156902/#review166634

> I don't understand what does '80' mean

it means 80% transparency.

> same here

it means 80% transparency.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4861ce4f82f6
Refresh Tab Tray UI. r=jwu,walkingice

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/4861ce4f82f6

Comment 22

[Oana Horvath [:ohorvath]]

Verified as fixed on Nightly 56.0a1, (2017-08-11).
Devices: 
Huawei Honor 8 (Android 6.0)
Motorola Nexus 6 (Android 7.0)
Prestigio Grace X5 (Android 4.4.2)
Huawei Honor 8 (Android 6.0)