Closed Bug 792410 Opened 7 years ago Closed 7 years ago

thumbnails are currently cut off in the tabs tray, update thumbnail styling in tabs menu

Categories

(Firefox for Android :: Theme and Visual Design, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox19 + verified
firefox20 --- verified
firefox21 --- verified
fennec 19+ ---

People

(Reporter: ibarlow, Assigned: wesj)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached image tab thumbnails
Building on the work Wes has done in bug 787765 to fix our thumbnail styling on about:home, we should apply these changes in the tab menu as well. Specifically, 

1. Removing the "recessed" border
2. Adding a drop shadow -- 2dp, no blur, black with 25% opacity
3. Bonus points for 2dp rounded corners

A mockup is attached here. Note that tab thumbnails have a shorter aspect ratio than the ones on about:home, since they don't have a title overlay along the bottom.
Attached patch patch (obsolete) — Splinter Review
I removed the border, added the shadow, and made the thumbnails bigger.

For the shadow height, I chose 88dp = 108dp (local_tab_row_height) - 2*10dp (margin). Then for the thumbnail height I chose 86dp to make the 2dp shadow, and then scaled the width to keep it close to the same width/height ratio as before.

I didn't know how to add the rounded corners, but it looks like we don't have those on about:home right now anyway.
Assignee: nobody → margaret.leibovic
Attachment #677971 - Flags: review?(sriram)
Attached image screenshot (obsolete) —
Ian, how's this?

(PS, I'm doing this for the maple cookies ;)
Attachment #677972 - Flags: feedback?(ibarlow)
Attached patch patch v2 (obsolete) — Splinter Review
Actually, I realized we can get rid of that second ImageView.
Attachment #677971 - Attachment is obsolete: true
Attachment #677971 - Flags: review?(sriram)
Attachment #677978 - Flags: review?(sriram)
(In reply to Margaret Leibovic [:margaret] from comment #2)
> Created attachment 677972 [details]
> screenshot
> 
> Ian, how's this?
> 
> (PS, I'm doing this for the maple cookies ;)

This is looking awesome! You might just want to check your shadow opacity and size compared to what Wes did on about:home. These look a little on the heavy side to me. 

Other than that, very cookie worthy :)
I had to remove the shadows on about:home for performance reasons.
(In reply to Ian Barlow (:ibarlow) from comment #4)

> This is looking awesome! You might just want to check your shadow opacity
> and size compared to what Wes did on about:home. These look a little on the
> heavy side to me. 

After implementing this, I was actually wondering if I did the right thing. Basically my patch just puts a 2dp gray shadow below the thumbnail image, but given that we're talking about opacity, should this shadow be overlapping the bottom 2dp of the thumbnail image?

(In reply to Wesley Johnston (:wesj) from comment #5)
> I had to remove the shadows on about:home for performance reasons.

Uh oh, what were the problems? Were you doing the shadow differently than I'm doing in this patch?
Comment on attachment 677978 [details] [diff] [review]
patch v2

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

This looks good. But..

1. R.drawable.tab_thumbnail_shadow in different folders must be removed if they aren't used anywhere else.
2. layout-xlarge-v11/tabs_row.xml isn't modified. 10" tablets will suffer.

Doubts:
1. How would 150dp width work on smaller phones? Isn't the text super truncated?

Suggestions:
If we are creating a similar thumbnail for both about:home and tabs-tray, we could merge them both to a custom view, and use it in XML.
Thereby when "rounded corners" or "shadows" are added, they would work in all places the same way.
Attachment #677978 - Flags: review?(sriram) → review-
The way "selected tab" is shown has changed in newer mockups. It's better to fix them all in one-go (as they all are layout changes in the same file).
(In reply to Sriram Ramasubramanian [:sriram] from comment #7)

> 1. R.drawable.tab_thumbnail_shadow in different folders must be removed if
> they aren't used anywhere else.
> 2. layout-xlarge-v11/tabs_row.xml isn't modified. 10" tablets will suffer.

Oops, good catches. I'll fix those. :)

> Doubts:
> 1. How would 150dp width work on smaller phones? Isn't the text super
> truncated?

I'll can test that on a Galaxy S, but I think that's the smallest screen phone I have.

> Suggestions:
> If we are creating a similar thumbnail for both about:home and tabs-tray, we
> could merge them both to a custom view, and use it in XML.
> Thereby when "rounded corners" or "shadows" are added, they would work in
> all places the same way.

The about:home thumbnails are slightly different because they have the title in them at the bottom. I'm not sure if that will make sharing them more complicated.

(In reply to Sriram Ramasubramanian [:sriram] from comment #8)
> The way "selected tab" is shown has changed in newer mockups. It's better to
> fix them all in one-go (as they all are layout changes in the same file).

Are these mockups in a bug somewhere?

I'll try to find some time to update my patch this weekend. Contributing to Fennec is like my hobby right now :)
Duplicate of this bug: 823297
Without these changes, thumbnails are currently cut off in the tabs tray (see screenshot in bug 823297).
Blocks: 787765
Oh no, pressure's on! I can try to get back into this later this week, but anyone, feel free to take this from me if you have the time sooner.
Sriram - Margaret is understandably slammed with other B2G work. Any chance you can jump on this early in the FF19 beta cycle?
Flags: needinfo?(sriram)
Margaret is coming back! :)
But ya, I'm working on tabs menu changes and can take care of this.
Flags: needinfo?(sriram)
Yeah, I'm sorry, I won't have time for this before the 24th, which is a long time from now.
Assignee: margaret.leibovic → sriram
Can we get a fix finished this week? We'd really like to get this into our third beta (going to build Tuesday), since it'll be hard to justify this fix much later than that.
Summary: Update thumbnail styling in tabs menu → thumbnails are currently cut off in the tabs tray, update thumbnail styling in tabs menu
If this is regarding the styling, it'll done as a part of private browsing. https://bug817675.bugzilla.mozilla.org/attachment.cgi?id=697581
I don't know why it is being tracked for 19, and what's expected for that.
tracking-fennec: --- → ?
Assignee: sriram → wjohnston
(In reply to Sriram Ramasubramanian [:sriram] from comment #7)
> Suggestions:
> If we are creating a similar thumbnail for both about:home and tabs-tray, we
> could merge them both to a custom view, and use it in XML.

I have a WIP for this, but since this will need to be uplifted at this point, it seems better to do something simple like what margaret has here. I'll unbitrot and clean it up a bit.
tracking-fennec: ? → 19+
Attached patch Patch v1Splinter Review
This is more complex than it looks. Basically, we need a scale type that will maintain the thumbnail's aspect ratio, but force it to take up the entire view size. CENTER_CROP does that, but doesn't keep the view's top left corner in place, leading to the thumbnail looking clipped. This does it using a custom GeckoImageView class and keeps the top left in place. I anticipate we can extend it later if we need support for other things.

We should also use this on about:home, but wanted to minimize the risk for beta 3 a bit.
Attachment #677978 - Attachment is obsolete: true
Attachment #703586 - Flags: review?
Comment on attachment 703586 [details] [diff] [review]
Patch v1

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

Looks good. Please test it in GB phones with the canvas changes.
r+ with the changes.

::: mobile/android/base/GeckoViewsFactory.java
@@ +111,5 @@
>                  return new GeckoTextSwitcher(context, attrs);
>              else if (TextUtils.equals(viewName, "TextView"))
>                  return new GeckoTextView(context, attrs);
> +            else if (TextUtils.equals(viewName, "widget.GeckoImageView"))
> +                return new GeckoImageView(context, attrs);

The identifier could be "Gecko.ThumbnailView" and you could compare only "ThumbnailView".
The class name could be "ThumbnailView".

GeckoImageView is so generic in that it doesn't convey the meaning.

::: mobile/android/base/resources/layout-xlarge-v11/tabs_row.xml
@@ +21,5 @@
>                          android:layout_height="wrap_content"
>                          android:layout_weight="1.0"
>                          android:layout_margin="10dip">
>  
> +            <org.mozilla.gecko.widget.GeckoImageView android:id="@+id/thumbnail"

<Gecko.ThumbnailView/> is better.

::: mobile/android/base/widget/GeckoImageView.java
@@ +15,5 @@
> +
> +/* Special version of ImageView for thumbnails. Scales a thumbnail so that it maintains its aspect
> + * ratio and so that the images width and height are the same size or greater than the view size
> + */
> +public class GeckoImageView extends ImageView {

GeckoImageView is so generic in that it doesn't convey the meaning.
ThumbnailView could be a better one.

@@ +26,5 @@
> +        super(context, attrs);
> +    }
> +
> +    @Override
> +    public void onDraw(Canvas canvas) {

This requires saving the canvas, doing the draw, and restoring the canvas.

@@ +33,5 @@
> +            int w1 = d.getIntrinsicWidth();
> +            int h1 = d.getIntrinsicHeight();
> +            int w2 = getWidth();
> +            int h2 = getHeight();
> +    

Extra space.

@@ +39,5 @@
> +            if (w2/h2 < w1/h1) {
> +                scale = (float)h2/h1;
> +            } else {
> +                scale = (float)w2/w1;
> +            }

This could be shortened using a ternary operator.

@@ +41,5 @@
> +            } else {
> +                scale = (float)w2/w1;
> +            }
> +
> +            mMatrix = new Matrix();

Android forbids creating new Matrix inside onDraw(). This will show error with lint.
Could you initialize just once, and change the scale when needed?

@@ +50,5 @@
> +        d.draw(canvas);
> +    }
> +
> +    @Override
> +    protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {

I don't think we need to do this on onMeasure().
onSizeChanged() could work I guess. And, we could make the scale of the matrix to 1.0f instead of setting it to null.
There by, only one object is used.
Attachment #703586 - Flags: review? → review+
Comment on attachment 703586 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 787765 - retheme about:home thumbnails
User impact if declined: lack of polish on tabs tray thumbnails
Testing completed (on m-c, etc.): landed on mc today
Risk to taking this patch (and alternatives if risky): Low. While there's a lot of code here to extend the Android ImageView, most of its just java stuff. This does the exact same thing as the normal imageView with a slightly different scale matrix.
String or UUID changes made by this patch: none.
Attachment #703586 - Flags: approval-mozilla-beta?
Attachment #703586 - Flags: approval-mozilla-aurora?
Comment on attachment 703586 [details] [diff] [review]
Patch v1

[Triage Comment]
We'll all need to be on the lookout for tab/UX regressions, but this is good to go into FF19b3 (going to build Tuesday).
Attachment #703586 - Flags: approval-mozilla-beta?
Attachment #703586 - Flags: approval-mozilla-beta+
Attachment #703586 - Flags: approval-mozilla-aurora?
Attachment #703586 - Flags: approval-mozilla-aurora+
Grr... Playing with the onSizeChange/onLayout fix i forgot to add a curly brace back in the final patch. Landed a fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/40badd9d545a
Heh. I missed a bunch of review nits and broke the tree, so I just yanked this and will put it back in right.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe60d3c7d0a0
Back in. Tested and everything builds fine here. Sorry 'bout that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba89f8698668
https://hg.mozilla.org/mozilla-central/rev/ba89f8698668
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Attachment #677972 - Attachment is obsolete: true
Attachment #677972 - Flags: feedback?(ibarlow)
Unable to reproduce on Nightly 21.0a1 (2012-01-28), Aurora 20.0a2 (2012-01-28), Beta 19.0b3.

Verifying.
You need to log in before you can comment on or make changes to this bug.