Closed
Bug 1090364
Opened 10 years ago
Closed 10 years ago
Add fading edge to tab strip
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
Attachments
(6 files)
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/jpeg
|
Details | |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details |
This is related to bug 1024816 but focused on the fading edge aspect.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8512785 -
Flags: review?(michael.l.comella)
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b7772a34efc5 https://hg.mozilla.org/integration/fx-team/rev/34e45f316b66
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
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 → ---
Assignee | ||
Updated•10 years ago
|
Priority: -- → P1
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Comment 15•10 years ago
|
||
Future travelers: As per comment 10, part 2 of this bug was disabled in part 2 of bug 1091519.
Updated•10 years ago
|
Attachment #8512798 -
Attachment description: Screenshot → Screenshot (old implementation)
Comment 17•10 years ago
|
||
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)
Comment 18•10 years ago
|
||
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
Assignee | ||
Comment 19•10 years ago
|
||
(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)
Assignee | ||
Comment 20•10 years ago
|
||
(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)
Comment 21•10 years ago
|
||
(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));
Comment 22•10 years ago
|
||
(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)
Comment 23•10 years ago
|
||
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).
Updated•10 years ago
|
Assignee | ||
Comment 24•10 years ago
|
||
(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.
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5c804aa57e4b
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•