1768527 - FasTrak's custom MapQuest map never finishes loading in Firefox for Android, due to endless loop of resize -> reload

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, defect)

Description

[Daniel Holbert [:dholbert]]

STR:

1. Visit https://www.bayareafastrak.org/en/tolls/retailmap.html in Firefox for Android
   (or on desktop with RDM, with touch simulation activated (the little finger icon))

ACTUAL RESULTS:
The site loads and then immediately reloads, via this resize listener in its html:

 $(window).on({
[...]
	resize:function(e){
		waitForFinalEvent("document.location.reload()", 200, "uniqueIdyyy");
	}
 });

EXPECTED RESULTS:
No such automatic/infinite reload.

It seems that in Firefox (and I think only in Firefox?) we trigger a resize event shortly after the page loads, which makes the site reload via that ^ listener, and this repeats forever; so the site is effectively unusable.

Chrome gives EXPECTED RESULTS.

Comment 1

[Daniel Holbert [:dholbert]]

Using mozregression on desktop with RDM, I go back to https://bugzilla.mozilla.org/show_bug.cgi?id=1521934 , which wasn't really a regression but rather was just a pref-flip (devtools.responsive.metaViewport.enabled) to make RDM a bit more true-to-life for emulating mobile devices.

With that pref manually set to true, I get an older regression range which points to bug 1290420, which also seems to have been a change to make RDM more true-to-life for emulating mobile devices. i.e. Nightly 2018-10-04 is the first build where I can reproduce on desktop RDM, with devtools.responsive.metaViewport.enabled manually set to true.

So: this isn't a regression from bug 1290420 or from the devtools.responsive.metaViewport.enabled pref, but it does require those in order to have the right set of conditions to be able to repro on desktop.

This is reproducible in GeckoView Example App, but unfortunately we only save GVE builds for a year, so we can't go back further than May 2021 with those builds to see when this really regressed (on mobile).

Comment 2

[Daniel Holbert [:dholbert]]

hiro and/or botond, I wonder if this is something you've encountered before?

Obviously this "reload-on-resize" pattern is not-great; but it does seem to reveal that we seem to be sending a resize event here where Blink/WebKit are not, which is perhaps worth addressing if we can.

Comment 3

[Daniel Holbert [:dholbert]]

(Looks like bug 1528052 was similar, but its mitigation apparently isn't sufficient for this particular site. --> adding see-also.)

Comment 4

[Hiroyuki Ikezoe (:hiro)]

So basically the reason why the redundant resize event is triggered is that we call PresShell::ResizeReflowIgnoreOverride after we set PresShell::mDidInitialize to true.

One of the places where we trigger PresShell::ResizeReflowIgnoreOverride is in the before-first-paint event case. There are multiple sources of trigger ResizeReflowIgnoreOverride after mDidInitialize to true. I would say it's not worth spending more time on investigating all possible scenarios there. Rather I'd say we should spend time on figuring out how to solve it here in this bug.

A naive way I can think of right now is to drop this !mIsFirstPaint check in MobileViewportManager::RefreshViewportSize so that we will never call ResizeReflowIgnoreOverride if the mobile viewport size isn't changed.

I wonder why the !mIsFirstPaint check is necessary, it was introduced in bug 1178847.

I am setting S2 since such kind of sites are totally un-usable, there's no workaround.

Comment 5

[Botond Ballo [:botond]]

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

I wonder why the !mIsFirstPaint check is necessary, it was introduced in bug 1178847.

Though I'm not sure, I have two theories:

1. mMobileViewportSize might get set relatively early (e.g. in response to a DOMMetaAdded event), before the document is first painted. Then the reflow triggered by that early RefreshViewportSize() call might have early-exited somewhere, and when the first paint does happen, we need the reflow to actually take place?
2. We may need it to ensure the logic in UpdateResolutionForFirstPaint is run?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

The comment 4 does just describe a fact how a redundant resize event happens. There's a real underlying issue why it happens.

The underlying issue is that while we are initializing our PresShell in nsDocumentViewer::InitPresentationStuff we set the pres context's visible area with the document viewer bounds, which means the visible area is as if the document has <meta "name=viewport" content="width=device-width">. So if the document doesn't have any meta viewport, the document gets rendered on the device width canvas, then it's re-rendered on 980px width canvas. That's the reason why the resize event happens on documents which has no meta viewport.

In nsDocumentViewer::InitPresentationStuff, we can use the proper visible area via MobileViewportManager which has already been initialized.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768527 - Add meta viewport to make the tests work as intended on mobile. r?masayuki

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768527 - Add meta viewport to make the tests work as same as desktops. r?emilio

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768527 - Add meta viewport to make the test consistent with the fix for bug 1768527. r?emilio

I have totally no idea why some test cases in table-rows-with-zero-columns.html
pass with the meta viewport, we will track it in bug 1916567.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768527 - Add meta viewport to make the test consistent with the fix for bug 1768527. r?emilio

Similar to the previous commit, we will track this unexpected pass in bug
1916568.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768527 - Add meta viewport to make the tests work as intended on mobile. r?emilio

On our CI for Android, the device screen width is 800, without meta viewport
Gecko tries to render such documents in 980px width canvas, thus the document
gets scaled by 0.816326531 (= 800 / 980), with this scale the tests in this change
will not work as intended with the next commit. There will be two failure cases;

1. the content is not scrollable because of the scale
   abspos-contributes-to-static-parent-bounds.html is one of this category
2. fractional scroll position difference (or some such) because of our
   scroll position rounding / pixel alignment issues

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768527 - Set the initial visible area with MobileViewportManager. r?emilio!,botond!

On mobile environments, specifically web contents without meta viewport tags get
rendered on 980px width device (i.e. desktop mode), thus using the document
viewer size directly for the pres context's visible area would result a resize event.
Using MobileViewportManager avoids the resize event.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Reminder to please file a follow-up to remove the extra reflow from the width=device-width case, which is what seems to be causing the need for comment 10 and comment 9... And just a question, I think that's not really introduced by this patch, correct?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez (:emilio) from bug 1916568 comment 3)

Created attachment 9422517 [details]
Test-case that updates on window resize too.

This also seems like an incremental layout issue, but one where we get the margins correct in the initial layout and then break them, right?

When I load this test-case on my machine, I get 30px, however after a resize I get 0px.

With D220976, we get 30px (without the meta viewport), thus the patch reduces the extra reflow I suppose.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b45563a1114
Add meta viewport to make the tests work as intended on mobile. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/b76812613c49
Add meta viewport to make the tests work as same as desktops. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a504b7a18e2c
Add meta viewport to make the test consistent with the fix for bug 1768527. r=emilio
https://hg.mozilla.org/integration/autoland/rev/de576a4406e0
Add meta viewport to make the test consistent with the fix for bug 1768527. r=emilio
https://hg.mozilla.org/integration/autoland/rev/3f775b657457
Add meta viewport to make the tests work as intended on mobile. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cca9937c0323
Set the initial visible area with MobileViewportManager. r=emilio,botond

Comment 16

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

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

Comment 17

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/1b45563a1114
https://hg.mozilla.org/mozilla-central/rev/b76812613c49
https://hg.mozilla.org/mozilla-central/rev/a504b7a18e2c
https://hg.mozilla.org/mozilla-central/rev/de576a4406e0
https://hg.mozilla.org/mozilla-central/rev/3f775b657457
https://hg.mozilla.org/mozilla-central/rev/cca9937c0323

Comment 18

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

Upstream PR merged by moz-wptsync-bot