Spun off from bug 1423013.

I am not going to move any sites linked to bug 1423013 into here because all of the sites are potentially affected by this bug, i.e. whether the content is shrunk or not depends on the timing of the initial paint. If the overflowed contents is loaded before the initial paint, the content will be shrunk as expected.

Gah, we presumably have to fix bug 1519007 first since with the patches for this bug, bug 1519007 can be easily reproduced. Note that the patches for this bug is basically just doing additional calls of MobileViewportManager::UpdateResoution so the underlying issue may be there?

I am going to upload the patches for the reference without review requests.

Botond told me on Phabricator that the issue in comment 1 is bug 1516056.

I think it might also make sense to fix bug 1520077 and bug 1520081 before this bug, as those changes should simplify the code in UpdateMinimumScaleSize(), making further changes to it easier to think about.

There were three unexpected-pass cases and an unexpected-failure case in wpt.

initial-scale=1 prevents from scaling down the content so that scrollIntoView
works as expected.

Depends on D16154

Bug 1516056 is unlikely to make 67. However, I will see if I can come up with a more targeted fix for the issue described in comment 1, which can.

We can then discuss if we want this fix in 67.

Yeah, I don't think it's worth fixing this in 67.

Question, I'm not sure if this desktop or mobile oriented but...

Is another way of saying this "doesn't choose a zoom level that makes the content larger than the visual viewport"?

The opposite of Bug 1509552

See also Bug 1523844

(In reply to csheany from comment #10)

Question, I'm not sure if this desktop or mobile oriented but...

This bug should affect mobile only.

Is another way of saying this "doesn't choose a zoom level that makes the content larger than the visual viewport"?

Yes.

See also Bug 1523844

Yep. Note, I did say in bug 1523844 comment 11 that this bug might fix that one.

I thought that would have been the fix back then I just wasn't sure it was beneficial.

As suggested in Comment 8 is that a patch that can land in 67.

It seems different than the ones posted thus far.

(In reply to csheany from comment #12)

As suggested in Comment 8 is that a patch that can land in 67.

I am not sure whether the "more targeted fix" mentioned in comment 8 will fix bug 1523844. I am not even sure that the full fix in this bug will. It's just a theory. We will see, once the fixes in question are actually written :)

I just meant could the targeted fix be the opposite of Bug 1509552 and patched seperately in Bug 1523844.

(In reply to csheany from comment #14)

I just meant could the targeted fix be the opposite of Bug 1509552 and patched seperately in Bug 1523844.

Ok, I think I understand now. The answer is no, the "opposite of bug 1509552" part would be the full fix here. It's the part that depends on bug 1516056, which is likely to target 68.

Why is that blocking this?

For the reason Hiro explained in comment 1.

I give up :)

With the fix for bug 1531057 in place, I can't reproduce the issue from comment 1 any more.

I believe that means this no longer needs to depend on bug 1516056, and we can proceed with the fix here.

I was seeing the issue on two different sites. https://www.ravelry.com/account/login https://webcompat.com/issues/17093 and https://www.spit-ct.ro/documente-declarare-persoane-fizice# https://webcompat.com/issues/17687. The first site has been reconstructed so that the link is no longer valid, but I can still see the issue on the second one.

That's being said, now it's hard to tell whether the issue can be easier reproducible with the patches for this bug, I can also see the issue on beta easily. (Probably you can easily confirm the issue if you drag the content to right during page loading.) So I think we can land the patches here independently. I am going to rebase the patches.

I was seeing the issue on two different sites. https://www.ravelry.com/account/login https://webcompat.com/issues/17093 and https://www.spit-ct.ro/documente-declarare-persoane-fizice# https://webcompat.com/issues/17687. The first site has been reconstructed so that the link is no longer valid, but I can still see the issue on the second one.

I did test that page, and didn't see the issue. If you still see it, do you have steps for how to trigger it?

(In reply to Botond Ballo [:botond] from comment #21)

I was seeing the issue on two different sites. https://www.ravelry.com/account/login https://webcompat.com/issues/17093 and https://www.spit-ct.ro/documente-declarare-persoane-fizice# https://webcompat.com/issues/17687. The first site has been reconstructed so that the link is no longer valid, but I can still see the issue on the second one.

I did test that page, and didn't see the issue. If you still see it, do you have steps for how to trigger it?

Ok, probably:

1. Load the site https://www.spit-ct.ro/documente-declarare-persoane-fizice#
2. Wait for finish loading
3. Shrink the content as possibe by pinch zoom
4. Try to swipe rightward (nothing happens here)
5. Pinch zoom in a bit
6. Swipe rightward

Then I could see the issue now on nightly. 3 might be not necessary though.

Ok, I do see it now, thanks! Definitely a different issue from what bug 1531057 fixed.

Oh, and this STR is not even fixed by bug 1516056!

Ok, I am going to file a new bug for it. As far as I can tell, the issue never happens on Chrome.

Botond found bug 1519007 for that. :)

https://hg.mozilla.org/mozilla-central/rev/a99248b3ec38
https://hg.mozilla.org/mozilla-central/rev/7f3a85a7e45a
https://hg.mozilla.org/mozilla-central/rev/17f0796b0f6e

Hiro, what are your thoughts on the possibility of uplifting these patches to 67? Do you think it would be safe to do so, or would it be better to let them ride the 68 train?

(I know you gave an opinion already in comment 9, but since then this bug has become unblocked and the patches have landed, so the analysis may have changed.)

In terms of stability, we can uplift the patches here. And also, for users, it would be really nice to have this fix in Firefox 67 since many of sites has been affected by this bug (see comment 0).

What I was concerned before was bug 1519007, but it turned out that bug 1519007 happened regardless of these patches (see comment 20), and Botond fixed bug 1519007 (hooray!). So now I think it's worth uplifting the patches to 67.

Comment on attachment 9035547 [details]
Bug 1519013 - Shrink the content to display size if user has never changed the site resolution. r?botond

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: It's not a regression, but bug 1423013 made this bug more noticeable
• User impact if declined: Mobile users will see sites don't fit the screen width, then the site is able to scroll to the left and right
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch does actually tweak the initial scale value in response to the layout content size until user interaction happens, so I think it's harmless.
• String changes made/needed: None
• Do you want to request approval of these patches as well?: on, on

(In reply to Hiroyuki Ikezoe (:hiro) from comment #31)

What I was concerned before was bug 1519007, but it turned out that bug 1519007 happened regardless of these patches (see comment 20), and Botond fixed bug 1519007 (hooray!). So now I think it's worth uplifting the patches to 67.

Cool, thanks!

Note, I do plan to request uplift for bug 1519007 as well, after a few days of baking on nightly.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)

I did test that page, and didn't see the issue. If you still see it, do you have steps for how to trigger it?

Ok, probably:

1. Load the site https://www.spit-ct.ro/documente-declarare-persoane-fizice#
2. Wait for finish loading
3. Shrink the content as possibe by pinch zoom
4. Try to swipe rightward (nothing happens here)
5. Pinch zoom in a bit
6. Swipe rightward

Then I could see the issue now on nightly. 3 might be not necessary though.

I would like to have the fix verified by QE on Nightly before evaluating an uplift to beta, Hiro, you didn't request manual testing in your uplift reuest, but aren't those steps above valid STR to test the fix? Thanks

(In reply to Pascal Chevrel:pascalc from comment #35)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)

I did test that page, and didn't see the issue. If you still see it, do you have steps for how to trigger it?

Ok, probably:

1. Load the site https://www.spit-ct.ro/documente-declarare-persoane-fizice#
2. Wait for finish loading
3. Shrink the content as possibe by pinch zoom
4. Try to swipe rightward (nothing happens here)
5. Pinch zoom in a bit
6. Swipe rightward

Then I could see the issue now on nightly. 3 might be not necessary though.

I would like to have the fix verified by QE on Nightly before evaluating an uplift to beta, Hiro, you didn't request manual testing in your uplift reuest, but aren't those steps above valid STR to test the fix? Thanks

Right, that one isn't the case what this bug fixed.

Here is a test case to confirm the issue still exists on beta and has been fixed on nightly.

The content in the test case has two boxes, the one is a bigger blue box and the other one is a smaller blue box on the green box.

When you load this test case on Fennec, you will only see the blue box on beta, but you will see the both boxes (i.e. Fennec shrinks the content to the device screen size).

Laurentiu, can you take a look to verify this on Nightly? Thanks!

Hello,

I performed the test case posted in Comment 36 on a Samsung Galaxy S8+ (Android 8.0.0) and Huawei P9 Lite (Android 6.0).
On Beta 67.0b7 only the blue square is visible when loading the page and a zoom out has to be performed in order to see the whole page, on Nightly 68.0a1 (2019-04-03) the whole page is visible.

Due to my findings, I'll mark this issue as verified.

Comment on attachment 9035547 [details]
Bug 1519013 - Shrink the content to display size if user has never changed the site resolution. r?botond

Low risk, verified by QA on Nightly, uplift approved for 67 beta 9, thanks.

Backed out for bustages on MobileViewportManager.cpp

Backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/78075f5bf04a65a4665dc2c44f250b2de8a35f8e
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238662447&repo=mozilla-beta&lineNumber=22853

There were also bustages on nsGfxScrollFrame.cpp
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238662463&repo=mozilla-beta&lineNumber=21908

https://hg.mozilla.org/releases/mozilla-beta/rev/ee00e620926a

The bustages are the result of trying to apply the second patch without the first. All three patches need to be applied.

https://hg.mozilla.org/releases/mozilla-beta/rev/5b205e13c556
https://hg.mozilla.org/releases/mozilla-beta/rev/bbfac9aab665
https://hg.mozilla.org/releases/mozilla-beta/rev/e6c48e95795d

Hi,

Verified as fixed on Beta 67.0b9, following the test case from Comment 36, with Samsung Galaxy S8 (Android 9.0), and Huawei MediaPad Mi2 (Android 5.1.1).

Thanks!

:botond , I did that because only that patch was approved by Pascal.