1014113 - Find in page zoom is zooms too much

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect, P5)

Description

[:Margaret Leibovic]

There are some recent comments in bug 958111 complaining that the behavior added there is too aggressive. I think we should try to polish that interaction.

This feature is on beta now, but disabling it would just consist of removing one line in FindHelper.js, so maybe we should consider doing that if we don't think this is good enough to ship.

Comment 1

[Robin Ricard [:rricard]]

I think I'll take that one too, it's related to a part I'm used to work on.

Comment 2

[Robin Ricard [:rricard]]

But before going further into this bug I'd like to finish with 994724

Comment 3

[Robin Ricard [:rricard]]

Sorry for the mess here !

Comment 4

[:Margaret Leibovic]

Robin, FYI, I'm disabling this feature in bug 1014708, but we can re-enable it when we're ready to land a patch for this bug.

Comment 5

[Robin Ricard [:rricard]]

Okay, no problem for me ! I want to get all of that right.

Comment 6

[:Margaret Leibovic]

Let's also make sure to keep an eye out for bug 973909 as we work on this.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 8

[Robin Ricard [:rricard]]

Wow ! forgot about this one, working on it right now !

Comment 9

[Robin Ricard [:rricard]]

Attachment: Find in page zoom is zooms too much

Defines a viewport factor around the highlighted word
May need more testing (See bug 1015395)

Comment 10

[Robin Ricard [:rricard]]

From my manual tests it seems to work. I'll try to see if I can do the same thing with Robocop. After that I'll fire up some tries !

Comment 11

[:Margaret Leibovic]

Comment on attachment 8527183 [details] [diff] [review]
Find in page zoom is zooms too much

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

Looking good! I just have a few comments/questions.

::: mobile/android/chrome/content/FindHelper.js
@@ +130,5 @@
>          this._targetTab.setViewport(JSON.parse(this._initialViewport));
>          this._targetTab.sendViewportUpdate();
>        }
>      } else {
> +      const zoomFactor = 1/2;

I think it would be more intuitive to make the zoomFactor an integer, instead of inverting it where you're using it down below.

@@ +133,5 @@
>      } else {
> +      const zoomFactor = 1/2;
> +
> +      // we replace the start of the zoom rect to keep the highlighted word in the middle
> +      let x = aData.rect.x + (aData.rect.width * (1 - 1 / zoomFactor)) / 2;

Can you elaborate on the logic that went into this calculation? Where does the 2 come from? If you make the zoomFactor an integer like I suggested above, I think it would be more intutitive to write this as:

let x = aData.rect.x - (aData.rect.width * zoomFactor) / 2;

@@ +136,5 @@
> +      // we replace the start of the zoom rect to keep the highlighted word in the middle
> +      let x = aData.rect.x + (aData.rect.width * (1 - 1 / zoomFactor)) / 2;
> +      let y = aData.rect.y + (aData.rect.height * (1 - 1 / zoomFactor)) / 2;
> +
> +      let rect = new Rect(x > 0 ? x : 0,

You can just do Math.max(x, 0) here.

Comment 12

[Robin Ricard [:rricard]]

Attachment: Find in page zoom is zooms too much

Defines a viewport factor around the highlighted word
May need more testing (See bug 1015395)

Comment 13

[:Margaret Leibovic]

Comment on attachment 8530538 [details] [diff] [review]
Find in page zoom is zooms too much

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

This looks fine to me, but let's make sure we fix up the tests before we land this.

::: mobile/android/chrome/content/FindHelper.js
@@ +130,5 @@
>          this._targetTab.setViewport(JSON.parse(this._initialViewport));
>          this._targetTab.sendViewportUpdate();
>        }
>      } else {
> +      const spacingFactor = 2;

I think you should add a comment explaining what this factor means, and how changing it would affect the zoom behavior.

@@ +132,5 @@
>        }
>      } else {
> +      const spacingFactor = 2;
> +
> +      // we replace the start of the zoom rect to keep the highlighted word in the middle

Nit: Use sentence capitalization and punctuation in comments.

Comment 14

[Robin Ricard [:rricard]]

Attachment: Find in page zoom is zooms too much

Defines a viewport factor around the highlighted word
May need more testing (See bug 1015395)

Comment 15

[:Margaret Leibovic]

Comment on attachment 8531288 [details] [diff] [review]
Find in page zoom is zooms too much

This is looking good, but as I mentioned before, I want to see us update the tests before landing this.

Comment 16

[Mark Capella [:capella]]

For shorter search strings we zoom in quite a bit and lose surrounding context. I wonder if we can consider using a larger spacingFactor of say 6? 

Example compare spacingFactor 2 vs. 6

https://www.dropbox.com/s/s5aw76x2cu6yhd2/bug1014113_sf_compare.png?dl=0

Comment 17

[Mark Capella [:capella]]

Also, (sorry to jump in), with this patch there's an odd regression that should be looked into.

If you open the FindInPageBar view after visiting a page, and search for / type 'e' for example, then hit the backspace arrow to clear the search string, you wind up with a blank rendered page

https://www.dropbox.com/s/hjmru68drjmb7zx/bug1014113_blank.png?dl=0

And some messages in logcat like:

W/GeckoConsole(31782): [JavaScript Error: "SyntaxError: JSON.parse: unexpected character at line 1 column 179 of the JSON data" {file: "chrome://browser/content/browser.js" line: 1710}]
W/GeckoConsole(31782): [JavaScript Error: "SyntaxError: JSON.parse: unexpected character at line 1 column 9 of the JSON data" {file: "chrome://browser/content/browser.js" line: 1710}]

Comment 18

[Robin Ricard [:rricard]]

Hi ! Long time no see ! Sorry for that, I'm quite under lots of work/pressure/whatever right now but I'll try to finish this patch one day !

First of all, thanks for your feedback mark, I'll look into it (I agree 6 is better and this bug, well I'll try to figure out what's wrong there ... !)

Secondly, I'll see what's happening on the side of 1015395. We need better tests !

Comment 19

[Robin Ricard [:rricard]]

Attachment: Find in page zoom is zooms too much

Defines a viewport factor around the highlighted word
May need more testing (See bug 1015395)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a299390d38

Comment 20

[Robin Ricard [:rricard]]

Think I found what was wrong mark, thanks to WebIDE. It was sending NaNs to java. Of course, the viewport wasn't parsable in JSON (NaNs are not authorized there) so I had to check for the rect's size in the ZoomHelper. Now I abort the zoom if the rect has a null dimensions (or negative dimensions).

Comment 21

[Robin Ricard [:rricard]]

Attachment: Here's the patch with the fix !

Comment 22

[Mark Capella [:capella]]

Comment on attachment 8553800 [details] [diff] [review]
Here's the patch with the fix !

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

Oh, that was pretty much my feedback :p I trust you've tested it, and you're reviewer will catch the rest.

::: mobile/android/chrome/content/ZoomHelper.js
@@ +124,5 @@
>     */
>    zoomToRect: function(aRect, aClickY = -1) {
> +    if(aRect.width < 1 || aRect.height < 1) {
> +      // Protect from empty or negative-sized rects & potentials NaN in following calculations
> +      return;

Rect() has an isEmpty() function that might be prettier:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Geometry.jsm?mark=116-118#79

Comment 23

[Robin Ricard [:rricard]]

Yup ! Thanks, that'll be much better ! Updating it right away !

Comment 24

[Robin Ricard [:rricard]]

Attachment: Refactored with .isEmpty(). Thanks !

Comment 25

[Robin Ricard [:rricard]]

Firing up a try to be sure I'm not breaking anything...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c0f4d59ce99

Comment 26

[Mark Capella [:capella]]

Comment on attachment 8553815 [details] [diff] [review]
Refactored with .isEmpty(). Thanks !

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

lgtm :)

Comment 27

[:Margaret Leibovic]

Can you upload a build and needinfo antlam for UX feedback? I think capella's suggestion to change the zoom factor is good, but we should make sure our UX team approves this change before we land it.

Comment 28

[Robin Ricard [:rricard]]

Ok, I'm on it !

Comment 29

[Robin Ricard [:rricard]]

Comment on attachment 8553815 [details] [diff] [review]
Refactored with .isEmpty(). Thanks !

Restarted a try on this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4732445ad51

Comment 30

[Robin Ricard [:rricard]]

Attachment: Screenshot

Here's a screenshot. Factor is set to 6.

Comment 31

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

Comment on attachment 8553815 [details] [diff] [review]
Refactored with .isEmpty(). Thanks !

Hey Robin!

I'm not seeing a zoom right now on Nightly (Find in Page just seems to keep whatever zoom % the user was using). 

Could you upload a build (.APK) so I can do a compare more easily? I tried following the comments but this would be more thorough.

Also, is this just for Tablets or Mobile as well?

Thanks!

Comment 32

[Robin Ricard [:rricard]]

Hey !

Yep it has not landed yet but you can use the following build (from the try: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c4732445ad51/try-android-api-11/fennec-38.0a1.en-US.android-arm.apk)

It works on every screen size !

Comment 33

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

I'm having trouble installing it (probably name clash), can we change the build name so I don't have to delete the Nightly I'm currently using?

Thanks!

Comment 34

[Robin Ricard [:rricard]]

Here is a build from my computer: https://dl.dropboxusercontent.com/u/99291843/Bugzilla%20Attachments/fennec-38.0a1.en-US.android-arm.apk

Comment 35

[:Margaret Leibovic]

Comment on attachment 8553815 [details] [diff] [review]
Refactored with .isEmpty(). Thanks !

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

This looks good to me. Once we get approval from antlam we can check this in. And we should also fix bug 1128287.

Comment 36

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

(In reply to Robin Ricard [rricard] from comment #34)
> Here is a build from my computer:
> https://dl.dropboxusercontent.com/u/99291843/Bugzilla%20Attachments/fennec-
> 38.0a1.en-US.android-arm.apk

Hey Riccard, this works pretty well. I tested it on the Nexus 9 and the 6.

Thanks!

Comment 37

[:Margaret Leibovic]

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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32355c0d9d64

Comment 39

[Carsten Book [:Tomcat]]

backed this out since this caused a perma-failure rc2 like https://treeherder.mozilla.org/logviewer.html#?job_id=1998606&repo=fx-team

Comment 40

[Robin Ricard [:rricard]]

Oh ! I didn't expected this patch to create the problem so I wasn't testing the right thing locally ... I will investigate on this side ! Thanks for catching this !

Comment 41

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.