1570566 - Taobao.com isn't displayed properly in landscape mode on Android

Status: RESOLVED FIXED

(Web Compatibility :: Site Reports, defect)

Description

[Emily Kager [:ekager]]

Copied from: https://github.com/mozilla-mobile/fenix/issues/3898
Can reproduce on Fennec but not Chrome

Steps to reproduce

1. Visit taobao.com
2. Rotate device to landscape view

Expected behavior

The page is properly displayed, no UI defects.

Actual behavior

The page isn't properly displayed, everything seems to be cornered to the left

Device information

• Android device:
  • Google Pixel 3XL (Android 9)
  • Google Pixel 3 (Android 9)
  • Pixel Xl (Android Q Beta 4)
  • Samsung Galaxy S9 (Android 8)
  • Nexus 9 (Android 7.1.1)

• Fenix version: 1.0.1-rc.1 Build # 11851847 4/7

Notes

Comment 1

[Chris Peterson [:cpeterson]]

I think this is a Gecko layout issue so I'm sending this bug to the Core::Layout component.

This bug was originally reported in the Fenix issue tracker, but Emily says the bug is also reproducible in Fennec.

https://github.com/mozilla-mobile/fenix/issues/3898

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This is not a layout issue. The width is fixed via CSS.

This is reproducible in RDM in https://h5.m.taobao.com/. If you rotate the device nothing happens, but if you then do window.dispatchEvent(new CustomEvent("resize")), then it works. They update the page by literally reloading, which is just amazing.

There are also various suspicious bits about orientation sensors being deprecated.

I've dug a bit more with no clear result, but I think this pretty clearly is not a layout bug. I suspect the url is being reset via location / location.href, but I couldn't verify without a build because those properties are unforgeable.

Mike, is this some issue someone from your team can investigate?

STR are:

• Open https://h5.m.taobao.com on RDM / Fennec / GeckoView.
• Rotate screen, see the page not reload.

If you don't have the cycles and Chris thinks is important, please ni? me and I'll get to it eventually.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Oh, also beware: if you open the page in Chrome (which I did to try to intercept / log the navigation), they try to run xdg-open, somehow. I haven't tried to open whatever they're trying me to open...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Ah, there you go. In index.min.js:

window.addEventListener("resize",function(){window.innerWidth!==Xe&&window.location.reload()})}]);

Not sure what could be in that Xe

Comment 5

[Emilio Cobos Álvarez (:emilio)]

So that's working properly... But why does window.location.reload() doesn't work from the resize listener then?

I recall reading that code... LOL

Here's the issue https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/dom/base/Location.cpp#740

  if (window && window->IsHandlingResizeEvent()) {
    // location.reload() was called on a window that is handling a
    // resize event. Sites do this since Netscape 4.x needed it, but
    // we don't, and it's a horrible experience for nothing. In stead
    // of reloading the page, just clear style data and reflow the
    // page since some sites may use this trick to work around gecko
    // reflow bugs, and this should have the same effect.

    nsCOMPtr<Document> doc = window->GetExtantDoc();

    nsPresContext* pcx;
    if (doc && (pcx = doc->GetPresContext())) {
      pcx->RebuildAllStyleData(NS_STYLE_HINT_REFLOW,
                               RestyleHint::RestyleSubtree());
    }

    return NS_OK;
  }

Hah!

Boris, permission to revert bug 258917? I never liked that RebuildAllStyleData call :)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Or alternatively just wontfix this or something, if we believe that RebuildAllStyleData call is worth keeping.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Or putting behind a pref, disable on android and Nightly with the hope of eventually removing or what not.

Comment 8

[Karl Dubost💡 :karlcow]

This is happening in Firefox 67 to Firefox 70 at least. So indeed unrelated to Bug 1528052, fixed in 68. (as mentioned by hiroyuki on IRC).

Comment 9

[Karl Dubost💡 :karlcow]

Attachment: before-rotation

The site once accessed in Firefox Nightly 70 with RDM and Firefox Mobile UA.

Comment 10

[Karl Dubost💡 :karlcow]

Attachment: after-rotation

The site once accessed in Firefox Nightly 70 with RDM and Firefox Mobile UA and then rotated in landscape mode.

Comment 11

[Karl Dubost💡 :karlcow]

Attachment: chrome-landscape-orientation

And this is what it looks like in Chrome once in landscape mode.

Comment 12

[Karl Dubost💡 :karlcow]

hiroyuki is also mentioning Bug 1523844 which is in active development.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Those are not related. The site is just handling dynamic resizes by reloading if the width changes. The layout is correct, they just set a bunch of widths to innerWidth.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Do other browsers generally reload if you reload inside a resize handler?

Because https://html.spec.whatwg.org/multipage/history.html#dom-location-reload says:

If the currently executing task is the dispatch of a resize event in response to the user resizing the browsing context
Repaint the browsing context and return.

Now maybe they are not treating this orientation-change case as "in response to the user resizing the browsing context"? Or maybe they're not following the spec at all here?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Looking at this test-case, it seems they're just not following the spec whatsoever:

<!doctype html>
<script>
  onresize = () => location.reload();
  onload = () => console.log("load");
</script>

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK. Well, if we figure the original problem that bug 258917 is no longer an issue, we can go ahead and back it out, I guess, and get the spec changed.

That said, for a desktop user that taobao website will become horrible because opening devtools reloads, resizing the window reloads continuously as you resize, opening the findbar reloads, etc.... Maybe they just do that in their mobile version?

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Yes, they only do that on mobile.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

I'll write something, yeah... I'm not sure other browsers have TypeAheadFind, so I'll disable only on Desktop Nightly and Mobile for now. If it causes desktop regressions we can just keep the pref on on nightly and off for desktop or something.

Comment 19

[Boris Zbarsky [:bzbarsky]]

To be clear, the issue with find is not TypeAheadFind per se but that our findbar appearing changes the size of the content area. This is true whether you're using TAF or just Cmd+F/Ctrl+F. See also bug 1281655.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1570566 - Don't block reloading during a resize event handler on Android and Nightly. r=bzbarsky

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1570566 - Cleanup a bit Location::Reload() and History::Go(0). r=bzbarsky

This code was jumping through some hoops that seemed unnecessary. Given
History::Go(0) already calls Location::Reload(false), seems we can just handle
the new pref in one place, and also simplify the code a bit.

Depends on D40430

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17ea07407731
Don't block reloading during a resize event handler on Android and Nightly. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/c4265d8d0301
Cleanup a bit Location::Reload() and History::Go(0). r=bzbarsky

Comment 23

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/17ea07407731
https://hg.mozilla.org/mozilla-central/rev/c4265d8d0301

Comment 24

[Ryan VanderMeulen [:RyanVM]]

I assume we want to let this fix ride the trains. Feel free to nominate for uplift if that's not the case, however.

Comment 25

[Tom Ritter [:tjr]]

Emilio, I found another 2+ year old Nightly difference - can this one right the trains out too?

https://searchfox.org/mozilla-central/rev/4c3f6e8bf87fffb7c62feb4c76a14e0eb0b94c1f/modules/libpref/init/StaticPrefList.yaml#1965-1982