1061508 - Consider fading edge in toolbar's title instead of ellipsis

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Attachment: Screenshot_2014-09-02-10-54-52.png

Anthony what do you think? Wes, you seemed to feel strongly against it on IRC. Thoughts?

Comment 1

[Wesley Johnston (:wesj)]

I'm not a big fan of fading text. I personally find it difficult to read text that fades out as I'm reading. ellipsis gives me a more definite "Sorry, we're out of room" message. I think I would like fading better if we supported scrolling the title (bug 863845). i.e. the fading makes me feel like I should be able to get the rest. The ellipsis is a more definitive "This is all we're gonna show".

That said, UX decided in the past that having one extra (faded) letter was better for usability. I've always been curious if there have been any usability studies done on the subject (based on conversations I've had with Google UX, I get the feeling the answer is no).

Comment 2

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

Not a strong preference to either one right now but when I think about either truncating or fading, I prefer fading. 

From a UX stand point, I guess it gives an extra character rather than using that space for '...' . I talked to Yuan and she seems indifferent as well, I'd be open to giving this a try.

Comment 3

[Lucas Rocha (:lucasr)]

FWIW, latest Chrome for Android is using a fading effect in the location entry.

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Change FadedTextView to be a ThemedTextView (r=bnicholson)

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: BUg 1061508 - Use FadedTextView in the toolbar (r=bnicholson)

Comment 6

[Lucas Rocha (:lucasr)]

It seems we have both reservations and excitement about this change. Let's experiment this in Nightly to see how we feel about it and hopefully get some wider feedback.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8484479 [details] [diff] [review]
Change FadedTextView to be a ThemedTextView (r=bnicholson)

Because we needs the private mode bits in toolbar.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8484480 [details] [diff] [review]
BUg 1061508 - Use FadedTextView in the toolbar (r=bnicholson)

Actually use it in the toolbar layout.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8484479 [details] [diff] [review]
Change FadedTextView to be a ThemedTextView (r=bnicholson)

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

::: mobile/android/base/widget/FadedTextView.java
@@ +13,5 @@
>  import android.graphics.drawable.Drawable;
>  import android.text.Layout;
>  import android.util.AttributeSet;
> +
> +import org.mozilla.gecko.widget.ThemedTextView;

Nit: Put this import after org.mozilla.gecko.R and remove empty space between them.

@@ -36,5 @@
>      public FadedTextView(Context context, AttributeSet attrs) {
> -        this(context, attrs, android.R.attr.textViewStyle);
> -    }
> -
> -    public FadedTextView(Context context, AttributeSet attrs, int defStyle) {

Why remove this constructor?

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 8484480 [details] [diff] [review]
BUg 1061508 - Use FadedTextView in the toolbar (r=bnicholson)

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

UX-wise, I agree with Wes that a faded view feels like it should be scrollable. But as you pointed out, Chrome uses this same effect and isn't scrollable, so maybe it doesn't mean what I expected.

::: mobile/android/base/resources/layout/toolbar_display_layout.xml
@@ +23,5 @@
>                   android:contentDescription="@string/site_security"
>                   android:visibility="gone"/>
>  
> +    <org.mozilla.gecko.widget.FadedTextView android:id="@+id/url_bar_title"
> +                                            style="@style/UrlBar.Title"

Hm, I'm confused how this would even work after you dropped the style constructor in the first patch. What am I missing?

Comment 11

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

Attachment: prev_tabletui_overflow_mock2.png

tried fading some of the labels/titles in the new tablet's UI (WIP)

Showing one more letter (faded) seems better than not (and showing a '...' in it's place)

Comment 12

[Lucas Rocha (:lucasr)]

Fade all things! ;-) Could you please file a separate bug?

Comment 13

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

^ done! Bug 1070029

Comment 14

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #9)
> Comment on attachment 8484479 [details] [diff] [review]
> Change FadedTextView to be a ThemedTextView (r=bnicholson)
> 
> Review of attachment 8484479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/widget/FadedTextView.java
> @@ +13,5 @@
> >  import android.graphics.drawable.Drawable;
> >  import android.text.Layout;
> >  import android.util.AttributeSet;
> > +
> > +import org.mozilla.gecko.widget.ThemedTextView;
> 
> Nit: Put this import after org.mozilla.gecko.R and remove empty space
> between them.

Done.

> @@ -36,5 @@
> >      public FadedTextView(Context context, AttributeSet attrs) {
> > -        this(context, attrs, android.R.attr.textViewStyle);
> > -    }
> > -
> > -    public FadedTextView(Context context, AttributeSet attrs, int defStyle) {
> 
> Why remove this constructor?

(In reply to Brian Nicholson (:bnicholson) from comment #10)
> Comment on attachment 8484480 [details] [diff] [review]
> BUg 1061508 - Use FadedTextView in the toolbar (r=bnicholson)
> 
> Review of attachment 8484480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> UX-wise, I agree with Wes that a faded view feels like it should be
> scrollable. But as you pointed out, Chrome uses this same effect and isn't
> scrollable, so maybe it doesn't mean what I expected.
> 
> ::: mobile/android/base/resources/layout/toolbar_display_layout.xml
> @@ +23,5 @@
> >                   android:contentDescription="@string/site_security"
> >                   android:visibility="gone"/>
> >  
> > +    <org.mozilla.gecko.widget.FadedTextView android:id="@+id/url_bar_title"
> > +                                            style="@style/UrlBar.Title"
> 
> Hm, I'm confused how this would even work after you dropped the style
> constructor in the first patch. What am I missing?

The 'style' XML attribute is handled by Android's AssetManager. The style constructor is meant to be used to set a default style for the View.

Comment 15

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #9)
> Comment on attachment 8484479 [details] [diff] [review]
> Change FadedTextView to be a ThemedTextView (r=bnicholson)
> 
> Review of attachment 8484479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/widget/FadedTextView.java
> @@ +13,5 @@
> >  import android.graphics.drawable.Drawable;
> >  import android.text.Layout;
> >  import android.util.AttributeSet;
> > +
> > +import org.mozilla.gecko.widget.ThemedTextView;
> 
> Nit: Put this import after org.mozilla.gecko.R and remove empty space
> between them.

Done.

> @@ -36,5 @@
> >      public FadedTextView(Context context, AttributeSet attrs) {
> > -        this(context, attrs, android.R.attr.textViewStyle);
> > -    }
> > -
> > -    public FadedTextView(Context context, AttributeSet attrs, int defStyle) {
> 
> Why remove this constructor?

ThemedView (from which ThemedTextView is generated) doesn't have it. I could add it to ThemedView.frag but we don't really need it here i.e. android.R.attr.textViewStyle is redundant in this case.

Comment 16

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/0f2bceed4532
https://hg.mozilla.org/integration/fx-team/rev/556258d3b939

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f2bceed4532
https://hg.mozilla.org/mozilla-central/rev/556258d3b939

Comment 18

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in
Build: Firefox for Android 35.0a1 (2014-10-05)
Devices:
Nexus 4 (Android 4.4.4)
Asus Transformer Pad TF300T (Android 4.2.1)