1126866 - Make the zoomed text always the same size

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 
I think Chrome makes the zoomed-in-upon text always the same size, no matter how big or small it is when tapped. This seems useful - I wonder if there's anything we can do here. For example, the top text on reddit is still awfully small (i.e. "front", "all", "random", etc.)


:domivinc
One possibility is to fix the zoomFactor value based on the clicked area by the user.
The font size of the cluster areas (clickable elements) could be used to calculate the zoomFactor.

The font size could be detected in the method GetClosest (layout/base/PositionedEventTargeting.cpp) when the number of elements in the cluster is incremented. The average value of the different font sizes should be sent to "browser.js".
In the class ZoomedView (mobile/android/base/ZoomedView.java), ZOOM_FACTOR should be replaced by a variable (zoomFactor), the value will be set just before the call to "startZoomDisplay" using data coming from the event "Gesture:clusteredLinksClicked".

Comment 1

[Dominique Vincent [:domivinc]]

Attachment: a-12022015 2-Bug_1126866___Fix_the_text_size_in_zoomed_view_r_kats.patch

Comment 2

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

Comment on attachment 8563593 [details] [diff] [review]
a-12022015 2-Bug_1126866___Fix_the_text_size_in_zoomed_view_r_kats.patch

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

Not sure this is my jurisdiction - kats?

::: mobile/android/base/ZoomedView.java
@@ +39,5 @@
>  public class ZoomedView extends FrameLayout implements LayerView.OnMetricsChangedListener,
>          LayerView.ZoomedViewListener, GeckoEventListener {
>      private static final String LOGTAG = "Gecko" + ZoomedView.class.getSimpleName();
>  
> +    private static final int ZOOM_FACTOR = 3;

nit: DEFAULT_ZOOM_FACTOR

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8563593 [details] [diff] [review]
a-12022015 2-Bug_1126866___Fix_the_text_size_in_zoomed_view_r_kats.patch

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

It is now!

Comment 4

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> It is now!

Why? I'm not sure I'm qualified to review coordinate code without a deep dive (which I probably shouldn't invest in).

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Considering you reviewed this code in bug 663803 I would hope you know it enough to qualify as a reviewer for this. And if you don't then I think spending some time on is a worthwhile investment. I barely touch fennec code anymore, and thus far most of my reviewing on this feature has been on the gecko side.

Anyway I guess you already gave it a f+ and it's been sitting for a while so I'll finish off the review but I'll expect to be able to push future reviews on the java-side stuff here to you or snorp.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8563593 [details] [diff] [review]
a-12022015 2-Bug_1126866___Fix_the_text_size_in_zoomed_view_r_kats.patch

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

::: mobile/android/base/ZoomedView.java
@@ +300,5 @@
>      private void setCapturedSize(ImmutableViewportMetrics metrics) {
>          float parentMinSize = Math.min(metrics.getWidth(), metrics.getHeight());
> +        // For metrics.zoomFactor lower than 1, the zoom factor of the zoomed view is calculated
> +        // to get always the same size for the content in the zoomed view.
> +        // For metrics.zoomFactor greater than 1, the zoom factor is always set to the default 

nit: trailing whitespace

@@ +302,5 @@
> +        // For metrics.zoomFactor lower than 1, the zoom factor of the zoomed view is calculated
> +        // to get always the same size for the content in the zoomed view.
> +        // For metrics.zoomFactor greater than 1, the zoom factor is always set to the default 
> +        // value ZOOM_FACTOR, thus the zoomed view is always a zoom of the normal view.
> +        zoomFactor = Math.max(ZOOM_FACTOR , (int) (ZOOM_FACTOR / metrics.zoomFactor));

nit: remove space before comma

@@ +307,5 @@
> +        viewWidth = (int) (parentMinSize * W_CAPTURED_VIEW_IN_PERCENT / (zoomFactor * 100.0)) * zoomFactor;
> +        viewHeight = (int) (parentMinSize * H_CAPTURED_VIEW_IN_PERCENT / (zoomFactor * 100.0)) * zoomFactor;
> +        // Display in zoomedview is corrupted when width is an odd number
> +        if ((viewWidth % 2) != 0) {
> +            viewWidth = viewWidth + 1;

This is likely the same thing that I was seeing in bug 776906 comment 11. The cairo backend that's used to draw the zoomed view to requires the stride to be a multiple of 4 which only happens if the width is even. This is fine, or you can do |viewWidth &= ~0x1;|. Either way, the comment is good and should stay :)

Comment 7

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Considering you reviewed this code in bug 663803 I would hope you know it
> enough to qualify as a reviewer for this.

Perhaps a miscommunication? I r+'d the java components sans the coordinate code, which I requested snorp look at (bug 663803 comment 119). I then thought snorp asked you to look at it (bug 663803 comment 135).

> And if you don't then I think
> spending some time on is a worthwhile investment. I barely touch fennec code
> anymore, and thus far most of my reviewing on this feature has been on the
> gecko side.

Then I agree, it sounds like it is more worthwhile for myself or snorp to learn.

> Anyway I guess you already gave it a f+ and it's been sitting for a while so
> I'll finish off the review but I'll expect to be able to push future reviews
> on the java-side stuff here to you or snorp.

Agreed - thanks.

Comment 8

[Dominique Vincent [:domivinc]]

Attachment: patch-21022015 2-Bug_1126866___Fix_the_text_size_in_zoomed_view_r_kats.patch

All changes done!

Comment 9

[Dominique Vincent [:domivinc]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/27dffa80e6dc