Closed Bug 1593752 Opened 5 years ago Closed 5 years ago

Scrolling down is buggy using Nightly channel

Categories

(Core :: Layout: Block and Inline, defect, P2)

Product:
Core
Component:
Layout: Block and Inline
Version:
71 Branch
Platform:
All
Unspecified
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla72
Tracking Flags:
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 + verified
firefox72 --- verified

People

(Reporter: mvcmaciel, Assigned: TYLin)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1593752 - When inserting canonical <body> element, reframe root element only if their used WritingModes are different.
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
Details | Review

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

  1. Access "https://www.osram.com.br/cb/"
    using Firefox Nightly (72.0a1 (2019-11-04) (64-bit))
    on Windows 10 (I'll try later using Linux)

  2. Scroll down the page

Actual results:

Scrolling down is very buggy, and not smooth

Expected results:

Using Firefox Stable (70.0.1 (64-bits)), scrolling down is smooth

Status: UNCONFIRMED → NEW
status-firefox70: --- → unaffected
status-firefox71: --- → affected
status-firefox72: --- → affected
status-firefox-esr68: --- → unaffected
Component: Untriaged → Layout: Block and Inline
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Regressed by: 1102175
Hardware: Unspecified → All
Version: 72 Branch → 71 Branch

We may be reframing the root accidentally more often than before.

Flags: needinfo?(aethanyc)

Yeah, we're hitting the WipeInsertionParent: Root case a lot according to the profile.

The page has:

function(e) {
        e.viewportSize = {}, e.viewportSize.getHeight = function() {                                                                                                                                                                          
            return t("Height")
        }, e.viewportSize.getWidth = function() {
            return t("Width")
        };
        var t = function(t) {
            var n, i = t.toLowerCase(),
                r = e.document,
                o = r.documentElement;
            if (void 0 === e["inner" + t]) n = o["client" + t];
            else if (e["inner" + t] != o["client" + t]) {
                var a = r.createElement("body");
                a.id = "vpw-test-b", a.style.cssText = "overflow:scroll";
                var s = r.createElement("div");
                s.id = "vpw-test-d", s.style.cssText = "position:absolute;top:-1000px", s.innerHTML = "<style>@media(" + i + ":" + o["client" + t] + "px){body#vpw-test-b div#vpw-test-d{" + i + ":7px!important}}</style>", a.appendChild(s), o.insertBefore(a, r.head), n = 7 == s["offset" + t] ? o["client" + t] : e["inner" + t], o.removeChild(a)
            } else n = e["inner" + t];
            return n
        }
    }(this)

And calls that during a scroll event listener...

A bit unminified it'd be:

window.viewportSize = {};
window.viewportSize.getHeight = function() {
  return test("Height")
}
window.viewportSize.getWidth = function() {
  return test("Width")
};
function test(property) {
  let cssPropertyName = property.toLowerCase();
  let documentElement = document.documentElement;
  if (window["inner" + property] == undefined)
    return documentElement["client" + property];
  if (window["inner" + property] == documentElement["client" + property])
    return window["inner" + property];
  let body = document.createElement("body");
  body.id = "vpw-test-b";
  body.style.cssText = "overflow:scroll";
  let div = document.createElement("div");
  div.id = "vpw-test-d";
  div.style.cssText = "position:absolute;top:-1000px";
  div.innerHTML = "<style>@media(" + cssPropertyName + ":" + documentElement["client" + property] + "px){body#vpw-test-b div#vpw-test-d{" + cssPropertyName + ":7px!important}}</style>";
  body.appendChild(div);
  documentElement.insertBefore(body, document.head);
  let offset = 7 === div["offset" + property] ? documentElement["client" + property] : window["inner" + property];
  documentElement.removeChild(a);
  return offset;
}

[Tracking Requested - why for this release]: It's not clear to me what the best fix for this is, but it'd be unfortunate to ship this regression, as it looks pretty bad. We also don't know how many websites have similar (silly) code.

tracking-firefox71: --- → ?
Priority: -- → P2

This looks pretty similar to bug 1592902, too, just that before we weren't hitting the "changing the viewport element" condition.

See Also: → 1592902

This looks really bad ... I was re-thinking the review comment for Bug 1102175 Part 1. Maybe we can move the <body> reframing logic in WipeInsertionParent to WipeContainingBlock. There we compare the <body>'s used WritingMode and its parent's WritingMode, and only do reframing if the two WritingMode are different.

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

Tracking for 71. Should we backout bug 1102175 in beta or is an easy fix upliftable?

tracking-firefox71: ? → +
Flags: needinfo?(aethanyc)

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #6)

This looks really bad ... I was re-thinking the review comment for Bug 1102175 Part 1. Maybe we can move the <body> reframing logic in WipeInsertionParent to WipeContainingBlock. There we compare the <body>'s used WritingMode and its parent's WritingMode, and only do reframing if the two WritingMode are different.

So this one is weird, because you're inserting a <body style="overflow: scroll">, effectively. So you also need to propagate the scrollbars to the viewport. In the general case where the <body> comes before another body that has a non-visible overflow value, you also need to reframe that second <body>, though in this case the regular <body> has overflow: visible so we may be able to not reframe here.

But handling the general case right is gnarly... :(

I didn't dig into the original script, but per comment 2, the test function removes the newly inserted <body> at the end, so maybe it's ok not to reframe at all like before bug 1102175 was introduced.

(In reply to Pascal Chevrel:pascalc from comment #7)

Tracking for 71. Should we backout bug 1102175 in beta or is an easy fix upliftable?

The fix is small enough to be uplifted. I'll request an uplift once it lands on mozilla-central.

Flags: needinfo?(aethanyc)
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f2c17f2a388c
When inserting canonical <body> element, reframe root element only if their used WritingModes are different. r=emilio

https://hg.mozilla.org/mozilla-central/rev/f2c17f2a388c

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox72: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Flags: in-testsuite+

Comment on attachment 9106662 [details]
Bug 1593752 - When inserting canonical <body> element, reframe root element only if their used WritingModes are different.

Beta/Release Uplift Approval Request

  • User impact if declined: The user cannot scroll the page smoothly as describe in comment 0.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: I can verify that the site in comment 0 can now be scrolled smoothly on Nightly 2019-11-06, but it'll be good if QE can also verify the patch is working on beta.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change tightens the condition that checks whether to re-construct the entire frame tree, and the change restores to the status quo prior to fixing bug 1102175 (which introduces this bug). No new logic is added.
  • String changes made/needed: None
Attachment #9106662 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9106662 [details]
Bug 1593752 - When inserting canonical <body> element, reframe root element only if their used WritingModes are different.

Fix for a 71 layout perf regression, covered by tests and evaluated as low risk, uplift approved for 71 beta 8, thanks.

Attachment #9106662 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Reproduced on Windows 10 / Ubuntu 18.04 using Firefox Nightly 72.0a1(20191105095755) and Firefox Beta 71.0b7(20191104135555)
Verified fixed using Firefox Nightly 72.0a1(20191106215426) and Firefox Beta 71.0b8(20191106230619)

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
status-firefox71: fixed → verified
status-firefox72: fixed → verified
Flags: qe-verify+
Blocks: 1594297
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: