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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dominique Vincent [:domivinc]]

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.

Comment 1

[Dominique Vincent [:domivinc]]

Attachment: patch1127901.diff

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Dominique, do you mind posting a screenshot?

Comment 3

[Dominique Vincent [:domivinc]]

Attachment: zoomedview1.png

Comment 4

[Dominique Vincent [:domivinc]]

Attachment: zoomedview2.png

Comment 5

[Dominique Vincent [:domivinc]]

Attachment: zoomedview3.png

Comment 6

[Dominique Vincent [:domivinc]]

Attachment: zoomedview4.png

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8567675 [details]
zoomedview1.png

Anthony, what do you think?

Comment 8

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

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)

Comment 9

[Dominique Vincent [:domivinc]]

(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.

Comment 10

[Dominique Vincent [:domivinc]]

Attachment: white.png

Comment 11

[Dominique Vincent [:domivinc]]

Attachment: transparent.png

Comment 12

[Dominique Vincent [:domivinc]]

Attachment: patch-25022015 1-Bug_1127901___White_background_for_the_close_button_of_zoomed_view___r_mcomella.patch

Comment 13

[Dominique Vincent [:domivinc]]

Attachment: 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.

Comment 14

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

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?

Comment 15

[Dominique Vincent [:domivinc]]

(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.

Comment 16

[Dominique Vincent [:domivinc]]

NI :antlam to get new design ideas about the "x" button

Comment 17

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

Thanks for the NI Dominique. Am actively thinking about this. 

Will leave NI on for now.

Comment 18

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

Attachment: 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?

Comment 19

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

Attachment: 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 :)

Comment 20

[Dominique Vincent [:domivinc]]

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.

Comment 21

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

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!

Comment 22

[Dominique Vincent [:domivinc]]

Attachment: patch-08032015 1-Bug_1127901___Zoomed_view_tool_bar_implementation___r_mcomella.patch

Comment 23

[Dominique Vincent [:domivinc]]

(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)

Comment 24

[Dominique Vincent [:domivinc]]

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.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 27

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

(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?

Comment 28

[Dominique Vincent [:domivinc]]

(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.

Comment 29

[Dominique Vincent [:domivinc]]

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.

Comment 30

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

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.

Comment 31

[Dominique Vincent [:domivinc]]

(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.

Comment 32

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

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! :)

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

And is Anthony's comment 32 sufficient for the swap area changes, Dominique?

Comment 35

[Dominique Vincent [:domivinc]]

(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!

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note: investigate drawing shadow is bug 1130201.

Comment 37

[Dominique Vincent [:domivinc]]

Attachment: patch-28032015 1-Bug_1127901___Zoomed_view_tool_bar_implementation___r_mcomella.patch

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.

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 39

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 40

[Dominique Vincent [:domivinc]]

Attachment: patch-03042015 1-Bug_1127901___Zoomed_view_tool_bar_implementation___r_mcomella.patch

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.

Comment 41

[Dominique Vincent [:domivinc]]

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.

Comment 42

[Dominique Vincent [:domivinc]]

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).

Comment 43

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 44

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Axel, we include some strings that are magnification levels, i.e. "x2", "x4", etc. Should these be localized?

Comment 45

[Francesco Lodolo [:flod]]

(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.

Comment 46

[Dominique Vincent [:domivinc]]

Attachment: 2015-05-01-percentage300.png

Comment 47

[Dominique Vincent [:domivinc]]

Attachment: 2015-05-01-percentage.png

Comment 48

[Francesco Lodolo [:flod]]

(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.

Comment 49

[Dominique Vincent [:domivinc]]

Attachment: patch-01052015 1-Bug_1127901___Zoomed_view_tool_bar_implementation___r_mcomella.patch

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?

Comment 50

[Dominique Vincent [:domivinc]]

Thanks Francesco, I will add the percentage in a localizable string if this design is confirmed.

Comment 51

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

%'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!

Comment 52

[Dominique Vincent [:domivinc]]

Last build available here: http://rusez.com/apk/fennec-40.0a1.en-US.android-arm.apk

Comment 53

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

Looks good!

Comment 54

[Dominique Vincent [:domivinc]]

Attachment: patch-02052015 1-Bug_1127901___Zoomed_view_tool_bar_implementation___r_mcomella.patch

Michael, I added the percentage in a localizable string.

Comment 55

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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).

Comment 56

[Francesco Lodolo [:flod]]

Also add that "%" is the percent sign, and why we have two of them (being encoded, I didn't expect it myself).

Comment 57

[Dominique Vincent [:domivinc]]

Attachment: patch-02052015 1-Bug_1127901___Zoomed_view_tool_bar_implementation___r_mcomella.patch

Localization note updated.

Comment 58

[Dominique Vincent [:domivinc]]

https://tbpl.mozilla.org/?tree=Try&rev=e908269f1991

Comment 59

[Dominique Vincent [:domivinc]]

Michael, all tests are green now.
In case of localization changes, is there something special to do before adding the "checkin-needed" keyword?

Comment 60

[Francesco Lodolo [:flod]]

(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.

Comment 61

[Dominique Vincent [:domivinc]]

(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 62

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d795db527abd

Comment 63

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/18d878c18fa9

Comment 64

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/42152093a51c

Comment 65

[Ryan VanderMeulen [:RyanVM]]

Comment 63 and 64 were investigating a crash that ended up being unrelated to this patch.

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42152093a51c

Comment 67

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97ad1fc3d5c7

Comment 68

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/97ad1fc3d5c7