958111 - "Find in Page" on Android should zoom in on the highlighted result

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, enhancement)

Description

[Matt Brubeck (:mbrubeck)]

If the page is currently zoomed out a lot, we zoom in to make the selected "Find in page" result more readable, like we did in old versions of Fennec here:
http://hg.mozilla.org/mozilla-central/file/cbeab7da0e3a/mobile/xul/chrome/content/common-ui.js#l274

This should be relatively easy now that bug 916481 and bug 958101 added new code to the Finder.jsm "onFindResult" API that returns the bounding rect of the selected result.  FindHelper.onFindResult will receive the rect as one of its parameters here:
http://hg.mozilla.org/mozilla-central/file/711f08b0aa1d/mobile/android/chrome/content/FindHelper.js#l68

...and it can use this to send a "ZoomToRect" message like the one we send here (we might want to factor this code out into a helper function):
http://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4627

Comment 1

[Robin Ricard [:rricard]]

I could take a look on it and it could be a good opportunity to take my hands on the project if noone is interested in doing it.

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to ricard.robin from comment #1)
> I could take a look on it and it could be a good opportunity to take my
> hands on the project if noone is interested in doing it.

Great!  https://wiki.mozilla.org/Mobile/Get_Involved has more information that should help you get started, and please don't hesitate to ask (in this bug, or by email or IRC) if you have any questions.

Comment 3

[Robin Ricard [:rricard]]

I'm currently stuck because the BrowserEventHandler manages everything related to the zoom. I can't actually access it from the FindHelper (or I don't know how). I could detach all zoom-related actions in an another file such as ZoomHelper.js but I don't know if it'll be worth it.

Comment 4

[Matt Brubeck (:mbrubeck)]

It's probably simplest to just call sendMessageToJava directly from FindHelper.onFindResult, something like this:

  sendMessageToJava({
    type: "Browser:ZoomToRect",
    x: aData.rect.x,
    y: aData.rect.y,
    w: aData.rect.width,
    h: aData.rect.height,
  });

Comment 5

[Robin Ricard [:rricard]]

Ok, I'm working on it. Thanks !

Comment 6

[Robin Ricard [:rricard]]

Ok, you actually fixed the bug. But now that I start to understand how things are working around here, I would like to make it more pertinent. 
The zoom is actually too heavy ...
I would like to put a small algorithm to get it work only when the user need it (ie. the text is too small) and when he needs it, don't zoom too much.

Comment 7

[Robin Ricard [:rricard]]

diff --git a/mobile/android/chrome/content/FindHelper.js b/mobile/android/chrome/content/FindHelper.js
--- a/mobile/android/chrome/content/FindHelper.js
+++ b/mobile/android/chrome/content/FindHelper.js
@@ -73,11 +73,18 @@ var FindHelper = {
           Cu.reportError("Warning: selected tab changed during find!");
           // fall through and restore viewport on the initial tab anyway
         }
         this._targetTab.setViewport(JSON.parse(this._initialViewport));
         this._targetTab.sendViewportUpdate();
       }
     } else {
       this._viewportChanged = true;
+      sendMessageToJava({
+        type: "Browser:ZoomToRect",
+        x: aData.rect.x,
+        y: aData.rect.y,
+        w: aData.rect.width,
+        h: aData.rect.height,
+      });
     }
   }
 };

Comment 8

[Robin Ricard [:rricard]]

Okay, I'll wrap it up in mercurial tomorrow but I refactored many things ... But it works !

diff --git a/mobile/android/chrome/content/FindHelper.js b/mobile/android/chrome/content/FindHelper.js
--- a/mobile/android/chrome/content/FindHelper.js
+++ b/mobile/android/chrome/content/FindHelper.js
@@ -72,12 +72,18 @@ var FindHelper = {
           // this should never happen
           Cu.reportError("Warning: selected tab changed during find!");
           // fall through and restore viewport on the initial tab anyway
         }
         this._targetTab.setViewport(JSON.parse(this._initialViewport));
         this._targetTab.sendViewportUpdate();
       }
     } else {
+      var rect = {};
+      rect.h = aData.rect.height;
+      rect.w = aData.rect.width;
+      rect.x = aData.rect.x;
+      rect.y = aData.rect.y;
+      ZoomHelper.zoomToRect(rect, -1, false, true);
       this._viewportChanged = true;
     }
   }
 };
diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
--- a/mobile/android/chrome/content/browser.js
+++ b/mobile/android/chrome/content/browser.js
@@ -79,16 +79,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 [
   ["SelectHelper", "chrome://browser/content/SelectHelper.js"],
   ["InputWidgetHelper", "chrome://browser/content/InputWidgetHelper.js"],
   ["AboutReader", "chrome://browser/content/aboutReader.js"],
   ["MasterPassword", "chrome://browser/content/MasterPassword.js"],
   ["PluginHelper", "chrome://browser/content/PluginHelper.js"],
   ["OfflineApps", "chrome://browser/content/OfflineApps.js"],
   ["Linkifier", "chrome://browser/content/Linkify.js"],
+  ["ZoomHelper", "chrome://browser/content/ZoomHelper.js"],
 ].forEach(function (aScript) {
   let [name, script] = aScript;
   XPCOMUtils.defineLazyGetter(window, name, function() {
     let sandbox = {};
     Services.scriptloader.loadSubScript(script, sandbox);
     return sandbox[name];
   });
 });
@@ -198,17 +199,17 @@ function doChangeMaxLineBoxWidth(aWidth)
     range = BrowserApp.selectedTab._mReflozPoint.range;
   }
 
   try {
     docViewer.pausePainting();
     docViewer.changeMaxLineBoxWidth(aWidth);
 
     if (range) {
-      BrowserEventHandler._zoomInAndSnapToRange(range);
+      ZoomHelper.zoomInAndSnapToRange(range);
     } else {
       // In this case, we actually didn't zoom into a specific range. It
       // probably happened from a page load reflow-on-zoom event, so we
       // need to make sure painting is re-enabled.
       BrowserApp.selectedTab.clearReflowOnZoomPendingActions();
     }
   } finally {
     docViewer.resumePainting();
@@ -1375,18 +1376,18 @@ var BrowserApp = {
       return;
 
     let focused = this.getFocusedInput(aBrowser);
 
     if (focused) {
       let shouldZoom = Services.prefs.getBoolPref("formhelper.autozoom");
       if (formHelperMode == kFormHelperModeDynamic && this.isTablet)
         shouldZoom = false;
-       // _zoomToElement will handle not sending any message if this input is already mostly filling the screen
-      BrowserEventHandler._zoomToElement(focused, -1, false,
+       // ZoomHelper.zoomToElement will handle not sending any message if this input is already mostly filling the screen
+      ZoomHelper.zoomToElement(focused, -1, false,
           aAllowZoom && shouldZoom && !ViewportHandler.getViewportMetadata(aBrowser.contentWindow).isSpecified);
     }
   },
 
   observe: function(aSubject, aTopic, aData) {
     let browser = this.selectedBrowser;
 
     switch (aTopic) {
@@ -2924,26 +2925,26 @@ Tab.prototype = {
    *    painting.
    * 2. During the next call to setViewport(), which is in the Tab prototype,
    *    we detect that a call to changeMaxLineBoxWidth should be performed. If
    *    we're zooming out, then the max line box width should be reset at this
    *    time. Otherwise, we call performReflowOnZoom.
    *   2a. PerformReflowOnZoom() and resetMaxLineBoxWidth() schedule a call to
    *       doChangeMaxLineBoxWidth, based on a timeout specified in preferences.
    * 3. doChangeMaxLineBoxWidth changes the line box width (which also
-   *    schedules a reflow event), and then calls _zoomInAndSnapToRange.
-   * 4. _zoomInAndSnapToRange performs the positioning of reflow-on-zoom and
-   *    then re-enables painting.
+   *    schedules a reflow event), and then calls ZoomHelper.zoomInAndSnapToRange.
+   * 4. ZoomHelper.zoomInAndSnapToRange performs the positioning of reflow-on-zoom
+   *    and then re-enables painting.
    *
    * Some of the events happen synchronously, while others happen asynchronously.
    * The following is a rough sketch of the progression of events:
    *
    * double tap event seen -> onDoubleTap() -> ... asynchronous ...
    *   -> setViewport() -> performReflowOnZoom() -> ... asynchronous ...
-   *   -> doChangeMaxLineBoxWidth() -> _zoomInAndSnapToRange()
+   *   -> doChangeMaxLineBoxWidth() -> ZoomHelper.zoomInAndSnapToRange()
    *   -> ... asynchronous ... -> setViewport() -> Observe('after-viewport-change')
    *   -> resumePainting()
    */
   performReflowOnZoom: function(aViewport) {
     let zoom = this._drawZoom ? this._drawZoom : aViewport.zoom;
 
     let viewportWidth = gScreenWidth / zoom;
     let reflozTimeout = Services.prefs.getIntPref("browser.zoom.reflowZoom.reflowTimeout");
@@ -4570,51 +4571,16 @@ var BrowserEventHandler = {
         break;
 
       default:
         dump('BrowserEventHandler.handleUserEvent: unexpected topic "' + aTopic + '"');
         break;
     }
   },
 
-  _zoomOut: function() {
-    BrowserEventHandler.resetMaxLineBoxWidth();
-    sendMessageToJava({ type: "Browser:ZoomToPageWidth" });
-  },
-
-  _isRectZoomedIn: function(aRect, aViewport) {
-    // This function checks to see if the area of the rect visible in the
-    // viewport (i.e. the "overlapArea" variable below) is approximately
-    // the max area of the rect we can show. It also checks that the rect
-    // is actually on-screen by testing the left and right edges of the rect.
-    // In effect, this tells us whether or not zooming in to this rect
-    // will significantly change what the user is seeing.
-    const minDifference = -20;
-    const maxDifference = 20;
-    const maxZoomAllowed = 4; // keep this in sync with mobile/android/base/ui/PanZoomController.MAX_ZOOM
-
-    let vRect = new Rect(aViewport.cssX, aViewport.cssY, aViewport.cssWidth, aViewport.cssHeight);
-    let overlap = vRect.intersect(aRect);
-    let overlapArea = overlap.width * overlap.height;
-    let availHeight = Math.min(aRect.width * vRect.height / vRect.width, aRect.height);
-    let showing = overlapArea / (aRect.width * availHeight);
-    let dw = (aRect.width - vRect.width);
-    let dx = (aRect.x - vRect.x);
-
-    if (fuzzyEquals(aViewport.zoom, maxZoomAllowed) && overlap.width / aRect.width > 0.9) {
-      // we're already at the max zoom and the block is not spilling off the side of the screen so that even
-      // if the block isn't taking up most of the viewport we can't pan/zoom in any more. return true so that we zoom out
-      return true;
-    }
-
-    return (showing > 0.9 &&
-            dx > minDifference && dx < maxDifference &&
-            dw > minDifference && dw < maxDifference);
-  },
-
   onDoubleTap: function(aData) {
     let data = JSON.parse(aData);
     let element = ElementTouchHelper.anyElementFromPoint(data.x, data.y);
 
     // We only want to do this if reflow-on-zoom is enabled, we don't already
     // have a reflow-on-zoom event pending, and the element upon which the user
     // double-tapped isn't of a type we want to avoid reflow-on-zoom.
     if (BrowserEventHandler.mReflozPref &&
@@ -4635,27 +4601,27 @@ var BrowserEventHandler = {
       let docShell = webNav.QueryInterface(Ci.nsIDocShell);
       let docViewer = docShell.contentViewer.QueryInterface(Ci.nsIMarkupDocumentViewer);
       docViewer.pausePainting();
 
       BrowserApp.selectedTab.probablyNeedRefloz = true;
     }
 
     if (!element) {
-      this._zoomOut();
+      ZoomHelper.zoomOut();
       return;
     }
 
     while (element && !this._shouldZoomToElement(element))
       element = element.parentNode;
 
     if (!element) {
-      this._zoomOut();
+      ZoomHelper.zoomOut();
     } else {
-      this._zoomToElement(element, data.y);
+      ZoomHelper.zoomToElement(element, data.y);
     }
   },
 
   /**
    * Determine if reflow-on-zoom functionality should be suppressed, given a
    * particular element. Double-tapping on the following elements suppresses
    * reflow-on-zoom:
    *
@@ -4671,111 +4637,17 @@ var BrowserEventHandler = {
         aElement instanceof Ci.nsIDOMHTMLMediaElement ||
         aElement instanceof Ci.nsIDOMHTMLPreElement) {
       return true;
     }
 
     return false;
   },
 
-  /* Zoom to an element, optionally keeping a particular part of it
-   * in view if it is really tall.
-   */
-  _zoomToElement: function(aElement, aClickY = -1, aCanZoomOut = true, aCanScrollHorizontally = true) {
-    const margin = 15;
-    let rect = ElementTouchHelper.getBoundingContentRect(aElement);
-
-    let viewport = BrowserApp.selectedTab.getViewport();
-    let bRect = new Rect(aCanScrollHorizontally ? Math.max(viewport.cssPageLeft, rect.x - margin) : viewport.cssX,
-                         rect.y,
-                         aCanScrollHorizontally ? rect.w + 2 * margin : viewport.cssWidth,
-                         rect.h);
-    // constrict the rect to the screen's right edge
-    bRect.width = Math.min(bRect.width, viewport.cssPageRight - bRect.x);
-
-    // if the rect is already taking up most of the visible area and is stretching the
-    // width of the page, then we want to zoom out instead.
-    if (BrowserEventHandler.mReflozPref) {
-      let zoomFactor = BrowserApp.selectedTab.getZoomToMinFontSize(aElement);
-
-      bRect.width = zoomFactor <= 1.0 ? bRect.width : gScreenWidth / zoomFactor;
-      bRect.height = zoomFactor <= 1.0 ? bRect.height : bRect.height / zoomFactor;
-      if (zoomFactor == 1.0 || this._isRectZoomedIn(bRect, viewport)) {
-        if (aCanZoomOut) {
-          this._zoomOut();
-        }
-        return;
-      }
-    } else if (this._isRectZoomedIn(bRect, viewport)) {
-      if (aCanZoomOut) {
-        this._zoomOut();
-      }
-      return;
-    }
-
-    rect.type = "Browser:ZoomToRect";
-    rect.x = bRect.x;
-    rect.y = bRect.y;
-    rect.w = bRect.width;
-    rect.h = Math.min(bRect.width * viewport.cssHeight / viewport.cssWidth, bRect.height);
-
-    if (aClickY >= 0) {
-      // if the block we're zooming to is really tall, and we want to keep a particular
-      // part of it in view, then adjust the y-coordinate of the target rect accordingly.
-      // the 1.2 multiplier is just a little fuzz to compensate for bRect including horizontal
-      // margins but not vertical ones.
-      let cssTapY = viewport.cssY + aClickY;
-      if ((bRect.height > rect.h) && (cssTapY > rect.y + (rect.h * 1.2))) {
-        rect.y = cssTapY - (rect.h / 2);
-      }
-    }
-
-    if (rect.w > viewport.cssWidth || rect.h > viewport.cssHeight) {
-      BrowserEventHandler.resetMaxLineBoxWidth();
-    }
-
-    sendMessageToJava(rect);
-  },
-
-  _zoomInAndSnapToRange: function(aRange) {
-    // aRange is always non-null here, since a check happened previously.
-    let viewport = BrowserApp.selectedTab.getViewport();
-    let fudge = 15; // Add a bit of fudge.
-    let boundingElement = aRange.offsetNode;
-    while (!boundingElement.getBoundingClientRect && boundingElement.parentNode) {
-      boundingElement = boundingElement.parentNode;
-    }
-
-    let rect = ElementTouchHelper.getBoundingContentRect(boundingElement);
-    let drRect = aRange.getClientRect();
-    let scrollTop =
-      BrowserApp.selectedBrowser.contentDocument.documentElement.scrollTop ||
-      BrowserApp.selectedBrowser.contentDocument.body.scrollTop;
-
-    // We subtract half the height of the viewport so that we can (ideally)
-    // center the area of interest on the screen.
-    let topPos = scrollTop + drRect.top - (viewport.cssHeight / 2.0);
-
-    // Factor in the border and padding
-    let boundingStyle = window.getComputedStyle(boundingElement);
-    let leftAdjustment = parseInt(boundingStyle.paddingLeft) +
-                         parseInt(boundingStyle.borderLeftWidth);
-
-    BrowserApp.selectedTab._mReflozPositioned = true;
-
-    rect.type = "Browser:ZoomToRect";
-    rect.x = Math.max(viewport.cssPageLeft, rect.x  - fudge + leftAdjustment);
-    rect.y = Math.max(topPos, viewport.cssPageTop);
-    rect.w = viewport.cssWidth;
-    rect.h = viewport.cssHeight;
-    rect.animate = false;
-
-    sendMessageToJava(rect);
-    BrowserApp.selectedTab._mReflozPoint = null;
-  },
+  
 
   onPinchFinish: function(aData) {
     let data = {};
     try {
       data = JSON.parse(aData);
     } catch(ex) {
       console.log(ex);
       return;
diff --git a/mobile/android/chrome/jar.mn b/mobile/android/chrome/jar.mn
--- a/mobile/android/chrome/jar.mn
+++ b/mobile/android/chrome/jar.mn
@@ -48,16 +48,17 @@ chrome.jar:
   content/PluginHelper.js              (content/PluginHelper.js)
   content/OfflineApps.js               (content/OfflineApps.js)
   content/MasterPassword.js            (content/MasterPassword.js)
   content/FindHelper.js                (content/FindHelper.js)
   content/PermissionsHelper.js         (content/PermissionsHelper.js)
   content/FeedHandler.js               (content/FeedHandler.js)
   content/Feedback.js                  (content/Feedback.js)
   content/Linkify.js                   (content/Linkify.js)
+  content/ZoomHelper.js                (content/ZoomHelper.js)
 #ifdef MOZ_SERVICES_HEALTHREPORT
   content/aboutHealthReport.xhtml      (content/aboutHealthReport.xhtml)
 * content/aboutHealthReport.js         (content/aboutHealthReport.js)
 #endif
   content/aboutAccounts.xhtml          (content/aboutAccounts.xhtml)
 * content/aboutAccounts.js             (content/aboutAccounts.js)
 
 % content branding %content/branding/

Comment 9

[Matt Brubeck (:mbrubeck)]

Great!  When you get your patch ready, please attach the final version to this bug as an attachment:
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

Comment 10

[Robin Ricard [:rricard]]

Ok, I'll send the patch but you have to know that I don't know where is the place where I can add some tests for my feature ...

Comment 11

[Robin Ricard [:rricard]]

Attachment: 958111.patch

Ok, this patch alone won't work. I didn't got how mq works and moreover I'm used to git, and there's no rebase in mercurial ...
So, sorry for the double patch ...

Comment 12

[Robin Ricard [:rricard]]

Attachment: 958111-1.patch

Provides a second patch ... following the first one ...

Comment 13

[Matt Brubeck (:mbrubeck)]

(In reply to ricard.robin from comment #10)
> Ok, I'll send the patch but you have to know that I don't know where is the
> place where I can add some tests for my feature ...

I believe you can use a 'robocop' test for this feature:
https://wiki.mozilla.org/Auto-tools/Projects/Robocop/WritingTests

(In reply to ricard.robin from comment #11)
> So, sorry for the double patch ...

After enabling the 'mq' extension, you can use "hg qimport -r tip; hg qpop" to move your latest commit from your history into your patch queue, then repeat for the previous commit.  Then you can "hg qpush" the first patch and then "hg qfold" the second patch to combine it with the first.

Comment 14

[Robin Ricard [:rricard]]

Thanks I'll look to it soon !

Comment 15

[Robin Ricard [:rricard]]

Attachment: 958111.patch

The patch with the whole implementation and the tests

Comment 16

[Robin Ricard [:rricard]]

Finally I made it ! I think It'll probably not pass in the first place but I'm open to your review !

Comment 17

[Robin Ricard [:rricard]]

If you've got some feedback or any news it would be great. Anyway thanks for your help ! It is a nice "first bug" !

Comment 18

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8362708 [details] [diff] [review]
958111.patch

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

Thanks!  As soon as I have time I'll check the patch and let you know if I have any questions or comments.  If it appears ready, I'll pass it along to one of the Firefox for Android team members to review.  Once they approve it, we'll check it in and ship it!

Comment 19

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8362708 [details] [diff] [review]
958111.patch

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

This looks good!  Thanks for factoring out the zooming code into a new module; that's a nice improvement.

I commented below with some minor style and organization issues.  After addressing those issues, could you please attach a new version of your patch to this bug, set the "review" flag to "?" on the attachment page, and enter ":margaret" in the field next to the review flag?  Thanks!  And as always, let me know if you have any questions.

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

Nit: We use "let" instead of "var" because it allows more control over scope.

@@ +81,5 @@
> +      rect.h = aData.rect.height;
> +      rect.w = aData.rect.width;
> +      rect.x = aData.rect.x;
> +      rect.y = aData.rect.y;
> +      ZoomHelper.zoomToRect(rect, -1, false, true);

I think we should make zoomToRect accept standard rect objects with "width" and "height" instead of "w" and "h", so we could just pass the original aData.rect instead of copying its properties into a new object.

::: mobile/android/chrome/content/ZoomHelper.js
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +var ZoomHelper = {
> +	zoomInAndSnapToRange: function(aRange) {

Nit: Indentation should use 2 'space' characters instead of tabs.  (We can easily fix this when landing the patch, if needed.)

@@ +85,5 @@
> +	},
> +	
> +	zoomToRect: function(aRect, aClickY = -1, aCanZoomOut = true, aCanScrollHorizontally = true) {
> +		const margin = 15;
> +		var rect = aRect;

Please just use aRect throughout, instead of creating this alias, for simplicity.

@@ +118,5 @@
> +	
> +		rect.type = "Browser:ZoomToRect";
> +		rect.x = bRect.x;
> +		rect.y = bRect.y;
> +		rect.w = bRect.width;

Here would be a good place to create a new object called "rect" (or maybe "message"?), so that we don't modify aRect.  (Callers might not expect this function to mutate its argument.)

Comment 20

[Robin Ricard [:rricard]]

Attachment: 958111.patch

Bug 958111 - "Find in Page" on Android should zoom in on the highlighted result

Moved out the zoom functions out of browser.js to make a new helper called ZoomHelper
Called this new helper from the FindHelper
Added a new assertion in the testFindInPage robocop test

Comment 21

[Robin Ricard [:rricard]]

I thank you for spending so much time helping me getting all of this. Now I'm ready for another bugs ! It's very exciting to help this awesome community. I'll keep the patches coming ! And again if you have other suggestions on this patch, feel free to ask me some updates.

Comment 22

[:Margaret Leibovic]

Comment on attachment 8363825 [details] [diff] [review]
958111.patch

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

Thanks so much for the patch! This is looking good. I pushed it to try server, and if all the tests pass, we can land a new version of the patch with my comments below addressed.

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

::: mobile/android/base/tests/testFindInPage.java
@@ +23,2 @@
>          width = mDriver.getGeckoWidth()/2;
> +        width2 = mDriver.getGeckoWidth()/32;

Please add some comments explaining the reasoning behind this math.

@@ +46,5 @@
>          } finally {
>              painted.close();
>          }
>      }
> +    

Nit: please remove this whitespace.

::: mobile/android/chrome/content/ZoomHelper.js
@@ +11,5 @@
> +    let boundingElement = aRange.offsetNode;
> +    while (!boundingElement.getBoundingClientRect && boundingElement.parentNode) {
> +      boundingElement = boundingElement.parentNode;
> +    }
> +  

Please go through and remove any the trailing whitespace from this file. It looks like some ended up on most blank lines.

Comment 23

[:Margaret Leibovic]

Oof, looks like the tests didn't even build properly:
https://tbpl.mozilla.org/php/getParsedLog.php?id=33420489&tree=Try

Were you able to run testFindInPage locally to make sure it worked?

Comment 24

[Robin Ricard [:rricard]]

Ok It works on my side but I'll try another device (I was testing directly on my Nexus 5). I'll create an emulated tablet. I fixed spaces and I'll review the tests.

Comment 25

[Robin Ricard [:rricard]]

Ok, I know what happend, it's stupid. I recompiled partially the project (only JS) because I was using the same workflow as with JS with Robocop. But this time I recompiled everything, and of course my java was totally incorrect. My bad. I'm pushing a new patch soon !

Comment 26

[Robin Ricard [:rricard]]

Attachment: 958111.patch

Bug 958111 - "Find in Page" on Android should zoom in on the highlighted result

Moved out the zoom functions out of browser.js to make a new helper called ZoomHelper
Called this new helper from the FindHelper
Added a new assertion in the testFindInPage robocop test

Comment 27

[Robin Ricard [:rricard]]

Didn't heard from you since a long time. If you have any feedback on my last patch, please let me know.

Comment 28

[:Margaret Leibovic]

(In reply to ricard.robin from comment #27)
> Didn't heard from you since a long time. If you have any feedback on my last
> patch, please let me know.

Ah, sorry I missed this! You should set the the review? flag in the future when you want someone to look at a patch, so that it will end up in their review queue.

Comment 29

[:Margaret Leibovic]

Comment on attachment 8365576 [details] [diff] [review]
958111.patch

(setting the flag myself)

Comment 30

[:Margaret Leibovic]

Comment on attachment 8365576 [details] [diff] [review]
958111.patch

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

::: mobile/android/base/tests/testFindInPage.java
@@ +47,5 @@
>              painted.close();
>          }
>      }
> +    
> +    public void testZoomWhenFound() {

I don't see you call this method anywhere. How does this work?

Comment 31

[Robin Ricard [:rricard]]

Oh ! Indeed ! Leftover !

Comment 32

[Robin Ricard [:rricard]]

Attachment: 958111.patch

Thanks for your feedback ! Hope this one will be good, sorry for the multiple resubmissions !

Comment 33

[:Margaret Leibovic]

Thanks for updating the patch so quickly! I pushed this one to try:
https://tbpl.mozilla.org/?tree=Try&rev=7281f6552736

I'll check back later, and if it's green I'll give the patch an r+ and we can land it.

Comment 34

[Robin Ricard [:rricard]]

Doesn't seem to pass ... Let me see what's wrong there

Comment 35

[Robin Ricard [:rricard]]

OK it just fails on 4.0 ...

Comment 36

[:Margaret Leibovic]

I've been re-triggering those jobs, since they seem like an intermittent testHomeBanner failure (bug 965358), not anything to do with the find in page bar, but I'd like to confirm it's intermittent before we push this patch.

Comment 37

[Robin Ricard [:rricard]]

rc1 seems to fail too on this build (actually on testFindInPage). I don't know why but it seems to be a process crash. Maybe it's because of the two assertions for one context (but why ?).

Comment 38

[:Margaret Leibovic]

(In reply to ricard.robin from comment #37)
> rc1 seems to fail too on this build (actually on testFindInPage). I don't
> know why but it seems to be a process crash. Maybe it's because of the two
> assertions for one context (but why ?).

I also re-triggered that job to see if that failure was just intermittent. Maybe we can get some input from gbrown to see if he knows what might cause that kind of failure.

Comment 39

[Geoff Brown [:gbrown]]

The "process crashed?" messages following testBrowserProvider can be ignored:

11:25:36     INFO -  340 INFO TEST-PASS | testBrowserProvider - TestExpireHistory | All thumbnails were deleted - 0 should equal 0
11:25:36     INFO -  341 INFO TEST-PASS | testBrowserProvider - TestExpireHistory | Expected number of inserts matches - 3000 should equal 3000
11:25:36     INFO -  INFO | automation.py | Application ran for: 0:05:01.378159
11:25:36     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmp51Gdoppidlog
11:25:37     INFO -  /data/anr/traces.txt not found
11:25:38     INFO -  WARNING | leakcheck | refcount logging is off, so leaks can't be detected!
11:25:38     INFO -  runtests.py | Running tests: end.
11:25:39     INFO -  Mochi-Remote ERROR | Automation Error: Missing end of test marker (process crashed?)

That is a pre-existing problem and there is a fix for that on the way (probably landing in a few days).

But the testFindInPage assertions look new to me:

11:33:01     INFO -  13 INFO TEST-UNEXPECTED-FAIL | testFindInPage | PaintExpecter - blockUtilClear timeout

14:54:36     INFO -  9 INFO TEST-UNEXPECTED-FAIL | testFindInPage | Pixel at 160,77 - Color rgba(255,255,255,255) not close enough to expected rgb(255,0,0)

I don't know that test well, and I don't immediately see the cause for those -- sorry.

Comment 40

[:Margaret Leibovic]

(In reply to Geoff Brown [:gbrown] from comment #39)

> But the testFindInPage assertions look new to me:
> 
> 11:33:01     INFO -  13 INFO TEST-UNEXPECTED-FAIL | testFindInPage |
> PaintExpecter - blockUtilClear timeout
> 
> 14:54:36     INFO -  9 INFO TEST-UNEXPECTED-FAIL | testFindInPage | Pixel at
> 160,77 - Color rgba(255,255,255,255) not close enough to expected
> rgb(255,0,0)
> 
> I don't know that test well, and I don't immediately see the cause for those
> -- sorry.

Thanks for taking a look, this definitely sounds like the test is failing.

Perhaps this is only happening on the 4.0 devices because the screen dimensions are different than the 2.2 devices.

Comment 41

[:Margaret Leibovic]

Comment on attachment 8367380 [details] [diff] [review]
958111.patch

Clearing review until we figure out what's causing that testFindInPage failure.

Comment 42

[:Margaret Leibovic]

I made a try push without the test updates:
https://tbpl.mozilla.org/?tree=Try&rev=5339a2782c15

This passed for me locally, and I'm not sure we need to add that extra assertion, so let's see what happens without it.

Comment 43

[:Margaret Leibovic]

Sorry I've been so slow to help investigate this! I tried a few different things to fix the assertion that was failing, but I didn't have any luck with that, so I think we should just try disabling it, since I don't actually think it's as important as the assertion that checks if we actually found something.

I made a try run that did that, and it passed:
https://hg.mozilla.org/try/rev/2cbe0fc86592

I also think we can update the new element on the left side of the page to be blue as well, and then we only need that original isnotpixel assertion.

Do you want to try updating your patch with these changes? I'll push a new final version to try again, and then I can land it for you.

Comment 44

[Robin Ricard [:rricard]]

Ok, no problems, I'm working on it right now !

Comment 45

[Robin Ricard [:rricard]]

And You're actually right, one assertion should be enough !

Comment 46

[Robin Ricard [:rricard]]

Attachment: 958111.patch

Got back the original assertion. Still keeps the left colored section.

Comment 47

[:Margaret Leibovic]

Thanks for the quick response! It actually looks like this patch has bit-rotted since it was first written. Could you pull the latest mozilla-central and rebase on top of that? After that, I can push the patch to try for you.

Comment 48

[Robin Ricard [:rricard]]

Ok, I'll try to see if I need to merge something.

Comment 49

[Robin Ricard [:rricard]]

Ok, I think I messed up some things with my mercurial repository and mq. I actually managed to merge it by just stripping conflict lines (the ones with <<<<< and >>>>>) but then I didn't managed to get it on my patch. So I'm trying to fix my patch manually but I don't think it'll work.

Comment 50

[Robin Ricard [:rricard]]

Can you do the merge for me actually. Anyway, I'm trying to solve it using the git repository (I'm more used to it actually).

Comment 51

[Robin Ricard [:rricard]]

Attachment: 958111.patch

I redone it with git and the latest version.

Comment 52

[:Margaret Leibovic]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=9e23237d2481

Comment 53

[:Margaret Leibovic]

Oh no, still a failure! I didn't look at your patch closely enough... I think you should disable the first ispixel assertion (the one that happens before the successful search), since that's the one giving us trouble.

Comment 54

[:Margaret Leibovic]

I wrote another patch myself to just disable this assertion. If this try run is green, I'll get a review for that and (finally) land these patches.

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

Comment 55

[:Margaret Leibovic]

And yet another try!
https://tbpl.mozilla.org/?tree=Try&rev=397ebbe4bff4

Comment 56

[:Margaret Leibovic]

Attachment: disable first assertion in testFindInPage

I verified on try that disabling this assertion (and the actions around it) make the test pass. I feel like it's bad practice to disable tests to land a feature, but I could not reproduce this locally to debug, and I feel like this "page did not scroll" check isn't as important as the "page *did* scroll" check.

gbrown, what do you think?

Comment 57

[:Margaret Leibovic]

Comment on attachment 8375102 [details] [diff] [review]
958111.patch

r+, although we need to fix the testing situation before landing.

Comment 58

[Geoff Brown [:gbrown]]

Comment on attachment 8376303 [details] [diff] [review]
disable first assertion in testFindInPage

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

That seems reasonable.

Comment 59

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/91176f7dd8e2
https://hg.mozilla.org/integration/fx-team/rev/9fdffc707a26

Comment 60

[:Margaret Leibovic]

Oops, I just realized that ibarlow wasn't looped in on this bug...

I posted a build with the patch here:
http://people.mozilla.org/~mleibovic/findinpage.apk

ibarlow, can you give this a spin and let us know if you're okay with this change to the find in page interaction? We can also file follow-ups if there is more polish to do.

Comment 61

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/91176f7dd8e2
https://hg.mozilla.org/mozilla-central/rev/9fdffc707a26

Comment 62

[Mark Capella [:capella]]

When I do a "find in page", I'm supposed to see results like this?

https://www.dropbox.com/s/aouvxfb9iza5dr4/zoom.png

I'd been assuming this was a recent bug and someone was working to correct it ...

Comment 63

[Mark Capella [:capella]]

Wonder if I can ping margaret for a quick reality-check on my previous comment ... ? On my GS3 "Find" now zooms in so close to the text that it obscures surrounding information / readable context ... is my device / situation unique?

Or is the new-norm for user (me) to pinch-zoom out once the targeted text is located and auto-zoomed?

Comment 64

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

I'm seeing this too ^ - I agree it's a little annoying to be zoomed in that far.

Comment 65

[Wesley Johnston (:wesj)]

I wonder if we can just find the first enclosing display: block element and zoom to that? I'll file a separate bug for that...

Comment 66

[:Margaret Leibovic]

I filed bug 1014113 to track this fallout. I think we should consider disabling this before we hit release if we're not happy with the current behavior.