Closed Bug 1165127 Opened 6 years ago Closed 6 years ago

Zoomed View's purpose could be unintuitive to users

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: mcomella, Assigned: domivinc)

References

Details

Attachments

(2 files, 5 obsolete files)

When the view pops up, I imagine some users will say, "Wait! What just happened? Why do I have two windows?"

I think Chrome's implementation is more intuitive because it focuses on the two ambiguous links rather than whatever other content is in the page.

I think we could improve by one or all of:
  * Increasing the default zoom level (250%? 300%?)
  * Decreasing the size of the zoomed view to better focus on the clicked links
  * Providing an animation for displaying the zoomed view, zooming in on the ambiguous spot

Anthony, any thoughts?
Flags: needinfo?(alam)
No longer blocks: zoomedview
Depends on: zoomedview
(In reply to Michael Comella (:mcomella) from comment #0)
> When the view pops up, I imagine some users will say, "Wait! What just
> happened? Why do I have two windows?"

I agree, this could be a bit "weird" at first. But, there is a persistent "x" as well and so the impact is easily dismiss-able.

> I think Chrome's implementation is more intuitive because it focuses on the
> two ambiguous links rather than whatever other content is in the page.
> 
> I think we could improve by one or all of:
>   * Increasing the default zoom level (250%? 300%?)

I'm open to this. Though I'm not sure how we would determine this because it really depends on the device and the webpage the user is visiting. As this lands behind a Nightly flag, we can test it out and keep tabs on what feels best.

>   * Decreasing the size of the zoomed view to better focus on the clicked
> links

I think determining a fixed value of the "magnifying glass" will always be tricky. This depends a lot on the webpage in question, current zoomed ratio, as well as the eyes of the user. 

I think one benefit of our design is that it clearly allows users to move it around and interact with it. I do want to preserve this. Maybe there's a middle ground?

>   * Providing an animation for displaying the zoomed view, zooming in on the
> ambiguous spot

Animation. YES! I'll see if I can prototype something.

> Anthony, any thoughts?
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #1)
> > I think we could improve by one or all of:
> >   * Increasing the default zoom level (250%? 300%?)
> 
> I'm open to this. Though I'm not sure how we would determine this because it
> really depends on the device and the webpage the user is visiting.

Since I think the same size links will usually trigger the zoomed view for the user, it is specific to them: we can default to their last selected zoom value (also, is clicking on the percentages to change zoom level intuitive?).
Attached video mov_animatein.mov
Attaching animation mock.
Anthony, I implemented the animations based on your mock-up. You can test the apk here: 
http://rusez.com/apk/fennec-41.0a1.en-US.android-arm.apk
On my android device (2.3.3), the bounce effect at the end of the opening animation is not exactly the same used in your mock-up. The bounce seems to be more visible in my case. I used the default android bounce animation. I cannot test it on a 4.3 device to see if there is a difference.

There are 2 user cases not covered by your mock:

1- the user opens the zoomed view in a cluster of links and then move the zoomed view in another part. Where should be the final point of the closing animation in this case?

2- the user opens the zoomed view in a cluster of links and then clicks on another cluster in another part of the web page. The closing/opening animations should be done again or the zoomed view just moves to the correct position without animation?
Flags: needinfo?(alam)
Attachment #8613577 - Flags: review?(michael.l.comella)
Assignee: nobody → domivinc
(In reply to Dominique Vincent [:domivinc] from comment #4)
> On my android device (2.3.3), the bounce effect at the end of the opening
> animation is not exactly the same used in your mock-up. The bounce seems to
> be more visible in my case. I used the default android bounce animation. I
> cannot test it on a 4.3 device to see if there is a difference.

Yeah, it currently bounces way too much on open. We should try to get it closer to the mock if possible so it doesn't feel so "bouncy".

> There are 2 user cases not covered by your mock:
> 
> 1- the user opens the zoomed view in a cluster of links and then move the
> zoomed view in another part. Where should be the final point of the closing
> animation in this case?

Good point. I'd like to close towards the center of this zoomed view. I think that makes the most sense. 


> 2- the user opens the zoomed view in a cluster of links and then clicks on
> another cluster in another part of the web page. The closing/opening
> animations should be done again or the zoomed view just moves to the correct
> position without animation?

I've been thinking about this. I think pressing outside the "zoomed view" should simply dismiss/close the view. This way, we can keep the interaction model rather simple to begin with
Flags: needinfo?(alam) → needinfo?(domivinc)
Anthony, I'm going to work on the 2 first points. 

Regarding the last point ("clicking outside the zoomed view should close the view") we should not do it for different reasons:
- the user wants to interact with the view outside the zoomed view without closing it: to scroll down/up the page, to pause/start a video using a button outside the zoomed view, ... 
- if there is a click event on the body of the page, each time the user will close the zoomed view clicking outside, the body event will be sent and the actions linked will be done. Same issue if the user clicks outside the zoomed view but on a clickable area.

For all those reasons, we added the "X" button.
Flags: needinfo?(domivinc)
Attachment #8613577 - Flags: review?(michael.l.comella)
(In reply to Dominique Vincent [:domivinc] from comment #6)
> Anthony, I'm going to work on the 2 first points. 
> 
> Regarding the last point ("clicking outside the zoomed view should close the
> view") we should not do it for different reasons:
> - the user wants to interact with the view outside the zoomed view without
> closing it: to scroll down/up the page, to pause/start a video using a
> button outside the zoomed view, ... 

That's a fair point - I see what you're saying. 

Although, if the user wants to interact with an area outside this "zoomed view", I think we should then prioritize the UX for that action. We can adjust. I.e. at that moment, the "zoomed view" is not the focus, so, we should close it. If they need it again, it should automatically trigger anyways.

If they want to scroll (which I agree, could be common), they can scroll and if they're presented with more clustered links, this "zoomed view" triggers anyways. 

This cleans up our interaction model as well because 1) if they press a link, we load it, and 2) if they press an area without a link, we close the "zoomed view".

> - if there is a click event on the body of the page, each time the user will
> close the zoomed view clicking outside, the body event will be sent and the
> actions linked will be done. Same issue if the user clicks outside the
> zoomed view but on a clickable area.

I'm not too worried about this. Mainly, I'm unsure that we want to make every link outside the "zoomed view" an extra press away. 

Assuming that this "zoomed view" works automatically ("it just works"), I feel like it should also not be a hindrance to dismiss it (again, "it just works"). All that to say, triggering and dismissing should be just as easy because I still don't think it's common or beneficial to the browsing experience to keep this up more than it needs to be.
Flags: needinfo?(domivinc)
^ Given that, can we get a build that includes the third point ("clicking outside the zoomed view should close the view") as well? :)
(In reply to Anthony Lam (:antlam) from comment #7)

> Although, if the user wants to interact with an area outside this "zoomed
> view", I think we should then prioritize the UX for that action. We can
> adjust. I.e. at that moment, the "zoomed view" is not the focus, so, we
> should close it. If they need it again, it should automatically trigger
> anyways.
> 
> If they want to scroll (which I agree, could be common), they can scroll and
> if they're presented with more clustered links, this "zoomed view" triggers
> anyways. 
> 
> This cleans up our interaction model as well because 1) if they press a
> link, we load it, and 2) if they press an area without a link, we close the
> "zoomed view".

Reading this comment, I just realized that there are 2 different points of view regarding the zoomed view functionality.

A) Your way is to consider the zoomed view like a tool used only to help the user with clusters of links (it isn't the original design). The zoomed view is visible when the user scrolls on a cluster and is hidden when there is no more cluster.

B) The original way is to consider the zoomed view to help the user for any pages rendered with small fonts in a small screen (this design is here [1]). Actually the trigger to open the zoomed view is a click on a cluster of links. I would also prefer to get a way to open it whenever I want (probably based on the font size of the clicked area).
In the original design, the zoomed view was not only used in the cluster of links, but also for small input fields, or "+" buttons (see the picture here [2]).

The functionality A is a subset of B. I really prefer the B option covering all the cases and not only the clusters of links. If we really implement only the functionality A, we will have new change requests from the users to cover the small input fields and "+" buttons. 


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=663803#c27
[2] https://bug663803.bugzilla.mozilla.org/attachment.cgi?id=8394971
Flags: needinfo?(domivinc)
My apologies, I should've been more clear. 

I am not suggesting a split of any sort in the design of this feature. I don't think that we're at a point of separating out the two use cases and having to decide which one to implement. I agree with you, this should also show up for small input fields, etc... whenever the tap targets are "too small", it should show up. 

But to keep in the scope of this bug, let's focus on getting the animation right (animating in as well as out). The goal here is still to help ease the user into this feature (even on-boarding them of sorts) through a simple animation.

I'd be more than happy to discuss this in another bug so that we can keep this within the original scope. :)
Flags: needinfo?(domivinc)
Anthony, I didn't get enough free time today to complete the 2 animations changes. I'm reasonably confident that I will have a new version of the APK tomorrow.
Flags: needinfo?(domivinc)
No worries Domivinc! let me know when I can test a build. Thanks!
A new version of the apk can be tested here: 
http://rusez.com/apk/fennec-41.0a1.en-US.android-arm.apk

It includes the animations changes.
Attachment #8613577 - Attachment is obsolete: true
Flags: needinfo?(alam)
Attachment #8615980 - Flags: review?(michael.l.comella)
Hey domivinc,

Two pieces of feedback:

1) can we get the animation to be 250 ms? it seems too slow right now
2) lets get that first "overshoot" when the magnifying glass opens to be smaller so it feels slightly less bouncy.

Do you have the values somewhere so I can better see what we're manipulating? That might help me give you better values too.
Flags: needinfo?(alam) → needinfo?(domivinc)
Anthony, how did you make your mock-up video posted in comment 3? You probably added an animation with different parameters in the tool used to make the video. You just have to post your design specifications and I can try to match the parameters of your mock-up with the Android animations interpolators parameters. 

If you really want to investigate the Android animations Interpolators, there is a good tutorial here [1]. 

From a more general point of view, do you know if there is a Mozilla documentation somewhere about communication between designers and developers? Is there a document about good practices? 
Based on my experiences in different companies, I find useful to get a mock-up with at least 5 lines of text about the key parameters of the mock-up (not a full document: just 5 lines). But I can also work without the "5 lines". It's just less efficient from my point of view (we are probably facing the communication issues described in this good article here [2]).


The different animations parameters tested so far are available hereunder:

A) The Bounce Interpolator was used in the APK and code provided in comment 4.

B) The last APK you tested, used an Overshoot Interpolator with a tension of 1.8 . The opening animation delay was 800ms, the closing delay was 500ms (backup copy of the APK available here [3]).

C) The new APK [4] is using the following parameters:
- Overshoot Interpolator with a tension of 1.5 ,
- both animation delays set to 250ms.


[1] http://cogitolearning.co.uk/?p=1078
[2] http://www.smashingmagazine.com/2014/10/09/front-end-development-ode-to-specifications/
[3] http://rusez.com/apk/fennec-41.0a1.en-US.android-arm.Interpolator1.8.apk
[4] http://rusez.com/apk/fennec-41.0a1.en-US.android-arm.apk
Attachment #8615980 - Attachment is obsolete: true
Attachment #8615980 - Flags: review?(michael.l.comella)
Flags: needinfo?(domivinc) → needinfo?(alam)
Attachment #8616359 - Flags: review?(michael.l.comella)
(In reply to Dominique Vincent [:domivinc] from comment #15)
> Created attachment 8616359 [details] [diff] [review]
> patch-06062015
> 1-
> Bug_1165127___Animations_for_displaying_closing_the_zoomed_view___r_mcomella.
> patch
> 
> Anthony, how did you make your mock-up video posted in comment 3? You
> probably added an animation with different parameters in the tool used to
> make the video. You just have to post your design specifications and I can
> try to match the parameters of your mock-up with the Android animations
> interpolators parameters. 
> 
> If you really want to investigate the Android animations Interpolators,
> there is a good tutorial here [1]. 

Domivinc, thanks for posting this!

I've mocked this up in FramerJS and so the values do not exactly translate - that is why I ask. I also sometimes use other tools like Quartz composer and unfortunately there is no easy way to translate those either. 

> From a more general point of view, do you know if there is a Mozilla
> documentation somewhere about communication between designers and
> developers? Is there a document about good practices? 
> Based on my experiences in different companies, I find useful to get a
> mock-up with at least 5 lines of text about the key parameters of the
> mock-up (not a full document: just 5 lines). But I can also work without the
> "5 lines". It's just less efficient from my point of view (we are probably
> facing the communication issues described in this good article here [2]).
 
I understand your frustration here. This communication is often very dependent on the developer and designer in question. It varies quite a bit from person to person. 

As I mentioned in the beginning, it's going to be an iterative process. To be honest, I prefer the back and forth  as feedback and iteration are important. In the end, I'm in this to build out the best possible experience for fennec with you - together. 
 
> The different animations parameters tested so far are available hereunder:
> 
> A) The Bounce Interpolator was used in the APK and code provided in comment
> 4.
> 
> B) The last APK you tested, used an Overshoot Interpolator with a tension of
> 1.8 . The opening animation delay was 800ms, the closing delay was 500ms
> (backup copy of the APK available here [3]).
> 
> C) The new APK [4] is using the following parameters:
> - Overshoot Interpolator with a tension of 1.5 ,
> - both animation delays set to 250ms.

Awesome! I've tested this out on my N6. It's getting really close! I think the animation great on "open". But there seems to be a "delay" before this triggers. That makes it feel a bit disjointed for me. 

Two points of feedback:

Are the "animation delays" you're referring to doing this or does that value pertain to the animation duration? I'd like there to be no delays if possible.

Can we speed up the closing animation duration? It seems slower than 250 ms. I'd really like to tryi 150 ms to make it feel more snappy :)

Thanks Domivinc! we're so close!
Flags: needinfo?(alam) → needinfo?(domivinc)
Anthony, you’re right! If I’m talking about the animation, it’s an animation “duration” and not a “delay”. I should use the term “delay” talking about the time to get the zoomed view open.

I made the change: closing animation duration is now 150 ms. (no change on opening: 250ms).
New APK available here:
http://rusez.com/apk/fennec-41.0a1.en-US.android-arm.apk


My 2 cents about the iterative process:
The iterative process works very well with css/jscript/php languages to build a web application for instance. The designer and developer can work in a live environment with a very small duration for each iteration (15 s.). I used to chat with the designer asking me changes, 15s later the designer could see and test the changes in the web application. In this case, we worked very efficiently, exploring many design changes for a very low cost on both side (designer / developer).

But with an application like Fennec (C++/Java/android) the iterative process will never work very well. On the designer side, the time to test a new change is still very low (download and install the new apk on a device: 30 s.). But on the developer side the time to build and download to a server the new version of the Fennec application is very high: 10 to 15 minutes! 
Sometimes in my other Android applications development, when I knew that the number of iterations would probably be very high to get the final version, I moved the different parameters (color, position, animation duration, …) in preferences variables and the designer could play with the values without requiring a new version of the apk. There is a cost to move temporary the parameters in preference values, but after 2 or 3 iterations, it’s worth it.
Attachment #8616359 - Attachment is obsolete: true
Attachment #8616359 - Flags: review?(michael.l.comella)
Flags: needinfo?(domivinc) → needinfo?(alam)
Attachment #8616575 - Flags: review?(michael.l.comella)
Thanks Domivinc! that build feels great to me - ship it!

Great points about processes. I'd be happy to continue this conversation through email once this thread closes. I think you made some interesting points and I'm definitely interested in how the process you're describing got us to this final product.
Flags: needinfo?(alam)
Comment on attachment 8616575 [details] [diff] [review]
patch-08062015 1-Bug_1165127___Animations_for_displaying_closing_the_zoomed_view___r_mcomella.patch

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

This animation is slick!

Just a few readability changes.

::: mobile/android/base/ZoomedView.java
@@ +71,5 @@
>      private int containterSize; // shadow, margin, ...
>      private Point lastPosition;
>      private boolean shouldSetVisibleOnUpdate;
>      private PointF returnValue;
> +    private PointF animationStart;

nit: final

@@ +258,5 @@
> +                changeZoomFactor();
> +            }
> +        });
> +
> +        setOnTouchListener(new ZoomedViewTouchListener());

nit: I'd prefer to keep the ZoomedViewTouchListener reference around as a member variable.

@@ +343,5 @@
>          return returnValue;
>      }
>  
> +    private void moveZoomedView(ImmutableViewportMetrics metrics, float newLeftMargin, float newTopMargin,
> +            boolean updateAnimateStartPoint, boolean centerAnimateStartPoint) {

It'd probably be better to pass in one enum (e.g. StartPointUpdate.CENTER) flag rather than two mutually exclusive boolean flags.

Alternatively, I don't see many cases where moveZoomedView should be called and we shouldn't update the animation start (I only see refreshZoomedViewSize and that happens infrequently enough that we can just do the calculation) - perhaps we should just have a default update (center?) and use the other method when the caller specifies we should.

@@ +376,5 @@
>  
> +        if (updateAnimateStartPoint) {
> +            // Before this point, the animationStart point is relative to the layerView.
> +            // The position of the zoomed view is now calculated, so the position of the animation
> +            // can now be correctly set relative to the zoomed view

When you say it can be set relative to the zoomed view, you mean we're using animationStart because animationStart has already been set in startZoomDisplay using the zoomed view position, right? Can you add this information to this comment?

@@ +478,5 @@
>              setCapturedSize(metrics);
>          }
>          startTimeReRender = 0;
>          shouldSetVisibleOnUpdate = true;
> +        ImmutableViewportMetrics metrics = layerView.getViewportMetrics();

nit: whitespace above and below this block.
Attachment #8616575 - Flags: review?(michael.l.comella) → review+
Michael, I used an enum flag for the animation start point update with 3 different states. The animation start position must not be changed by the refreshZoomedViewSize call. This method is called in many different cases.
Attachment #8616575 - Attachment is obsolete: true
Attachment #8620943 - Flags: review?(michael.l.comella)
Comment on attachment 8620943 [details] [diff] [review]
patch-11062015 1-Bug_1165127___Animations_for_displaying_closing_the_zoomed_view___r_mcomella.patch

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

WFM w/ the allocation change.

::: mobile/android/base/ZoomedView.java
@@ +264,5 @@
> +                changeZoomFactor();
> +            }
> +        });
> +
> +        touchListener = new ZoomedViewTouchListener();

My idea with this is that the allocation wouldn't happen everytime the ZoomedView is shown (e.g. move it to the constructor, or lazily instantiate it here).
Attachment #8620943 - Flags: review?(michael.l.comella) → review+
touchListener allocation is now in the constructor.
Attachment #8620943 - Attachment is obsolete: true
Michael, RC5 test failed several times. All the RC5 tests were red yesterday, but I also re-tested today. The situation looks better today for the other jobs. Do I need to make another "hg push" to try server in order to pass it?
Flags: needinfo?(michael.l.comella)
RC5 is now green.
Flags: needinfo?(michael.l.comella)
Keywords: checkin-needed
(In reply to Dominique Vincent [:domivinc] from comment #24)
> Michael, RC5 test failed several times. All the RC5 tests were red
> yesterday, but I also re-tested today. The situation looks better today for
> the other jobs. Do I need to make another "hg push" to try server in order
> to pass it?

We had some issues w/ the test suite: https://bugzilla.mozilla.org/show_bug.cgi?id=1171789
https://hg.mozilla.org/mozilla-central/rev/05e42bcd0d77
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.