Yahoo Groups "Join Group" UI is offset from top corner in Nightly on Android
Categories
(Core :: Panning and Zooming, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox65 | --- | unaffected |
firefox66 | + | fixed |
firefox67 | --- | fixed |
People
(Reporter: dholbert, Assigned: hiro)
References
Details
(Keywords: regression)
Attachments
(3 files)
186.31 KB,
image/png
|
Details | |
387 bytes,
text/html
|
Details | |
47 bytes,
text/x-phabricator-request
|
lizzard
:
approval-mozilla-beta+
|
Details | Review |
STR:
- Visit https://login.yahoo.com/ and login to a yahoo account.
- Visit https://groups.yahoo.com/neo/groups/mozilla-firefox/info (or any Google Group info page)
- Click "Join Group" button at bottom right.
EXPECTED RESULTS:
Some "join group" popup content should appear and cover up the full screen.
ACTUAL RESULTS:
The popup content isn't snapped to the left edge of my viewport, and its content is awkwardly running off the left edge.
I get EXPECTED RESULTS from:
- Chrome on Android
- Firefox 65 release on Android (my phone's current beta version, from near end of the 65 beta cycle)
- Firefox Nightly 67 for Desktop, in Responsive Design Mode for Galaxy S9
I get ACTUAL RESULTS from:
- Firefox Nightly 67.0a1 (2019-02-04)
- Reference Browser 67 20190130001444
My phone hardware is a Pixel 3.
My initial guess is that this is a regression from the "visual viewport" rework that happened recently.
Reporter | ||
Comment 1•5 years ago
|
||
Actually, devtools shows that Yahoo is giving an absolutely-positioned element an explicit styling of left: 84px
in Firefox Nightly 67, vs. 0px
in Firefox 65. So that explains the layout difference, roughly.
Here's the element in question, copypasted from devtools in Firefox 65 release (though again, in nightly, we have left:84px
rather than the 0px
that's shown here):
<div id="yui_3_15_0_1_1549304840480_458"
class="yui3-widget yui3-panel yui3-widget-positioned yui3-widget-modal yui3-widget-stacked yui3-panel-focused"
style="left: 0px; top: -0.5px; z-index: 1001; height: 663px; right: 0px;" tabindex="0">
So, Yahoo must be doing some JS calculations based on viewport measurements (or something) that we expose, and something changed such that now we give them information that makes them to decide to use a nonzero left
offset, where previously they used a zero left
offset.
Reporter | ||
Comment 2•5 years ago
|
||
I'm not sure if Firefox 66 is affected or not, BTW; I just know 65 is unaffected and 67 is affected.
(I do have Firefox Beta on my phone, but it's currently at a 65 beta version, and I haven't been offered an update to beta66 in play store yet. I imagine that'll happen any day now though.)
Comment 3•5 years ago
|
||
Let's get a regression window to identify what regressed this. (The "viewport rework" is an ongoing effort consisting of many bugs; I agree that it's suspect, but I'd like to know which bug specifically.)
Comment 4•5 years ago
|
||
A testcase that's reduced enough that it doesn't require a yahoo login would also be helpful. (With such a testcase, I'd be happy to find the regression window myself.)
Reporter | ||
Comment 5•5 years ago
|
||
I'm bisecting with mozregression right now. (Reduced testcase may be difficult since there seems to be some probably-obfuscated JS involved.)
Comment hidden (obsolete) |
Reporter | ||
Comment 7•5 years ago
|
||
regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=654c9274673479fc772eef2f8a3002982e931a62&tochange=895cc0297ce2e47092dc5736cd444a573ba03ed2
--> regression from bug 1423013
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Comment 8•5 years ago
|
||
(which means Firefox 66 is affected --> setting status flag)
Assignee | ||
Comment 9•5 years ago
|
||
Does anyone have reduced test cases for this? I am wondering whether the element in question is position:fixed or not.
Reporter | ||
Comment 10•5 years ago
•
|
||
Unfortunately the page is assigning different styles in "good" vs. "bad" Firefox -- see comment 1.
The element in question is position:absolute and Yahoo gives it style="left:0px" in good Firefox versions vs. style="left: 84px" in bad Firefox versions.
(There is position:fixed content elsewhere in the page, if that matters)
Comment 11•5 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #1)
So, Yahoo must be doing some JS calculations based on viewport measurements (or something) that we expose, and something changed such that now we give them information that makes them to decide to use a nonzero
left
offset, where previously they used a zeroleft
offset.
There are certainly some measurements that bug 1423013 could have changed. scrollMaxX/Y come to mind, though there may be others.
Any such change should have made us more consistent with Chrome, not less, but I suppose the page may be doing browser-specific calculations that got messed up.
Assignee | ||
Comment 12•5 years ago
|
||
I haven't succeed in creating any reduced cases yet, but I am now suspecting the left property is calculated with window.innerWidth, which means bug 1514429.
Comment 13•5 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
I haven't succeed in creating any reduced cases yet, but I am now suspecting the left property is calculated with window.innerWidth, which means bug 1514429.
It is true that we report a different quantity for window.innerWidth
, than Chrome.
However, that's a long-standing difference. Bug 1423013 should not have changed what we report for window.innerWidth
. So that alone does not explain the problem.
Assignee | ||
Comment 14•5 years ago
|
||
yeah, so I am now reading the YUI library which is used in the site, and probably the element position is calculated based on this getXY method there are lots of suspicious things.
Assignee | ||
Comment 15•5 years ago
|
||
documentElement.clientWidth differs from what Chrome returns. So what happens on the site is;
- when the element in question is attached to the document, the element size is wider than ICB size, so we are going to use the minimum scale size in nsGfxScrollFrame.
- after the element was attached, the YUI library tries to position the element based on the documentElement.clientWidth, and unfortunately we return the minimum scale size width for the clientWidth.
- then the YUI library changes the left property to position the element at the center of the documentElement.
I thought the clientWidth value on the minimum-scale size covered by a wpt in this commit, but actually it's not. :/
Comment 16•5 years ago
•
|
||
[Tracking Requested - why for this release]:
(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
and unfortunately we return the minimum scale size width for the clientWidth.
Indeed, it looks like documentElement.clientWidth
/clientHeight
return the scroll port size, which has been modified in bug 1423013 to be the minimum scale size.
I'm going to suggest that we track this for 66, so we don't ship a change in the interpretation of clientWidth
/clientHeight
that we didn't intend in 66 only to change it back in 67.
(Another reason for tracking is that Ryan is adding a fast-path to Element::GetClientAreaRect()
in bug 1520666 (which is also tracked for 66), and the correct implementation of that will depend on whether or not this is fixed.)
Assignee | ||
Comment 17•5 years ago
|
||
I'd just like to make sure how Chrome handles these cases in their code.
Comment 18•5 years ago
|
||
Resetting the priority of the bug to P1 since it sounds like you'd like this to be fixed for 66.
Assignee | ||
Comment 19•5 years ago
|
||
The clientWidth value is based on this PageScaleConstraintsSet::GetLayoutSize() value.
The size value in question is set in PageScaleConstraintsSet::UpdatePageDefinedConstraints (it's the ICB size at this moment).
And for Android it's modified in PageScaleConstraintsSet::AdjustForAndroidWebViewQuirks, the layout width becomes the default min-width when the following conditions met (other stuffs in the function are not interesting for the clientWidth);
if (wide_viewport_quirk_enabled &&
use_wide_viewport &&
description.zoom != 1.0f &&
(description.max_width.IsAuto() ||
description.max_width.GetType() == kExtendToZoom))
The default min-width is calculated with values in viewport meta tag (or @viewport rules), the default min-width value is not affected by the minimum scale at all.
So I am going to change Element::GetClientAreaRect to return the ICBSize instead of the expanded layout viewport size.
(though I don't quite understand above the conditions (wide_viewport_quirk_enabled, etc.), it might be another cause of mismatches between Chrome, I will file a bug for it later).
Assignee | ||
Comment 20•5 years ago
|
||
This is what Chrome does.
documentElement-clientWidth-on-minimum-scale-size.tentative.html was the test
case for this but unfortunately it was disabled in bug 1515043. And it seems
that the test case failed on Android in the first place.
Assignee | ||
Comment 21•5 years ago
|
||
Now documentElement-clientWidth-on-minimum-scale-size.tentative.html passes without any failures;
Comment 22•5 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2ae0011bb82c Use the ICB size for Element.{clientWidth, clientHeight} instead of expanded the layout viewport size. r=botond
Comment 23•5 years ago
|
||
bugherder |
Assignee | ||
Comment 24•5 years ago
|
||
Comment on attachment 9043192 [details]
Bug 1525075 - Use the ICB size for Element.{clientWidth, clientHeight} instead of expanded the layout viewport size. r?botond!
Beta/Release Uplift Approval Request
Feature/Bug causing the regression
User impact if declined
User will see wrongly positioned elements or anything else calculated with the top level document's clientWidth or clientHeight
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)
This patches restored the previous values of clientWidth and Height
String changes made/needed
Comment 25•5 years ago
|
||
Comment on attachment 9043192 [details]
Bug 1525075 - Use the ICB size for Element.{clientWidth, clientHeight} instead of expanded the layout viewport size. r?botond!
Fix for recent regression, OK to uplift for beta 8.
Comment 26•5 years ago
|
||
bugherder uplift |
Updated•5 years ago
|
Description
•