1519013 - Shrink the overflowed contents to the display size if user has never changed zoom level by APZ

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1519013 - Introduce nsIPresShell::IsResolutionUpdatedByApz to tell whether the resolution has never been changed by APZ. r?botond

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1519013 - Shrink the content to display size if user has never changed the site resolution. r?botond

Depends on D16154

Comment 4

[Hiroyuki Ikezoe (:hiro)]

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

Comment 5

[Botond Ballo [:botond]]

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.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1519013 - Add initial-scale=1 in scrollintoview.html. r=botond

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

Depends on D16154

Comment 8

[Botond Ballo [:botond]]

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.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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

Comment 10

[csheany]

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

Comment 11

[Botond Ballo [:botond]]

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

Comment 12

[csheany]

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.

Comment 13

[Botond Ballo [:botond]]

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

Comment 14

[csheany]

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

Comment 15

[Botond Ballo [:botond]]

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

Comment 16

[csheany]

Why is that blocking this?

Comment 17

[Botond Ballo [:botond]]

For the reason Hiro explained in comment 1.

Comment 18

[csheany]

I give up :)

Comment 19

[Botond Ballo [:botond]]

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.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 21

[Botond Ballo [:botond]]

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?

Comment 22

[Hiroyuki Ikezoe (:hiro)]

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

Comment 23

[Botond Ballo [:botond]]

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

Comment 24

[Botond Ballo [:botond]]

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

Comment 25

[Hiroyuki Ikezoe (:hiro)]

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

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Botond found bug 1519007 for that. :)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a99248b3ec38
Introduce nsIPresShell::IsResolutionUpdatedByApz to tell whether the resolution has never been changed by APZ. r=botond
https://hg.mozilla.org/integration/autoland/rev/7f3a85a7e45a
Add initial-scale=1 in scrollintoview.html. r=botond
https://hg.mozilla.org/integration/autoland/rev/17f0796b0f6e
Shrink the content to display size if user has never changed the site resolution. r=botond

Comment 28

[Oana Pop-Rus]

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

Comment 30

[Botond Ballo [:botond]]

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

Comment 31

[Hiroyuki Ikezoe (:hiro)]

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 32

[Hiroyuki Ikezoe (:hiro)]

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

Comment 33

[Botond Ballo [:botond]]

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

Comment 34

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16217 for changes under testing/web-platform/tests

Comment 35

[Pascal Chevrel:pascalc (PTO until April 26)]

(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

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case

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

Comment 37

[Sorina Florean [:sflorean]]

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

Comment 38

[Laurentiu Apahidean]

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 39

[Pascal Chevrel:pascalc (PTO until April 26)]

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.

Comment 40

[Narcis Beleuzu [:NarcisB]]

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

Comment 41

[Narcis Beleuzu [:NarcisB]]

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

Comment 42

[Botond Ballo [:botond]]

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

Comment 43

[Natalia Csoregi [:nataliaCs]]

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

Comment 44

[miralobontiu]

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!

Comment 45

[Narcis Beleuzu [:NarcisB]]

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