990259 - Double-tap zoom does not work when text reflow is enabled

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce:
1) In the Firefox for Android settings, turn on "Text reflow"
2) Open a web page that uses the default (desktop-sized) layout viewport, such as http://daringfireball.net/
3) Double-tap on a column of text

Expected results: Firefox zooms to the width of the text column.

Actual results: Nothing happens.

On the same page with "Text reflow" disabled, double-tap works as expected.

Comment 1

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

Are there any JS errors in logcat?

Comment 2

[Matt Brubeck (:mbrubeck)]

E/GeckoConsole(24474): [JavaScript Error: "ReferenceError: aElement is not defined" {file: "chrome://browser/content/ZoomHelper.js" line: 106}]

Comment 3

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

Regression from bug 958111. ZoomHelper.zoomToRect doesn't have an "aElement" parameter. This seems to be a refactoring error when the code was moved to ZoomHelper.js.

Comment 4

[Aaron Train [:aaronmt]]

In 29, there is no visible user option to reflow text. I could not reproduce using the about:config option. Also, bug 958111 only landed on mozilla-30.

Comment 5

[:Margaret Leibovic]

Robin, would you have time to look into this bug?

Comment 6

[Robin Ricard [:rricard]]

I can look into it next week, not before probably...

Comment 7

[Robin Ricard [:rricard]]

It doesn't seem too hard to solve anyway. I will try to solve it before the end of the week if I can.

Comment 8

[:Margaret Leibovic]

(In reply to Robin Ricard from comment #7)
> It doesn't seem too hard to solve anyway. I will try to solve it before the
> end of the week if I can.

Thanks! Let me know if you don't have time to work on this, and I can take it over. We just want to make sure it gets fixed soon since it's a regression that's on Aurora now.

Comment 9

[Robin Ricard [:rricard]]

Ok, let's do this ! Hope I didn't do something too stupid in the last patch !

Comment 10

[Robin Ricard [:rricard]]

Attachment: 990259.patch

I should have solved this bug way sooner ! It was really easy to fix.

I just had this question : How can I submit patches to the build bot if I need to ? As I want to work more on firefox, it could make me more efficient at shipping stuff.

Comment 11

[Robin Ricard [:rricard]]

Attachment: 990259.patch

This way of doing things is more clean. Feel free to ping me back if another regression appears. I checked all of the variable scopes this time, it doesn't seem likely to appear again.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8402291 [details] [diff] [review]
990259.patch

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

Thanks for looking into this! I just want you to change the order of the method parameters, then we can land this.

::: mobile/android/chrome/content/ZoomHelper.js
@@ +85,3 @@
>    },
>  
> +  zoomToRect: function(aRect, aElement, aClickY = -1, aCanZoomOut = true, aCanScrollHorizontally = true) {

This change will break the zoomToRect call in FindHelper. If you want aElement to be an optional parameter, you should make it the last parameter, then the consumer in FindHelper can continue to not use it.

@@ +102,4 @@
>  
>      // 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 && aElement !== undefined) {

I'm a bit concerned about this change in behavior depending on whether or not the method is passed a valid element.

This seems like a fine temporary fix to restore the previous behavior, but if we expect any other consumers to user zoomToRect this code could use some refactoring to split the reflow on zoom part out.

Comment 13

[Robin Ricard [:rricard]]

Ok, that's exactly what I thought ! I'm now changing it to make the zoomToRect function do exactly what it's made for.

Comment 14

[Robin Ricard [:rricard]]

Attachment: 990259.patch

This one is much more different. Let me know if you see something wrong in it. I'll rerun the tests again on my Nexus 5 before going to sleep, just to be sure. But I think, that's a much better approach in terms of reusability. Thanks for your feedback !

Comment 15

[:Margaret Leibovic]

Comment on attachment 8402918 [details] [diff] [review]
990259.patch

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

I agree this is better, thanks! I just have two more points for you to address.

You should update the FindHelper consumer to get rid of the last two parameters:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/FindHelper.js#80

In fact, you could also get rid of the aClick parameter, since it defaults to -1 anyway.

::: mobile/android/chrome/content/ZoomHelper.js
@@ +98,5 @@
>      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 && aElement !== undefined) {

You can get rid of this aElement !== undefined check, since this method should always be called with a valid element, and it will run into problems in the getBoundingContentRect call up above before ever getting here.

Comment 16

[Robin Ricard [:rricard]]

Oh ! Nice catch !

Comment 17

[Robin Ricard [:rricard]]

Attachment: 990259.patch

Well, it seems to work on my side (on the concerned tests, I forgot to run it on the findInPage test last time ...).

Comment 18

[:Margaret Leibovic]

Comment on attachment 8402959 [details] [diff] [review]
990259.patch

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

This looks good, thanks!

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[:Margaret Leibovic]

I'll request approval to aurora once this is verified on nightly.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing Android 4.0 robocop-2 perma-fail since it landed.
https://hg.mozilla.org/integration/fx-team/rev/e2d52a541b50

https://tbpl.mozilla.org/php/getParsedLog.php?id=37430746&tree=Fx-Team

Yes, same failure as bug 973909.

Comment 23

[Robin Ricard [:rricard]]

OK ... That's weird ... What about the logcat ? I can't see what's wrong from an assertion error. I only fixed a missing variable in the scope and it works on my Nexus 5 with Android 4.3 ... I think I can fix it but I need some additional information if it's on the implementation side. If it's only the assertion, I can look into the test to see why it fails. In fact, the "is not different enough from" part in the error feels suspicious to me.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

You're asking the wrong guy. Margaret?

Comment 25

[Robin Ricard [:rricard]]

Sorry, since you reopened the issue, I assumed you may had information for me. I'll just ask margaret. Thanks anyway to reping me back on this issue !

Comment 26

[Robin Ricard [:rricard]]

Hi margaret, the issue reopened due to a test failing on 4.0 (Bug 973909). That's weird to me but maybe you could give me directions to investigate the bug. I don't see something obvious right now but I suspect it to be on the test side. "Color is not different enough from" seems very suspicious to me.

Comment 27

[:Margaret Leibovic]

These tests are so frustrating! Robin, have you been able to reproduce this issue when you run testFindInPage locally?

I can help look into this later today. If we can't reproduce it locally, we should just look more closely at the patch you wrote and see if we can figure out whether we might have accidentally changed the logic here while refactoring.

Comment 28

[Robin Ricard [:rricard]]

Well, I'll try to reproduce it but I just encountered a bug with the test runner on my computer lately. sqlite seems to not link properly, which is weird but I'll try to recompile fx from the beginning and I hope it'll work.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/e2d52a541b50

Confirmed that robocop-2 went green again after this was backed out.

Comment 30

[:Margaret Leibovic]

I also wasn't able to reproduce this locally.

Looking at this patch more closely, I'm worried that we're unintentionally changing the behavior of zoomToRect. I'm also worried that this logic is messy and potentially has other bugs, but let's just worry about one thing at a time here :)

I think we should try going with your first approach again, with my comments addressed, then we can do a larger refactoring in a follow-up bug. This code is definitely hairy and could use some clean-up, but I want us to address the regression caused by bug 958111 before doing any refactoring.

I made a try push with an updated version of your first approach:
https://tbpl.mozilla.org/?tree=Try&rev=45144779fad7

Comment 31

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #30)
> I also wasn't able to reproduce this locally.
> 
> Looking at this patch more closely, I'm worried that we're unintentionally
> changing the behavior of zoomToRect. I'm also worried that this logic is
> messy and potentially has other bugs, but let's just worry about one thing
> at a time here :)
> 
> I think we should try going with your first approach again, with my comments
> addressed, then we can do a larger refactoring in a follow-up bug. This code
> is definitely hairy and could use some clean-up, but I want us to address
> the regression caused by bug 958111 before doing any refactoring.
> 
> I made a try push with an updated version of your first approach:
> https://tbpl.mozilla.org/?tree=Try&rev=45144779fad7

Okay, that did the job. I'm just going to land this patch to fix the regression, and we can file a follow-up bug to work on the refactoring.

Comment 32

[:Margaret Leibovic]

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

Comment 33

[:Margaret Leibovic]

Attachment: patch that landed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 958111 
User impact if declined: double-tap zoom does not work when text reflow is enabled
Testing completed (on m-c, etc.): just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, minimal change to fix regression caused by refactoring in bug 958111
String or IDL/UUID changes made by this patch:

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9954edd7934

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5a256364b175

Comment 36

[Flaviu Cos]

Verified as fixed in builds:
- 31.0a1 (2014-04-17);
- 30.0a2 (2014-04-17);
Device: Lenovo Yoga Tab 10 (Android 4.2.2).

Comment 37

[Matthew Zaleski]

This does not seem to be a complete fix.  Let me know if I should create another Bug linked to this.

Device: Nexus 10 (4.4.2)
Version: 30.0a2 (2014-04-24)

The fix did correct it on some sites.  However all pages on wsj.com are still broken.  An example page (not locked behind paywall):
http://online.wsj.com/news/articles/SB10001424052702304788404579522612497528586

Comment 38

[Ryan VanderMeulen [:RyanVM]]

That's an issue for the bug assignee, not me. And if you have to ask, a new bug is probably the right thing to do :)

Comment 39

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

Yes, please file a new bug. The issue you are describing is probably unrelated to the patch that landed here.