Closed Bug 1090364 Opened 10 years ago Closed 9 years ago

Add fading edge to tab strip

Categories

(Firefox for Android Graveyard :: General, defect, P1)

All
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(6 files)

This is related to bug 1024816 but focused on the fading edge aspect.
Comment on attachment 8512784 [details] [diff] [review]
Add fading edge support to TwoWayView (r=mcomella)

Upstream change which will be available in the next release (0.1.4).
Attachment #8512784 - Flags: review?(michael.l.comella)
Attachment #8512785 - Flags: review?(michael.l.comella)
This is just the standard fading edge from Android btw.
Comment on attachment 8512784 [details] [diff] [review]
Add fading edge support to TwoWayView (r=mcomella)

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

As usual, r+ with the caveat that I don't know the upstream TwoWayView code, but I'm just confirming this patch only updates to the upstream TwoWayView.
Attachment #8512784 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8512785 [details] [diff] [review]
Enable fading edges in tab strip (r=mcomella)

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

::: mobile/android/base/newtablet/res/layout-large-v11/tab_strip.xml
@@ +9,5 @@
>          android:id="@+id/tab_strip"
>          android:layout_width="match_parent"
>          android:layout_height="match_parent"
>          android:layout_weight="1"
> +        android:layout_marginLeft="2dp"

Why marginLeft rather than paddingLeft?

@@ +10,5 @@
>          android:layout_width="match_parent"
>          android:layout_height="match_parent"
>          android:layout_weight="1"
> +        android:layout_marginLeft="2dp"
> +        android:requiresFadingEdge="vertical|horizontal"

Why is horizontal useful here?
Attachment #8512785 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #6)
> Comment on attachment 8512785 [details] [diff] [review]
> Enable fading edges in tab strip (r=mcomella)
> 
> Review of attachment 8512785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/newtablet/res/layout-large-v11/tab_strip.xml
> @@ +9,5 @@
> >          android:id="@+id/tab_strip"
> >          android:layout_width="match_parent"
> >          android:layout_height="match_parent"
> >          android:layout_weight="1"
> > +        android:layout_marginLeft="2dp"
> 
> Why marginLeft rather than paddingLeft?

Otherwise the left fading edge starts 2dp *within* the view.
 
> @@ +10,5 @@
> >          android:layout_width="match_parent"
> >          android:layout_height="match_parent"
> >          android:layout_weight="1"
> > +        android:layout_marginLeft="2dp"
> > +        android:requiresFadingEdge="vertical|horizontal"
> 
> Why is horizontal useful here?

I assume you meant vertical there? This actually just needs the horizontal.
https://hg.mozilla.org/mozilla-central/rev/b7772a34efc5
https://hg.mozilla.org/mozilla-central/rev/34e45f316b66
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Had to disable fading edge due to performance regressions. I'll have to implement this in a different way. Re-opening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: -- → P1
Comment on attachment 8526129 [details] [diff] [review]
Add fading edge to tab strip (r=mcomella)

Here's a much simpler approach. I only draw fading on the right edge. I feel we don't really need a fading edge on the left. I'll double-check with antlam.
Attachment #8526129 - Flags: review?(michael.l.comella)
Anthony, I've made the fading edge very subtle, here's an apk: https://dl.dropboxusercontent.com/u/1187037/fading-edge.apk

Thoughts?
Flags: needinfo?(alam)
Comment on attachment 8526129 [details] [diff] [review]
Add fading edge to tab strip (r=mcomella)

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

How do we know how much more performant this is over the previous implementation?

I skimmed the Android source and found that they do a few more calculations, save and restore canvas layers, as well as scale, rotate, and translate the canvas and for two sides (as opposed to one in your current patch). I'm not familiar enough with these operations to know how expensive each one and thus how your patch improves upon the old implementation.

Ignoring performance, looks functionally correct.

::: mobile/android/base/tabs/TabStripView.java
@@ +270,5 @@
>      @Override
> +    protected void onSizeChanged(int w, int h, int oldw, int oldh) {
> +        super.onSizeChanged(w, h, oldw, oldh);
> +
> +        fadingEdgePaint.setShader(new LinearGradient(w, 0, w - fadingEdgeSize, 0,

nit: I think it's more intuitive to draw left-to-right (assuming that also works) because of the english reading direction - any reason you chose right-to-left?

@@ +272,5 @@
> +        super.onSizeChanged(w, h, oldw, oldh);
> +
> +        fadingEdgePaint.setShader(new LinearGradient(w, 0, w - fadingEdgeSize, 0,
> +                new int[] { 0xDD292C29, 0x11292C29, 0x0 },
> +                new float[] { 0, 0.6f, 1.0f }, Shader.TileMode.CLAMP));

How did you come up with these colors and positions?
Attachment #8526129 - Flags: review?(michael.l.comella) → review+
Future travelers: As per comment 10, part 2 of this bug was disabled in part 2 of bug 1091519.
Attachment #8512798 - Attachment description: Screenshot → Screenshot (old implementation)
Attached image prev_rightoverflow.png
Gah, No matter what I try, I think that fading just doesn't look right (top one).

I have an idea though! Can we try this (bottom one)? If we do this WITH the patch that you guys had where we slide in the active tab (when the user releases their tap), we shouldn't have a problem!
Flags: needinfo?(alam) → needinfo?(lucasr.at.mozilla)
Clarifications from IRC:

<mcomella> Do the unselected tabs also overlap the + button?
<mcomella> Or do they go under?
<antlam> under
<mcomella> So they have a fading edge and the selected tab just overlaps and goes over?
<antlam> we could do a small fade if we find that we need there but it'll be a lot better than fading over the grey tab
<antlam> yep
(In reply to Michael Comella (:mcomella) from comment #14)
> Comment on attachment 8526129 [details] [diff] [review]
> Add fading edge to tab strip (r=mcomella)
> 
> Review of attachment 8526129 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How do we know how much more performant this is over the previous
> implementation?
> 
> I skimmed the Android source and found that they do a few more calculations,
> save and restore canvas layers, as well as scale, rotate, and translate the
> canvas and for two sides (as opposed to one in your current patch). I'm not
> familiar enough with these operations to know how expensive each one and
> thus how your patch improves upon the old implementation.

This is a much simpler approach than what Android's View does. Remember the reason why we reimplemented the way we draw the tab shapes in bug 1091519? Same reason here. Android's built-in implementation of fading edges applies an alpha mask which roughly means we'll be painting the view twice on each display frame.

> Ignoring performance, looks functionally correct.
> 
> ::: mobile/android/base/tabs/TabStripView.java
> @@ +270,5 @@
> >      @Override
> > +    protected void onSizeChanged(int w, int h, int oldw, int oldh) {
> > +        super.onSizeChanged(w, h, oldw, oldh);
> > +
> > +        fadingEdgePaint.setShader(new LinearGradient(w, 0, w - fadingEdgeSize, 0,
> 
> nit: I think it's more intuitive to draw left-to-right (assuming that also
> works) because of the english reading direction - any reason you chose
> right-to-left?

The gradient *is* drawn right-to-left here. I'd have to use a transformation Matrix to rotate the gradient otherwise.  

> @@ +272,5 @@
> > +        super.onSizeChanged(w, h, oldw, oldh);
> > +
> > +        fadingEdgePaint.setShader(new LinearGradient(w, 0, w - fadingEdgeSize, 0,
> > +                new int[] { 0xDD292C29, 0x11292C29, 0x0 },
> > +                new float[] { 0, 0.6f, 1.0f }, Shader.TileMode.CLAMP));
> 
> How did you come up with these colors and positions?

Basically trial-and-error. I basically tried values that make the gradient look good in the tab strip.
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Anthony Lam (:antlam) from comment #17)
> Created attachment 8526361 [details]
> prev_rightoverflow.png
> 
> Gah, No matter what I try, I think that fading just doesn't look right (top
> one).
> 
> I have an idea though! Can we try this (bottom one)? If we do this WITH the
> patch that you guys had where we slide in the active tab (when the user
> releases their tap), we shouldn't have a problem!

I don't follow. How is the tab strip and 'new tab' button supposed to behave in the second option?
Flags: needinfo?(alam)
(In reply to Lucas Rocha (:lucasr) from comment #19)
> The gradient *is* drawn right-to-left here. I'd have to use a transformation
> Matrix to rotate the gradient otherwise.  

I've never tried this, but intuitively, I'd figure we could swap the x0 and x1 values and reverse the colors/positions arrays:

fadingEdgePaint.setShader(new LinearGradient(w - fadingEdgeSize, 0, w, 0,
        new int[] { 0x0, 0x11292C29, 0xDD292C29 },
        new float[] { 1.0f, 0.6f, 0f }, Shader.TileMode.CLAMP));
(In reply to Lucas Rocha (:lucasr) from comment #20)
> I don't follow. How is the tab strip and 'new tab' button supposed to behave
> in the second option?

I'd like the "active tab" to float over the +. But the inactive ones behave as is (go under). I could Vidyo to explain this a bit better if you'd like.
Flags: needinfo?(alam)
Spoke about this on IRC with Lucas...

It seems too late in the cycle to land this. Instead, we're just going to do a combination of what we said in bug 1098245 and keep the left fade we currently have.

Though, this is still an experiment I'd like us to try (after we wrap up V1).
(In reply to Michael Comella (:mcomella) from comment #21)
> (In reply to Lucas Rocha (:lucasr) from comment #19)
> > The gradient *is* drawn right-to-left here. I'd have to use a transformation
> > Matrix to rotate the gradient otherwise.  
> 
> I've never tried this, but intuitively, I'd figure we could swap the x0 and
> x1 values and reverse the colors/positions arrays:
> 
> fadingEdgePaint.setShader(new LinearGradient(w - fadingEdgeSize, 0, w, 0,
>         new int[] { 0x0, 0x11292C29, 0xDD292C29 },
>         new float[] { 1.0f, 0.6f, 0f }, Shader.TileMode.CLAMP));

Fair enough, done.
https://hg.mozilla.org/mozilla-central/rev/5c804aa57e4b
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.