Closed Bug 1038880 Opened 10 years ago Closed 10 years ago

maximum-scale is ignored when minimum-scale is omitted

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

30 Branch
ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: olsh.me, Assigned: zafar142007, Mentored)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

OS: Android 4
Device: ASUS MeMO Pad HD 7 ME173X, 1280x800, 216 DPI
Actual width is ~533px;
I have a page with a fixed image 480px and the viewport is set to maximum-scale=1.0 and width=480
And it seems like maximum-scale property completely ignored. But when is minimal-scale is set, all is fine.

Here is the pages:
Page without minimum-scale http://olsh.github.io/tests/without-min-scale.html (maximum-scale is ignored)
Page with minimum-scale http://olsh.github.io/tests/with-min-scale.html (Works as expected)


Actual results:

The image is resized to full width of display. User is able to zoom in. 


Expected results:

The image is keep own size (480px) and stay in the left corner of display. And user is not able to zoom in.
OS: Windows 8.1 → Android
Hardware: x86_64 → ARM
Summary: maximum-scale ignored when minimum-scale is omitted → maximum-scale is ignored when minimum-scale is omitted
At http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=420209cd2a0a#6209 maxScale is getting clamped using minScale which is NaN in this case. So maxScale ends up being NaN as well. It should use kViewportMinScale instead of minScale if minScale is NaN.

Nice easy bug for somebody looking to get started with hacking Fennec.
Mentor: bugmail.mozilla
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [lang=js][good first bug]
Can I give it a try?
Absolutely! The first step is to get Fennec building and running. If you haven't done so already you can do that by following the instructions at https://wiki.mozilla.org/Mobile/Fennec/Android - once you have that up and running let me know and I can assign the bug to you. The code that needs to be changed is described in comment 1.
Can I take up this bug? I am new to mozilla and would like to contribute.
For sure - please see comment 3 on how to get started. Once you have a build going let me know and I can assign the bug to you and we can talk about how to fix the bug and test it.
Sure, I will get back to you soon after getting Fennec built and running. Thanks!
Can I take this bug? I have got the initial build running.
Sure, I haven't heard back from Nachiketh yet so you can work on it. If he gets his Fennec build going there are some other good first bugs he can work on.

The first step for a bug like this is usually to reproduce the problem using the build you made. So in this case, you want to load the two URLs in comment 0 and observe the difference in behavior. So in this case, note how you can zoom in one of the pages way more than the other one. That's what we need to fix.

The code in question is in mobile/android/chrome/content/browser.js. See comment 1 for a pointer to the code and what needs to be changed there. Let me know if that's enough information for you to go on, or if you need more details on how the code works and whatnot. Some people find it a little hard to dive right in but others like the challenge of figuring it out, so I don't want to make it too easy right off the bat :)

Once you have figured out what code change you need to make and verified that it has the intended effect, read through the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch to see how to create a patch for submission with your proposed changes, and upload the patch to this bug.

Let me know if you have any questions at all.
Assignee: nobody → zafar142007
Sure. However, I see that I am able to zoom in a little even in the second case. Is that normal?
Yes; the second page has a viewport property of "maximum-scale=1.0". However depending on which device you're using, the page may initially be rendered at a zoom of less than 1.0, so you may be able to zoom in a little bit before you reach the maximum scale of 1.0. On the first page you will be able to zoom in even more than that.
Okay then. I have taken the steps to correct the bug as you hinted. The image stays in the upper left corner but now I am also able to zoom in both cases. That doesn't seem to be right. My correction: I checked for NaN on minScale and replaced it with kViewportMinScale, if true, and if false, the statement stayed the same.
Attached patch Patch (v1) (obsolete) — Splinter Review
I have done the needful.
Attachment #8477947 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8477947 [details] [diff] [review]
Patch (v1)

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

This looks great! I have some comments below on the patch. Once you have addressed the comments please upload a new patch and flag me for review on it. Also one other thing - please update the commit message to describe what the patch is doing rather than just copying the bug title. So in this case something like "Ensure we use a non-NaN lower bound when clamping the max-scale" would be better.

::: mobile/android/chrome/content/browser.js
@@ +6327,5 @@
> +    if(isNaN(minScale)){
> +      maxScale = this.clamp(maxScale, kViewportMinScale, kViewportMaxScale);
> +    }
> +    else{
> +      maxScale = this.clamp(maxScale, minScale, kViewportMaxScale);

This looks correct to me, but needs some cosmetic changes to fit in with our existing coding style. The simplest thing I think would be to use the ternary operator here, and replace that middle parameter with (isNaN(minScale) ? kViewportMinScale : minScale).

If you prefer to use the if/else structure that's fine too, but then please use the existing bracing and spacing that you see in the surrounding code. Specifically, insert a space after the "if" and before the "{"s, and cuddle the "else" between the close and open braces on the same line.
Attachment #8477947 - Flags: feedback?(bugmail.mozilla) → feedback+
I have done as requested. Used ternary operator and changed the patch message.
Attachment #8477947 - Attachment is obsolete: true
Attachment #8477955 - Flags: feedback?
Attachment #8477955 - Flags: feedback? → feedback?(bugmail.mozilla)
Comment on attachment 8477955 [details] [diff] [review]
bug1038880-v2.patch

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

Looks good, thanks! And sorry - I should have specified that you should keep the bug number in the commit message. But anyway, I landed the patch in our fx-team branch, which should get merged to the main trunk (mozilla-central) in the next day or two. At that point this bug will be marked fixed.

Thanks a lot for the quick patch! If there's any other bugs that interest you please feel free to take a crack at them, or let me know if you'd like some help finding an appropriate next bug to work on.
Attachment #8477955 - Flags: feedback?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/cf45937d8c9d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Nice work, Zafar! I also added a mention of your contribution to the weekly mobile team meeting notes.

It looks like you were interested in bug 677001 - take a look at the comment Wes made on that bug and feel free to start working on that if you're interested. Or we can find some other bugs that might interest you, if you prefer.
Thanks a lot Kartikaya! I was thinking of bug https://bugzilla.mozilla.org/show_bug.cgi?id=677001 but I am not too sure how to approach it. I am trying to figure out how to use prompt.jsm but not entirely sure if I will be able to do it. Should I start with a simpler bug as my second one?
Looking at the discussion and comments I think it would be good to break down bug 677001 into smaller bugs that can be worked on one at a time. That should make it more manageable. In particular, I think the first step would be to write a standalone folder-picker activity. The second step would be to import that into Fennec and hook it up to the modeGetFolder mode of the nsIFilePicker interface. And the final step would be to add some settings or addons to let the user pick the download folder.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.