Closed Bug 1127901 Opened 9 years ago Closed 9 years ago

Improve the visibility of the "X" button in zoomed view

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: domivinc, Assigned: domivinc)

References

Details

Attachments

(12 files, 7 obsolete files)

82.21 KB, image/png
antlam
: feedback-
Details
73.81 KB, image/png
Details
77.12 KB, image/png
Details
92.60 KB, image/png
Details
49.35 KB, image/png
Details
47.15 KB, image/png
Details
78.76 KB, image/png
Details
1.73 MB, image/png
Details
89.66 KB, image/png
Details
110.48 KB, image/png
Details
115.35 KB, image/png
Details
30.37 KB, patch
Details | Diff | Splinter Review
Followup of bug 663803 (from comment 59 here https://bugzilla.mozilla.org/show_bug.cgi?id=663803#c59):

:mcomella 
* (followup) The "X" button can be hard to read sometimes due to the underlying content - maybe :antlam can come up with something there (and other UI nits)
:antlam
As Michael pointed out ^ the 'x' seems like it would be better served not being there in the "zoomed in area". That being said, I feel like it should just dismiss when I tap outside the magnifying glass area (in an area without links).
:domivinc
One issue with this approach is here: “in an area without links”. On some pages, the body has click/touch events attached. It will be impossible to close the zoomed view without trigger the body event.


:domivinc
One way to fix this issue is to replace the transparency by a white background.
Depends on: zoomedview
Assignee: nobody → domivinc
Attached patch patch1127901.diff (obsolete) — Splinter Review
Attachment #8567655 - Flags: review?(michael.l.comella)
Dominique, do you mind posting a screenshot?
Flags: needinfo?(domivinc)
Attached image zoomedview1.png
Flags: needinfo?(domivinc)
Attached image zoomedview2.png
Attached image zoomedview3.png
Attached image zoomedview4.png
Comment on attachment 8567675 [details]
zoomedview1.png

Anthony, what do you think?
Attachment #8567675 - Flags: feedback?(alam)
Comment on attachment 8567675 [details]
zoomedview1.png

This definitely needs to be increased. What size is it currently and where are we getting that 'x' icon from?

We should also look at placing that x in the middle (right aligned still) because it's awfully close to the Toolbar and will be to the hardware buttons if we place it on the bottom - that would be an annoying experience.

The hit area we use along the toolbar is 48dp x 48dp for our 'x' if that helps (as a non-arbitrary starting point at least)
Attachment #8567675 - Flags: feedback?(alam) → feedback-
(In reply to Anthony Lam (:antlam) from comment #8)
> Comment on attachment 8567675 [details]
> zoomedview1.png
> 
> This definitely needs to be increased. What size is it currently and where
> are we getting that 'x' icon from?
> 
> We should also look at placing that x in the middle (right aligned still)
> because it's awfully close to the Toolbar and will be to the hardware
> buttons if we place it on the bottom - that would be an annoying experience.
> 
> The hit area we use along the toolbar is 48dp x 48dp for our 'x' if that
> helps (as a non-arbitrary starting point at least)

Thanks Anthony.

The "x" size is 20 dp x 20 dp, the icon comes from the following existing resource: "drawable/close".

I'm going to make the changes and post new screen shots to get your feed back.
Attached image white.png
Attached image transparent.png
Attached image black.png
I used the 'x' from the toolbar (drawable/close_edit_mode_selector), and set the size to 48dp x 48dp.
I tested with different background colors (see attached files white.png, transparent.png and black.png), it's not really good!

Another idea: totally remove the 'x' and use the back button action to close the zoomed view. The drawback of this solution: when the user wants to go back to the previous page with the zoomed view on, the back button must be used twice.
Flags: needinfo?(alam)
Thanks Dominique! This gave me an idea. I'll try to get some mocks up.

It also seems like there's a lot of cases that keep creeping up. Let me take a look at this feature as a whole and hopefully get some design specs in here rather than make numerous small iterations. 

I looked at the previous bug as well but didn't find any mention of how we decided on "how much to zoom". Was this ever discussed? Can this be a user variable?
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(domivinc)
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #14)
> 
> It also seems like there's a lot of cases that keep creeping up. Let me take
> a look at this feature as a whole and hopefully get some design specs in
> here rather than make numerous small iterations. 

There is another point you should also take into account in your (re)design: the zoomed view behavior should be used when the user start to make a text selection. Some browsers display a zoom when the user starts a selection or moves the current selection. In this case, the zoomed view is really a user friendly behavior because most of the time, on small devices, it's hard to see where the selection starts and ends. I don't know if a bug is already open for this zoomed view linked to the selection tool.

> 
> I looked at the previous bug as well but didn't find any mention of how we
> decided on "how much to zoom". Was this ever discussed? Can this be a user
> variable?

Prior to bug 1126866, the zoom factor was fixed to 2 (for any  metric factor value of the page). After 1126866, the metrics factor of the page is used to calculate the zoom factor of the zoomed view.
We can easily put the zoom factor in a variable. This value could be set in a pref or it could also be changed using +/- buttons in a tool bar for instance. In this case, the right vertical tool bar could display 3 buttons: x (close), + (zoom in), - (zoom out).

If you want to investigate this kind of solution with a zoomed view tool bar, there is another button that could be added: a pinned zoomed view, the zoomed view area could be pinned and the user could scroll the main page without changing the zoomed area. The user case linked to a pinned zoomed view is the following: the user keeps a video area inside the pinned zoomed view in the top area of the screen, and scroll the page to read the comments linked to the video in the bottom area. When the user scrolls, the pinned zoomed view displays always the same original area without taking into account the scroll.
Flags: needinfo?(domivinc)
Flags: needinfo?(michael.l.comella)
NI :antlam to get new design ideas about the "x" button
Flags: needinfo?(alam)
Thanks for the NI Dominique. Am actively thinking about this. 

Will leave NI on for now.
Attached image prev_zoomedlinks1.png
Attaching mock. Ideally, I want to find a way to move this "bar" around so if the zoomed area were say... bottom left, the bar would adjust and be on top. Is this possible?
Flags: needinfo?(alam) → needinfo?(domivinc)
Attached image spec_zoomed1.png
Attaching specs. I also have a "x3" in there but not sure we want to cover that in this bug. The idea there is basically allowing users to change between a list of zoom ratios that we determine by pressing on that (x2, x3, x1.5, etc).

But, apart from that. Let me know if you have feedback or questions :)
Thanks Anthony for the mock-up. I started to work on it, one question and 2 remarks:

- Do you want a small gray border around the zoomed view (like in the second picture)?

- There is probably a technical issue regarding the rounded corner on the zoomed view side. Android doesn't like such design using rounded corners on a bitmap. There are different ways to implement it for other type of views using shape/corners xml elements but it should not work for a bitmap. 

- Regarding the move of the tool bar, it should not be an issue.

I should have a first implementation of your mock-up next Monday.
Flags: needinfo?(domivinc)
Hey Dominique!

(In reply to Dominique Vincent [:domivinc] from comment #20)
> Thanks Anthony for the mock-up. I started to work on it, one question and 2
> remarks:
> 
> - Do you want a small gray border around the zoomed view (like in the second
> picture)?

That's not a grey border, but a shadow

> - There is probably a technical issue regarding the rounded corner on the
> zoomed view side. Android doesn't like such design using rounded corners on
> a bitmap. There are different ways to implement it for other type of views
> using shape/corners xml elements but it should not work for a bitmap. 

Could you elaborate a bit more on what you mean by this?

> - Regarding the move of the tool bar, it should not be an issue.
> 
> I should have a first implementation of your mock-up next Monday.

Awesome!
(In reply to Anthony Lam (:antlam) from comment #21)

> > - There is probably a technical issue regarding the rounded corner on the
> > zoomed view side. Android doesn't like such design using rounded corners on
> > a bitmap. There are different ways to implement it for other type of views
> > using shape/corners xml elements but it should not work for a bitmap. 
> 
> Could you elaborate a bit more on what you mean by this?
> 

With a bitmap, the corners cannot be done using xml rules. There are some tricks to simulate the corners using xml but you need to have only one background color around the bitmap and some paddings. It’s not the case.
We can do the corners using java code or using a bitmap library. We can change the shape of the bitmap. But it will have a performance cost. In our case, the zoomed view is not a static bitmap but a dynamic bitmap coming from Gecko. The bitmap can be changed very often each second depending of the page content.
The other difficulty is linked to the tool bar. In the zoomed view, the bitmap has only 2 rounded corners and not 4. The position of the rounded corners depends of the position of the tool bar (top/bottom).  In general, the libraries methods used to change the shape of a bitmap implement only 4 rounded corners.

In the linked patch, I tried to limit the impact of the bitmap rounded process in term of performances. But the corners design has really a cost in term of code complexity and probably performances.


> That's not a grey border, but a shadow
> 

Regarding the shadow around the zoomed view, I made it using the following specification: 
    size: 2dp
    position: right and bottom
    color: url_bar_shadow

Feel free to change the different values, I will update the code accordingly.


About the zoom factor button, I’m using the following list "x2", "x3", "x1.5". After "x1.5", the zoom is done again using the first value of the list "x2".

An apk is available here to test the changes if you don’t want to apply the code patch and rebuild:
http://rusez.com/apk/fennec-39.0a1.en-US.android-arm.apk
(about:config to set the zoomed view pref ON)
Anthony, I'm waiting for your functional review before asking for a code review from Michael. 
I'm not sure that NI flag should be used in this bug workflow case.
Flags: needinfo?(alam)
Regarding the rounded corner bitmap, I found this:

http://stackoverflow.com/a/18230837

I'm not sure of the performance implications but a clip (from my understanding, "don't draw here!") seems like it wouldn't be too expensive. In particular, I'm not sure how it might compare to your implementation, but it's something to consider.
(In reply to Dominique Vincent [:domivinc] from comment #24)
> I'm not sure that NI flag should be used in this bug workflow case.

Yep! NI should be fine here - it gives users a notification on Bugzilla so any time you want to explicitly notify a user, it can be used.
(In reply to Dominique Vincent [:domivinc] from comment #24)
> Anthony, I'm waiting for your functional review before asking for a code
> review from Michael. 
> I'm not sure that NI flag should be used in this bug workflow case.

Looking good! This feels great. Although, the control bar seems to swap between the bottom and top somewhat unpredictably. Can we get the change to happen when the user 'let's go'?

Also the shadow needs to be seen more from all edges. mcomella, maybe you could help me understand how we're drawing this shadow and we could just quickly tweak this?
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #25)
> Regarding the rounded corner bitmap, I found this:
> 
> http://stackoverflow.com/a/18230837
> 
> I'm not sure of the performance implications but a clip (from my
> understanding, "don't draw here!") seems like it wouldn't be too expensive.
> In particular, I'm not sure how it might compare to your implementation, but
> it's something to consider.

Michael, reading the post you provided, there is an answer regarding the pros and cons about the "clip" method in the following comment:
http://stackoverflow.com/questions/18229358/bitmap-in-imageview-with-rounded-corners/27469651#27469651

Drawing quality should be better with the current implementation, path clipping doesn't support anti-aliasing.

> Also the shadow needs to be seen more from all edges. mcomella, 
> maybe you could help me understand how we're drawing 
> this shadow and we could just quickly tweak this?

I'm not a css expert but my definition of a shadow visible on 4 edges is a border!
In css, there is a "box-shadow" allowing multiple shadows with different colors, different positions, ...
Many css examples are available in the following document:
www.css3.info/preview/box-shadow/

Let me know what is your preferred css shadow effect from the previous examples list, I should be able to reproduce it in Android.

> Although, the control bar seems to swap between the bottom and top somewhat unpredictably. 

The control bar swap occurs when the middle of the zoomed view is moving from the top area to the bottom area of the screen (or bottom from top). The limit between the top/bottom areas is in the middle of the screen and the height of this area is only 1px (very small height for the swapping area: it's a 1 px horizontal line).

> Can we get the change to happen when the user 'let's go'?
We can make this change but it won't work when the user will go on top or bottom edges of the screen moving from the other area. 
For instance: the zoomed view is in the top area, the control bar is displayed in the bottom of the zoomed view. If the user moves the zoomed view near the bottom edge of the screen, the control bar will still be displayed in the bottom and the user won't be able to position the zoomed view near the bottom edge of the screen. 

In the current implementation, the strange effect (probably what Anthony called "somewhat unpredictably") is due to the very small height of the swapping area: 1px. When the zoomed view is just in the middle of the screen, you can move 1 px higher the limit and the control bar is moved from top to bottom. If you move back from 1 px, the control bar moves from bottom to top ... 
We can improve the user experience using a taller swapping area in the middle of the screen (for instance 50dp). The swap will occur in 2 different limits depending of the direction of the move. 
Let me know if you want to try this solution.
Michael, could you have a look to my previous comment? I need to know what you want to do for the shadow and for the size of the swapping area.
Hey domivinc,

I hadn't realize that the shadow was drawn in CSS. I can't find them right now but I just spec'd out the specific shadow in our Reader View controls on Tablet (CSS also) so we should aim to use the same ones.
(In reply to Anthony Lam (:antlam) from comment #30)
> Hey domivinc,
> 
> I hadn't realize that the shadow was drawn in CSS. I can't find them right
> now but I just spec'd out the specific shadow in our Reader View controls on
> Tablet (CSS also) so we should aim to use the same ones.

Sorry Anthony, my previous explanation is probably not clear;  the current shadow is in the Android xml file (and not in css).
I just added css samples in my comment to get an idea of what type of shadows you want. There are more css samples available on Internet than Android xml shadow samples.
You just have to add a link to your shadow style (android, css, ...) and I will try to get the same effect for the zoomed view using an Android xml style structure.
If you want to get exactly the same effect used in the Reader View controls on tablet, could you add a screen copy or a link to the bug where I could find the visual design of this Reader View control.
Thanks for your help.
Flags: needinfo?(alam)
As per mcomella's bug referral, let's try to get that shadow in there.

Apart from that, I've also noticed that the menu bar switches once I cross half way of the devices vertical height. I think this bar should only switch once it comes into contact with the top and bottom edge of the screen. Can we please adjust that?

Thanks domivinc! it's almost there! :)
Flags: needinfo?(alam) → needinfo?(domivinc)
Sorry for the delay, Dominique.

For the shadow, as of the past week, we started using a background shadow drawable with the undesirable side effect of overdraw. Perf doesn't seem to be an issue on the few dialogues we've used it on thus far. ONote that this drawable has rounded corners so the container which is casting the shadow should also have rounded corners.

I just landed an implementation overlay_share_toast [1] on fx-team (in bug 1130203; note that the link will point to invalid data until the changesets get merged onto mozilla-central, probably tonight PST or tomorrow night PST).

Do you think this method would work for you?

Once we have more time, I think I'd like to try to create a more performant shadow via the onDraw method (that only draws where the View is not.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/overlay_share_toast.xml
And is Anthony's comment 32 sufficient for the swap area changes, Dominique?
Flags: needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #34)
> And is Anthony's comment 32 sufficient for the swap area changes, Dominique?

Yes, I will implement the swap area design from Anthony's comment 32.
I didn't yet investigate the changes required to fit your shadow implementation done in bug 1130203.

I should be able to work on both points tomorrow!
Flags: needinfo?(domivinc)
Added in this patch:
- the tool bar switches once it comes into contact with the top or the bottom edge of the screen,
- background shadow drawable is now used like in overlay_share_toast xml file.
Attachment #8574384 - Attachment is obsolete: true
Attachment #8584967 - Flags: review?(michael.l.comella)
Comment on attachment 8584967 [details] [diff] [review]
patch-28032015 1-Bug_1127901___Zoomed_view_tool_bar_implementation___r_mcomella.patch

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

I haven't finished the math in ZoomedView yet, but here's some to start - looking good so far!

::: mobile/android/base/ZoomedView.java
@@ +41,5 @@
>  
>  import java.nio.ByteBuffer;
>  import java.text.DecimalFormat;
>  
> +public class ZoomedView extends RelativeLayout implements LayerView.OnMetricsChangedListener,

RelativeLayouts do excess measurements and can be expensive - setting this to FrameLayout should be fine.

@@ +58,5 @@
>      private LayerView layerView;
>      private int viewWidth;
> +    private int viewHeight; // Only the zoomed view height, no toolbar, no shadow ...
> +    private int viewOuterWidth;
> +    private int viewOuterHeight; // Zoomed view height with toolbar and other elements like shadow, ...

nit: Maybe -> viewContainerHeight?

@@ +59,5 @@
>      private int viewWidth;
> +    private int viewHeight; // Only the zoomed view height, no toolbar, no shadow ...
> +    private int viewOuterWidth;
> +    private int viewOuterHeight; // Zoomed view height with toolbar and other elements like shadow, ...
> +    private int outerElementsSize; // shadow, margin, ...

nit: -> containterElementsSize ?

Your choice of outer or container - the latter is just more intuitive to me.

@@ +67,5 @@
> +    private ImageView closeButton;
> +    private TextView changeZoomFactorButton;
> +    private boolean toolBarOnTop;
> +    private float offsetDueToToolBarPosition;
> +    private int toolBarHeight;

nit: we consider "toolbar" as one word everywhere else in the code (i.e. -> toolbar || Toolbar)

@@ +79,5 @@
>      private Runnable requestRenderRunnable;
>      private long startTimeReRender;
>      private long lastStartTimeReRender;
>  
> +    class RoundedBitmapDrawable extends BitmapDrawable {

Any reason this shouldn't be private?

@@ +88,5 @@
> +        RoundedBitmapDrawable(Resources res, Bitmap bitmap, boolean squareOnTop, int cornerRadius) {
> +            super(res, bitmap);
> +            squareOnTopOfDrawable = squareOnTop;
> +            BitmapShader shader;
> +            shader = new BitmapShader(bitmap, Shader.TileMode.CLAMP,

nit: combine with declaration & final

@@ +92,5 @@
> +            shader = new BitmapShader(bitmap, Shader.TileMode.CLAMP,
> +                Shader.TileMode.CLAMP);
> +            paint.setAntiAlias(true);
> +            paint.setShader(shader);
> +            roundPx = cornerRadius;

nit: cornerRadius makes more sense as a name to me than roundPx.

@@ +101,5 @@
> +            int height = getBounds().height();
> +            int width = getBounds().width();
> +            RectF rect = new RectF(0.0f, 0.0f, width, height);
> +            canvas.drawRoundRect(rect, roundPx, roundPx, paint);
> +          //draw rectangles over the corners we want to be square

nit: indentation. Could use a new line above this too.

@@ +102,5 @@
> +            int width = getBounds().width();
> +            RectF rect = new RectF(0.0f, 0.0f, width, height);
> +            canvas.drawRoundRect(rect, roundPx, roundPx, paint);
> +          //draw rectangles over the corners we want to be square
> +            if (squareOnTopOfDrawable == true) {

nit: if (squareOnTopOfDrawable)

@@ +104,5 @@
> +            canvas.drawRoundRect(rect, roundPx, roundPx, paint);
> +          //draw rectangles over the corners we want to be square
> +            if (squareOnTopOfDrawable == true) {
> +                canvas.drawRect(0, 0, roundPx, roundPx, paint);
> +                canvas.drawRect(width-roundPx, 0, width, roundPx, paint);

nit: space between arithmetic operators (i.e. width - roundPx): here and below

@@ +115,1 @@
>      private class ZoomedViewTouchListener implements View.OnTouchListener {

nit: newline above this line.

@@ +162,5 @@
>              }
>              return true;
>          }
>  
> +        private boolean clickInZoomedView(float y) {

nit: -> isClickInZoomedView

@@ +164,5 @@
>          }
>  
> +        private boolean clickInZoomedView(float y) {
> +            if ((toolBarOnTop == true && y < toolBarHeight) ||
> +                (toolBarOnTop == false && y > ZoomedView.this.viewHeight)) {

nit: `if ((toolBarOnTop &&` and so forth - there are some more below (I'm not going to comment on it anymore).

@@ +165,5 @@
>  
> +        private boolean clickInZoomedView(float y) {
> +            if ((toolBarOnTop == true && y < toolBarHeight) ||
> +                (toolBarOnTop == false && y > ZoomedView.this.viewHeight)) {
> +                return false;

You can just return the result of the boolean statement instead.

@@ +260,1 @@
>          ImmutableViewportMetrics metrics = layerView.getViewportMetrics();

nit: newline above

@@ +464,5 @@
> +        }
> +        zoomFactor = ZOOM_FACTORS_LIST[currentZoomFactorIndex];
> +        ImmutableViewportMetrics metrics = layerView.getViewportMetrics();
> +        refreshZoomedViewSize(metrics);
> +        if(zoomFactor == (int) zoomFactor) {

`== (int)` seems ambiguous - perhaps `Math.round` or `Math.floor`?

::: mobile/android/base/resources/drawable/zoomedview_rounded_corner.xml
@@ +13,5 @@
> +        android:color="@color/toolbar_grey">
> +    </solid>
> +    <!-- Here is the corner radius -->
> +    <corners
> +        android:radius="@dimen/zoomed_view_corner_radius">

This can be more succinct: see [1] as a guide.

However, if we use @dimen/button_corner_radius (see later), we can just use [2] instead.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable/fxaccount_password_active.xml#16
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable/toolbar_grey_round.xml#8

::: mobile/android/base/resources/layout/zoomed_view.xml
@@ +12,5 @@
>      android:layout_height="wrap_content"
> +    android:background="@drawable/dropshadow"
> +    android:padding="@dimen/zoomed_view_shadow_size"
> +    android:visibility="gone">
> +    <RelativeLayout

RelativeLayouts do excess measuring and thus can be expensive - can this be done with a LinearLayout or FrameLayout?

nit: new line above this line

::: mobile/android/base/resources/values/dimens.xml
@@ +202,5 @@
>      <dimen name="find_in_page_control_margin_top">2dip</dimen>
>  
> +    <!-- ZoomedView dimensions. -->
> +    <dimen name="zoomed_view_toolbar_height">44dp</dimen>
> +    <dimen name="zoomed_view_shadow_size">3dp</dimen>

Let's make this generic: -> `drawable_dropshadow_size` or similar.

@@ +203,5 @@
>  
> +    <!-- ZoomedView dimensions. -->
> +    <dimen name="zoomed_view_toolbar_height">44dp</dimen>
> +    <dimen name="zoomed_view_shadow_size">3dp</dimen>
> +    <dimen name="zoomed_view_corner_radius">4dp</dimen>

Did you get 4dp from antlam? We have `@dimen/button_corner_radius` of 3dp so I wonder if we should either change that to 4dp or change this to 3dp?
Comment on attachment 8584967 [details] [diff] [review]
patch-28032015 1-Bug_1127901___Zoomed_view_tool_bar_implementation___r_mcomella.patch

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

Sorry, I forgot to update - snorp will be handling the calculation updates.

This is looking pretty good to me, though I'd like to review it again after some of the nits are cleaned up.
Attachment #8584967 - Flags: review?(snorp)
Attachment #8584967 - Flags: review?(michael.l.comella)
Attachment #8584967 - Flags: feedback+
Michael all the changes done except for this one:

>>RelativeLayouts do excess measuring and thus can be expensive - can this be
>>done with a LinearLayout or FrameLayout?

I didn’t find a way to do it with only one LinearLayout or FrameLayout. 

I’m waiting for the code review from :snorp and I will go back to you for a last code review.
Attachment #8584967 - Attachment is obsolete: true
Attachment #8584967 - Flags: review?(snorp)
Attachment #8588025 - Flags: review?(snorp)
Hi Michael, do you have time to work on this task in order to get a last code review? Let me know if you want more changes on the code, I will try to implement them.
Flags: needinfo?(michael.l.comella)
Attachment #8588025 - Flags: review?(snorp) → review+
Comment on attachment 8588025 [details] [diff] [review]
patch-03042015 1-Bug_1127901___Zoomed_view_tool_bar_implementation___r_mcomella.patch

Hi Michael, back to you for another review (see your comment 39).
Flags: needinfo?(michael.l.comella)
Attachment #8588025 - Flags: review?(michael.l.comella)
Comment on attachment 8588025 [details] [diff] [review]
patch-03042015 1-Bug_1127901___Zoomed_view_tool_bar_implementation___r_mcomella.patch

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

Sorry for the wait - lgtm w/ nits.

::: mobile/android/base/ZoomedView.java
@@ +66,5 @@
>      private boolean shouldSetVisibleOnUpdate;
>      private PointF returnValue;
> +    private ImageView closeButton;
> +    private TextView changeZoomFactorButton;
> +    private boolean toolBarOnTop;

nit: toolBarOnTop -> toolbarOnTop

@@ +166,5 @@
>          }
>  
> +        private boolean isClickInZoomedView(float y) {
> +            return ((toolBarOnTop && y < toolbarHeight) ||
> +                (toolBarOnTop == false && y > ZoomedView.this.viewHeight));

nit: `toolBarOnTop == false` -> `!toolBarOnTop`

@@ +241,5 @@
>          zoomedImageView = (ImageView) findViewById(R.id.zoomed_image_view);
> +        this.setOnTouchListener(new ZoomedViewTouchListener());
> +
> +        toolbarHeight = getResources().getDimensionPixelSize(R.dimen.zoomed_view_toolbar_height);
> +        containterElementsSize = getResources().getDimensionPixelSize(R.dimen.drawable_dropshadow_size);

nit: -> containerSize

@@ +370,5 @@
> +        }
> +        RelativeLayout.LayoutParams p = new RelativeLayout.LayoutParams(RelativeLayout.LayoutParams.WRAP_CONTENT,
> +                RelativeLayout.LayoutParams.WRAP_CONTENT);
> +        RelativeLayout.LayoutParams pChangeZoomFactorButton = new RelativeLayout.LayoutParams(RelativeLayout.LayoutParams.WRAP_CONTENT, toolbarHeight);
> +        RelativeLayout.LayoutParams pCloseButton = new RelativeLayout.LayoutParams(toolbarHeight, toolbarHeight);

Can you use getLayoutParams instead of constructing new params?

Note that you should still call setLayoutParams with the returned getLayoutParams (it request a layout). (Lucasr once said you can just call requestLayout directly instead of setLayoutParams in some situations as an optimization but I don't remember the details...)

@@ +465,5 @@
> +        zoomFactor = ZOOM_FACTORS_LIST[currentZoomFactorIndex];
> +
> +        ImmutableViewportMetrics metrics = layerView.getViewportMetrics();
> +        refreshZoomedViewSize(metrics);
> +        if(zoomFactor == Math.round(zoomFactor)) {

nit: `if (`

@@ +466,5 @@
> +
> +        ImmutableViewportMetrics metrics = layerView.getViewportMetrics();
> +        refreshZoomedViewSize(metrics);
> +        if(zoomFactor == Math.round(zoomFactor)) {
> +            changeZoomFactorButton.setText("x"+Integer.toString((int)zoomFactor));

Wondering if these strings should be localized.

I'll NI l10n.
Attachment #8588025 - Flags: review?(michael.l.comella) → review+
Axel, we include some strings that are magnification levels, i.e. "x2", "x4", etc. Should these be localized?
Flags: needinfo?(l10n)
(In reply to Michael Comella (:mcomella) from comment #44)
> Axel, we include some strings that are magnification levels, i.e. "x2",
> "x4", etc. Should these be localized?

I think it's better to have them localizable, with a very clear localization comment explaining what's going to happen with that string and space constraints, and layout able to cope with a long string (I guess cutting it off).

To be honest, I would have expected 2x, 3x, etc. even for English, and I wonder if percentages would be clearer.
Flags: needinfo?(l10n)
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #45)
> To be honest, I would have expected 2x, 3x, etc. even for English, and I
> wonder if percentages would be clearer.

Just in case, note that also the percentage should be a localizable string, since there might different typographical rules.
Michael, last code review changes done! 
From my point of view, the proposal from :flod regarding the display of the magnification levels using percentages looks better. I tested it (see attached screen copies and last patch). This proposal avoids string localization.
Anthony, could you have a look to the screen copies and tell us if the percentages option is good for you?
Flags: needinfo?(alam)
Thanks Francesco, I will add the percentage in a localizable string if this design is confirmed.
%'s works for me. 

I know I've asked for a build before but since this has been on/off - could I get a final build to make sure all the UX is good to go? :)

Thanks guys!
Flags: needinfo?(alam) → needinfo?(domivinc)
Last build available here: http://rusez.com/apk/fennec-40.0a1.en-US.android-arm.apk
Flags: needinfo?(domivinc)
Looks good!
Flags: needinfo?(alam)
Flags: needinfo?(michael.l.comella)
Michael, I added the percentage in a localizable string.
Attachment #8588025 - Attachment is obsolete: true
Attachment #8600346 - Attachment is obsolete: true
Attachment #8600457 - Flags: review?(michael.l.comella)
Comment on attachment 8600457 [details] [diff] [review]
patch-02052015 1-Bug_1127901___Zoomed_view_tool_bar_implementation___r_mcomella.patch

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

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +614,5 @@
>  
>  <!ENTITY colon ":">
>  
> +<!-- LOCALIZATION NOTE (percent): This text is appended after a number to display a percentage. -->
> +<!ENTITY percent "&formatS;&#37;&#37;">

The localization note is incorrect: "&formatS;" will be replaced by the number, so the text is not appended after a number. Please fix it.

Also, I'd explain a bit what this percentage represents (maybe the context of zoom is important in some cultures/languages).
Attachment #8600457 - Flags: review?(michael.l.comella) → review+
Also add that "&#37;" is the percent sign, and why we have two of them (being encoded, I didn't expect it myself).
Michael, all tests are green now.
In case of localization changes, is there something special to do before adding the "checkin-needed" keyword?
(In reply to Dominique Vincent [:domivinc] from comment #59)
> In case of localization changes, is there something special to do before
> adding the "checkin-needed" keyword?

No. New strings will be exposed automatically on l10n dashboards and tools.
Keywords: checkin-needed
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #60)
> (In reply to Dominique Vincent [:domivinc] from comment #59)
> > In case of localization changes, is there something special to do before
> > adding the "checkin-needed" keyword?
> 
> No. New strings will be exposed automatically on l10n dashboards and tools.

Thanks Francesco!
Comment 63 and 64 were investigating a crash that ended up being unrelated to this patch.
https://hg.mozilla.org/mozilla-central/rev/42152093a51c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Flags: needinfo?(michael.l.comella)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.