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.

Are there any JS errors in logcat?

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

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.

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.

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

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

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

(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.

Ok, let's do this ! Hope I didn't do something too stupid in the last 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.

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 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.

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

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 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.

Oh ! Nice catch !

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

Comment on attachment 8402959 [details] [diff] [review] 990259.patch Review of attachment 8402959 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks!

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

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.

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.

You're asking the wrong guy. Margaret?

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 !

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.

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.

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.

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/e2d52a541b50 Confirmed that robocop-2 went green again after this was backed out.

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

(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.

[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:

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

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

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 :)

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