Closed Bug 1648344 Opened 4 years ago Closed 4 years ago

Items are overlapped

Categories

(Core :: Panning and Zooming, defect, P3)

Product:
Core
Component:
Panning and Zooming
Version:
77 Branch
Platform:
Unspecified
Android
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla80
Project Flags:
Webcompat Priority ?
Tracking Flags:
Tracking Status
firefox80 --- fixed

People

(Reporter: karlcow, Assigned: bradwerth)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(5 files, 1 obsolete file)

Bug 1648344 Part 3: Update test expectations for viewport tags width no width or initial-scale.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1648344 Part 1: Apply default viewport width in more circumstances, matching Chrome.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1648344 Part 2: Cleanup Document to remove NoValidContent viewport type.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1648344 Part 4: Disable a wpt test on android.
47 bytes, text/x-phabricator-request
Details | Review
Screenshot_2020-07-29-17-22-18.png
78.34 KB, image/png
Details
  1. With Firefox Fenix Nightly Android (or Fennec or Firefox desktop with mobile settings on RDM)
  2. Go to https://m.mail.tim.it/

Actual:
notice how everything is wide

Expected:
text has reasonable size

Not happening in chrome. On a pixel 2 the width in firefox is 411px and in chrome 980px.

<meta name="viewport" content="user-scalable=no, user-scalable=false, user-scalable=0">

if I set

<meta name="viewport" content="width=980, user-scalable=no, user-scalable=false, user-scalable=0">

I get the same behavior than chrome.

Hiro/Brad, as the resident meta-viewport experts, do you have any thoughts on this? Maybe it's a dupe of some other bug somewhere?

Flags: needinfo?(hikezoe.birchill)
Flags: needinfo?(bwerth)

Sure, this will likely be related to code from Bug 1638773 or Bug 1598145. I'll find a fix.

Assignee: nobody → bwerth

Thanks!

Severity: -- → S3
Priority: -- → P3

Yeah, my wild guess is same as what Brad thinks, I think it's another fallback case from bug 1598145. (Clearing NI to me since Brad has started working on this)

Flags: needinfo?(hikezoe.birchill)

Hmm..., this is actually a different, older issue introduced by patches for Bug 1564021. In that bug, we determined that documents with missing or invalid meta viewport tags should be given the default layout viewport width of 980px. In this case, we have a valid meta viewport tag, but the tag makes no statement about viewport width. Since the meta viewport tag is present and valid, we choose a layout viewport width that matches the display width.

It would appear that Chrome has a different heuristic. Perhaps Chrome uses the default 980px width whenever width is unspecified. We could move to such a model. I'll post a patch that does this and we'll see if it breaks tests.

I think we have general disagreement with Chrome on when to apply the default viewport width. If we fix this, many of our existing tests will need to be changed to match Chrome behavior, for example dom/base/test/test_meta_viewport_auto_size_by_device_height.html and any other test_meta_viewport test that omits width constraints.

For example, in test_meta_viewport_auto_size_by_fixed_height_and_initial_scale_1.html, we have height=400, initial-scale=1. There's no width constraints. Chrome uses display size for this, and so do we. If you drop initial-scale, Chrome uses default width, but we don't. Changing to my proposed heuristic from comment 5 doesn't fix this, because that heuristic would also incorrectly affect the original case with initial-scale.

I'll work on this for a bit and see if I can come up with something that changes as little existing code as possible, that brings us into alignment with Chrome, if not with the spec.

To support legacy pages being viewed on mobile devices with narrow viewports,
we assign a default layout viewport of 980px when the meta viewport tag is
missing or invalid. This change also uses the default viewport width if there
are no meta viewport constraints on width ("width", "min-width", or
"max-width").

Comment on attachment 9160532 [details]
Bug 1648344 Part 1: Apply default viewport width of 980px when width is unconstrained.

This is an initial effort, not ready for review. As noted in comment 7, this fails the height=400, initial-scale=1 case (relative to Chrome) because the returned viewport is too large and causes overflow.

Brad, have you confirmed how Chrome pick the 980px? Is it coming from this @viewport rule? A fun fact that they use a different UA style sheet on tablets

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

Brad, have you confirmed how Chrome pick the 980px? Is it coming from this @viewport rule? A fun fact that they use a different UA style sheet on tablets

No I haven't investigated that. I'm not sure where the value comes from.

and Google didn't reply about unshipping @viewport :/ so it is difficult to know what will be done there.
https://github.com/w3c/csswg-drafts/issues/4766#issuecomment-585499609

Maybe I should file a bug against chromium. Let's see.

Note that the spec for @viewport rule will be obsoleted (that's my understanding), but it can be, of course, used internally as UA style sheet or some.

oh and we had already a bug for this Bug 1552713

I put together a sheet showing our meta viewport webcompat vs Chrome, before and after patch D81788. The patch fixes webcompat for some existing tests, and doesn't add incompatibility on any tests. I'm tempted to seek review and land it. I'm going to add a new part to the patch that updates our test expectations.

Flags: needinfo?(bwerth)

(In reply to Brad Werth [:bradwerth] from comment #16)

I put together a sheet showing our meta viewport webcompat vs Chrome, before and after patch D81788. The patch fixes webcompat for some existing tests, and doesn't add incompatibility on any tests. I'm tempted to seek review and land it. I'm going to add a new part to the patch that updates our test expectations.

Brad would it be possible to make your document visible by everyone. Currently it requires moz credentials to see, it means that people outside of moz can't see it. Thanks!

Flags: needinfo?(bwerth)

(In reply to Karl Dubost💡 :karlcow from comment #18)

Brad would it be possible to make your document visible by everyone. Currently it requires moz credentials to see, it means that people outside of moz can't see it. Thanks!

https://docs.google.com/spreadsheets/d/1UPlxK9VgWo8NpE1oxoUHDKi6xcJoaA8_dGAHWClo-zY/edit?usp=sharing should allow anyone to view and comment. I will go back and edit the original posting to change the link there to this new link.

Flags: needinfo?(bwerth)
Attachment #9160532 - Attachment is obsolete: true

Setting the effective max-width to the default viewport width value this way
matches Chrome behavior and improves web compatibility.

The NoValidContent type was added to detect cases where we needed to apply
default width the viewport. That approach is no longer needed, and this
enum can be removed, as well as the code that sets and checks that enum.

Depends on D84446

Attachment #9164442 - Attachment description: Bug 1648344 Part 2: Update test expecations for viewport tags width no width or initial-scale. → Bug 1648344 Part 3: Update test expecations for viewport tags width no width or initial-scale.
See Also: → 1654697

Android wpt css-scroll-anchoring/start-edge-in-block-layout-direction.html is failing. That test has meta viewport content "user-scalable=no". This case behaves oddly in release Firefox, at least in RDM. In RDM, the page will load unscaled, with overflow. Upon reload, it will be scaled. Chrome shows it scaled from the first load. I think what is happening is that this patch changes it to be like Chrome (scaled) on first load and therefore without overflow, which would cause differing behavior in a scroll anchoring test. Does Chrome pass this test?

I think I'll disable this test in a new part of this patch, and open a bug as a follow-up.

(In reply to Brad Werth [:bradwerth] from comment #24)

Android wpt css-scroll-anchoring/start-edge-in-block-layout-direction.html is failing. That test has meta viewport content "user-scalable=no". This case behaves oddly in release Firefox, at least in RDM. In RDM, the page will load unscaled, with overflow. Upon reload, it will be scaled. Chrome shows it scaled from the first load. I think what is happening is that this patch changes it to be like Chrome (scaled) on first load and therefore without overflow, which would cause differing behavior in a scroll anchoring test. Does Chrome pass this test?

Last time I checked, Chrome doesn't run any web platform tests on mobile environments.

I think I'll disable this test in a new part of this patch, and open a bug as a follow-up.

Just adding initial-scale=1 doesn't solve the failure?

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

(In reply to Brad Werth [:bradwerth] from comment #24)

I think I'll disable this test in a new part of this patch, and open a bug as a follow-up.

Just adding initial-scale=1 doesn't solve the failure?

If it didn't help, minimum-scale=1 should work.

Blocks: 1654910

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

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

(In reply to Brad Werth [:bradwerth] from comment #24)

I think I'll disable this test in a new part of this patch, and open a bug as a follow-up.

Just adding initial-scale=1 doesn't solve the failure?

If it didn't help, minimum-scale=1 should work.

It may help -- I would have to push another try build to check. Even if it does, we would then have to make the decision of whether we propose a change to the test or change our implementation or both. Let's just handle it in follow-up Bug 1654910.

Re-trying with Part 4 applied (wpt test disabled on Android): https://treeherder.mozilla.org/#/jobs?repo=try&revision=272a31a2bd49b6e33a98f2ba3808aad23fac2b92

(In reply to Brad Werth [:bradwerth] from comment #27)

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

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

(In reply to Brad Werth [:bradwerth] from comment #24)

I think I'll disable this test in a new part of this patch, and open a bug as a follow-up.

Just adding initial-scale=1 doesn't solve the failure?

If it didn't help, minimum-scale=1 should work.

It may help -- I would have to push another try build to check. Even if it does, we would then have to make the decision of whether we propose a change to the test or change our implementation or both. Let's just handle it in follow-up Bug 1654910.

I am pretty sure that the test should be changed. It has html { width: 200vw; height: 200vh;} style and tried to scroll both inline and block directions. If the content fits to the window size, scrolling will not work as expected.

Attachment #9164442 - Attachment description: Bug 1648344 Part 3: Update test expecations for viewport tags width no width or initial-scale. → Bug 1648344 Part 3: Update test expectations for viewport tags width no width or initial-scale.
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/757734d716e2
Part 1: Apply default viewport width in more circumstances, matching Chrome. r=hiro
https://hg.mozilla.org/integration/autoland/rev/5900b99f8c02
Part 2: Cleanup Document to remove NoValidContent viewport type. r=hiro
https://hg.mozilla.org/integration/autoland/rev/eeeaa4833c44
Part 3: Update test expectations for viewport tags width no width or initial-scale. r=hiro
https://hg.mozilla.org/integration/autoland/rev/35d658861d6e
Part 4: Disable a wpt test on android. r=hiro

(In reply to Brad Werth [:bradwerth] from comment #24)

Android wpt css-scroll-anchoring/start-edge-in-block-layout-direction.html is failing. That test has meta viewport content "user-scalable=no".

FYI I made this change in bug 1648796 for desktop zooming work.

Tested on Nightly 7/29 with Google Pixel 3 (Android 11), LG G7 FIT (Android 8), and Huawei MediaPad M2 (Android 5.1.1) and improvement can be seen after following the steps from the description. It is not like on Chrome, the page is still zoomed comparing the browsers.

Brad, we can consider this fixed based on the screenshot attached and testing results?

Flags: needinfo?(bwerth)

(In reply to Sorina Florean [:sflorean] from comment #34)

Tested on Nightly 7/29 with Google Pixel 3 (Android 11), LG G7 FIT (Android 8), and Huawei MediaPad M2 (Android 5.1.1) and improvement can be seen after following the steps from the description. It is not like on Chrome, the page is still zoomed comparing the browsers.

Brad, we can consider this fixed based on the screenshot attached and testing results?

Thanks for testing this on https://m.mail.tim.it/. My testing is done on macOS in Firefox with Responsive Design Mode and in Chrome with their Device Mode. For these simulated modes, I see identical rendering, both of which similar to the screenshot you posted. If you are getting different visuals, what does the other screenshot look like?

The page has a meta viewport content of user-scalable=no, user-scalable=false, user-scalable=0. I believe we still have some webcompat issues with user-scalable=no and I opened Bug 1654910 as a follow-up to address them. If you are getting differing behavior of FF vs Chrome, then adding your Steps to Reproduce to that bug would be helpful.

Flags: needinfo?(bwerth)

I believe the issue Sorina is mentioning is that the bottom footer (the blue bar) is covered by the bottom dynamic toolbar and the bottom dynamic toolbar. Though I haven't checked the content metrics (document's height, etc.) yet, presumably it will be fixed by bug 1650644.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: