Closed Bug 1563649 Opened 1 year ago Closed 1 year ago

Computed value of 'bottom' for a position:fixed element is different in Firefox than in Chrome for Android

Categories

(Core :: Layout, defect, P3)

68 Branch
Other
Android
defect

Tracking

()

VERIFIED FIXED
mozilla71
Webcompat Priority ?
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- verified
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- verified
firefox71 --- verified

People

(Reporter: ksenia, Assigned: hiro)

References

(Blocks 1 open bug, Regression, )

Details

(Keywords: regression, Whiteboard: [geckoview:p3], [wptsync upstream], [qa-triaged])

Attachments

(2 files, 1 obsolete file)

Attached file 34281.html

As reported here:
https://github.com/webcompat/web-bugs/issues/34281

Steps to reproduce:

Go to https://www.cleanmeats.com.au/2019/07/01/out-of-body-experience-shiok-meats-and-their-lab-grown-seafood/ in Firefox for Android or in Nightly desktop RDM and observe the behavior.

Expected:
Functional site

Actual:
Site is covered by a social panel and unusable

Regression points to:
222:01.32 INFO: Last good revision: 4b74d76e55a819852c8fa925efd25c57fdf35c9d
222:01.32 INFO: First bad revision: 96d77e9c3f2985c2b702c0910e0da686bee54105
222:01.32 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4b74d76e55a819852c8fa925efd25c57fdf35c9d&tochange=96d77e9c3f2985c2b702c0910e0da686bee54105

Site is relying on the computed bottom value of an element with class .g1-sharebar to do certain calculations and as a result applying top: 0px to it. That is causing element with .g1-sharebar to cover the whole screen.

I've attached a reduced test case. It has content on the page that is wider than the screen. The page looks the same in Firefox and Chrome, however when inspecting a computed value of bottom for an element with id=test, the values are different.

Botond, the suggestion of using minimum-scale=1.0 from here https://bugzilla.mozilla.org/show_bug.cgi?id=1557707 fixes the issue, however I wonder if computed values should match the actual values, in this case bottom: 0px?

Flags: needinfo?(botond)

This seems to be a bug. We have to adjust the value somewhere in GetUsedAbsoluteOffset in the case where we expanded the layout viewport size.

Thanks for the reduced testcase!

(In reply to Ksenia Berezina [:ksenia] from comment #1)

Botond, the suggestion of using minimum-scale=1.0 from here https://bugzilla.mozilla.org/show_bug.cgi?id=1557707 fixes the issue, however I wonder if computed values should match the actual values, in this case bottom: 0px?

I agree, we should be reporting bottom: 0px here.

Flags: needinfo?(botond)
Priority: -- → P3

I have a patch for this, but I don't know who is the right person to ask review.

Whiteboard: [geckoview:p3]

The patch looks sensible to me fwiw (test and comment in nsComputedDOMStyle missing). I would've maybe made the method a method in ViewportFrame or such instead of PresShell since it seems easy to misuse otherwise. But it is the right fix.

Assignee: nobody → hikezoe.birchill

Happy to take a patch for 70, but since this is already triaged and set to P3 priority I'm setting it as fix-optional.
That will remove the bug from weekly regression triage.

Sean, can you help find a reviewer for the patch? Thanks!

On IRC, Botond suggested me Markus for the reviewer. I will revise the patch as Emilio suggested in comment 5 (I did want to avoid calling doQueryFrame there but it's not a big deal) and send the review request to Markus.

Flags: needinfo?(svoisen)
Attachment #9079245 - Attachment is obsolete: true
Attachment #9085384 - Attachment description: Bug 1563649 - As with display list item and reflow for position:fixed elements, use the adjusted layout viewport size getComputedValue() r?mstange → Bug 1563649 - As with display list item and reflow for position:fixed elements, use the adjusted layout viewport size getComputedStyle() r?mstange
Pushed by hikezoe.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bad311a6808c
As with display list item and reflow for position:fixed elements, use the adjusted layout viewport size getComputedStyle() r=mstange
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18863 for changes under testing/web-platform/tests
Whiteboard: [geckoview:p3] → [geckoview:p3], [wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Upstream PR merged by moz-wptsync-bot

Hiro, should we uplift your position:fixed fix from GeckoView 71 Nightly to 70 Beta? This bug is marked as a regression (in Fennec and GeckoView 67), but it is also a P3, suggesting it is not an urgent problem.

btw, since Fennec is now in maintenance mode in ESR 68, fixes in Gecko 71 will not automatically ride to Fennec. If we need to fix this regression in Fennec, we'll need to uplift the fix to ESR 68.

Flags: needinfo?(hikezoe.birchill)

Comment on attachment 9085384 [details]
Bug 1563649 - As with display list item and reflow for position:fixed elements, use the adjusted layout viewport size getComputedStyle() r?mstange

Beta/Release Uplift Approval Request

  • User impact if declined: It depends on what the site in question does, one of the sites reported on webcompat.com is rendered as blank
  • 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): It just tweaks values by getComputedStyle for position:fixed elements, it shouldn't cause any serious issues, crashes etc.
  • String changes made/needed: none

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is one of mobile web compat issues we've been tackling and worth fixing in Fennec
  • User impact if declined: Same as above the description for beta uplift
  • Fix Landed on Version: 71
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Same as above the description for beta uplift
  • String or UUID changes made by this patch: none
Flags: needinfo?(hikezoe.birchill)
Attachment #9085384 - Flags: approval-mozilla-esr68?
Attachment #9085384 - Flags: approval-mozilla-beta?

Thanks you, Chris. Indeed we should uplift this to fennec.

Comment on attachment 9085384 [details]
Bug 1563649 - As with display list item and reflow for position:fixed elements, use the adjusted layout viewport size getComputedStyle() r?mstange

Fairly bad webcompat issue, let's uplift for beta 6. And for ESR for the next Fennec beta build.

Attachment #9085384 - Flags: approval-mozilla-esr68?
Attachment #9085384 - Flags: approval-mozilla-esr68+
Attachment #9085384 - Flags: approval-mozilla-beta?
Attachment #9085384 - Flags: approval-mozilla-beta+
Flags: qe-verify+
Whiteboard: [geckoview:p3], [wptsync upstream] → [geckoview:p3], [wptsync upstream], [qa-triaged]

Hi!
Verified as fixed on Nightly 71.0a1 (2019-09-15) and 70.0b7 with OnePlus 5T (Android 9), Huawei Honor 8 (Android 7.0), Prestigio Grace X5 (Android 4.4.2).
I will mark this as verified on Firefox 71 and Firefox 70.
Thanks!

I have tested the issue on Beta 68.2b3 using a Samsung Galaxy S9 (Android 8.0.0) and the issue no longer occurs, I will mark it accordingly.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.