Closed Bug 1525075 Opened 5 years ago Closed 5 years ago

Yahoo Groups "Join Group" UI is offset from top corner in Nightly on Android

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- unaffected
firefox66 + fixed
firefox67 --- fixed

People

(Reporter: dholbert, Assigned: hiro)

References

Details

(Keywords: regression)

Attachments

(3 files)

STR:

  1. Visit https://login.yahoo.com/ and login to a yahoo account.
  2. Visit https://groups.yahoo.com/neo/groups/mozilla-firefox/info (or any Google Group info page)
  3. 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.

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.

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.)

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.)

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.)

I'm bisecting with mozregression right now. (Reduced testcase may be difficult since there seems to be some probably-obfuscated JS involved.)

(which means Firefox 66 is affected --> setting status flag)

Does anyone have reduced test cases for this? I am wondering whether the element in question is position:fixed or not.

Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)

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)

(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 zero left 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.

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.

(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.

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.

documentElement.clientWidth differs from what Chrome returns. So what happens on the site is;

  1. 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.
  2. 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.
  3. 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. :/

Blocks: 1520239
See Also: → 1443320
See Also: 1443320

[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.)

Blocks: 1520666

I'd just like to make sure how Chrome handles these cases in their code.

Flags: needinfo?(hikezoe)

Resetting the priority of the bug to P1 since it sounds like you'd like this to be fixed for 66.

Priority: P3 → P1

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).

Flags: needinfo?(hikezoe)

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.

Now documentElement-clientWidth-on-minimum-scale-size.tentative.html passes without any failures;

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f5b28820cc77a67167d85d7b33cf8ebfed21cca&selectedJob=227837760

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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

Bug 1423013

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

Attachment #9043192 - Flags: approval-mozilla-beta?

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.

Attachment #9043192 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: