Closed Bug 1208023 Opened 4 years ago Closed 4 years ago

initial-scale=0 in META VIEWPORT breaks page

Categories

(Firefox for Android :: Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: hsteen, Assigned: kats)

References

()

Details

Attachments

(5 files, 1 obsolete file)

This site has a nonsense initial-scale=0 instruction in its meta viewport tag, and this confuses Firefox on Android into rendering it with some crazy zoom factor - the site is unusable. 

(We'll ask them to fix this error in https://webcompat.com/issues/1713 so it may go away)
We end up doing a div-by-zero at [1] which results in an infinite-sized CSS viewport. We should definitely guard against bogus scale values like this.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=2343972473da#8054
Attached patch Patch (obsolete) — Splinter Review
I changed kViewportMinScale to be 0.1 so that it is the inverse of kViewportMaxScale, and so that people can't enter arbitrarily small min-scale values which could end up being problematic.

The other change is to not use the scaleFloat value unless it's within the min/max range. Note that once the nsViewportInfo object is created it constraints the mDefaultZoom to be between the min/max so the only part that's exposed to badness is the code in nsDocument::GetViewportInfo. This patch makes sure that code avoids the badness and fixes the bug.
Assignee: nobody → bugmail.mozilla
Attachment #8665576 - Flags: review?(botond)
Comment on attachment 8665576 [details] [diff] [review]
Patch

Actually this might break the 0.01 threshold check in MVM::UpdateResolution. Let me check...
Attachment #8665576 - Flags: review?(botond)
Ended up changing some more things around. Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=86ea686bad08, if that's green I'll put the patches up for review.
Attachment #8668086 - Flags: review?(botond) → review+
Comment on attachment 8668087 [details] [diff] [review]
Part 2 - Bump up the minimum zoom from 0 to 0.1

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

::: dom/base/test/test_meta_viewport0.html
@@ +23,5 @@
>      tests.push(function test1() {
>        SpecialPowers.pushPrefEnv(scaleRatio(1.0),
>          function() {
>            let info = getViewportInfo(800, 480);
> +          fuzzeq(info.defaultZoom, 0.1, "initial scale is unspecified");

Might there be value in exposing isDefaultZoomValid in nsIDOMWindowUtils.getViewportInfo() (and checking for it here)?

@@ +24,5 @@
>        SpecialPowers.pushPrefEnv(scaleRatio(1.0),
>          function() {
>            let info = getViewportInfo(800, 480);
> +          fuzzeq(info.defaultZoom, 0.1, "initial scale is unspecified");
> +          fuzzeq(info.minZoom, 0.1,     "minumum scale defaults to the absolute minumum");

correct typo ("minumum") while you're at it
Attachment #8668087 - Flags: review?(botond) → review+
Attachment #8668088 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #10)
> > +          fuzzeq(info.defaultZoom, 0.1, "initial scale is unspecified");
> 
> Might there be value in exposing isDefaultZoomValid in
> nsIDOMWindowUtils.getViewportInfo() (and checking for it here)?

I was debating that too but in the end decided it wasn't really worth it. It's a bit of an implementation detail, I'd rather have a test that provides a garbage initial-scale value and checks that it still gets rendered properly. Come to think of it, I should probably add such a test for this bug...
Attached patch Part 4 - TestSplinter Review
This did the job for me locally; try push to make sure it works in automation:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d225a75a6711
Attachment #8669179 - Flags: review?(botond)
Attachment #8669179 - Flags: review?(botond) → review+
Does the test actually fail without the fixes? (I initially thought that's what your try push was trying to test, I missed that it didn't include the test patch either :)).
Locally, yes, the test fails without the fixes.
You need to log in before you can comment on or make changes to this bug.