Closed Bug 1522964 Opened 7 months ago Closed 7 months ago

Re-enable scroll anchoring on Fennec with gofaster intervention

Categories

(Core :: Layout: Scrolling and Overflow, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

Attachments

(1 file)

With the gofaster intervention in bug 1522755, let's re-enable scroll anchoring on Fennec for more testing.

I guess we're hoping there aren't many usages of Infinity.js in the wild (besides eBay) that our mobile users are likely to run into, yes?

I did a quick sanity check by looking at AirBnB (the authors of the library, before it was deprecated), and I didn't find any usages on 2 different infinitely-scrolling-sites there (and I couldn't trigger issues, with scroll anchoring enabled.

And I did a quick github search ( https://github.com/search?l=JavaScript&q=infinity.js&type=Code and https://github.com/search?q=%22infinity.js%22&type=Code ) and didn't turn up anything that looks like a match, on the first page of results (where I'd expect any actual exact-text "infinity.js" matches to turn up). I don't have a lot of confidence in my github-search-skills though.

(In reply to Daniel Holbert [:dholbert] from comment #2)

I guess we're hoping there aren't many usages of Infinity.js in the wild
(besides eBay) that our mobile users are likely to run into, yes?

I did a quick sanity check by looking at AirBnB (the authors of the library,
before it was deprecated), and I didn't find any usages on 2 different
infinitely-scrolling-sites there (and I couldn't trigger issues, with scroll
anchoring enabled.

And I did a quick github search (
https://github.com/search?l=JavaScript&q=infinity.js&type=Code and
https://github.com/search?q=%22infinity.js%22&type=Code ) and didn't turn up
anything that looks like a match, on the first page of results (where I'd
expect any actual exact-text "infinity.js" matches to turn up). I don't
have a lot of confidence in my github-search-skills though.

Yes, that's the hope. It's a stopgap that at a minimum should let us get some beta testing.

I'm still looking at bug 1520666 to learn more about this, as it appears the diagnosis might be more complicated than originally thought (as Chrome fires the resize event too with their dynamic toolbar). But I don't want to rely on finding out an answer before early beta.

(In reply to Ryan Hunt [:rhunt] from comment #3)

(In reply to Daniel Holbert [:dholbert] from comment #2)

Yes, that's the hope. It's a stopgap that at a minimum should let us get some beta testing.

Yes, ideally we don't ship to release with this workaround and we can come up with a better solution in the bug 1520666 investigation. In the meantime, it will be nice to let beta users try the feature on Fennec.

Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/37103bfa3dca
Re-enable scroll anchoring on Fennec with gofaster intervention. r=dholbert
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Ryan, did we want to uplift this to beta 66? Bug 1522755 got beta uplift.

Flags: needinfo?(rhunt)

Comment on attachment 9039199 [details]
Bug 1522964 - Re-enable scroll anchoring on Fennec with gofaster intervention. r?dholbert

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

1520666, 1522755

User impact if declined

We will miss out on testing scroll anchoring on Fennec Beta.

Is this code covered by automated tests?

No

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)

We've already had scroll anchoring enabled on nightly (just temporarily disabled for the ebay scrolling regression) and the gofaster intervention has been tested on nightly to fix the blocking issue. The risk is that we'll find more scrolling regressions that aren't easily fixed, in which case we'll want to disable scroll anchoring again. But we want to find these issues by testing on a wider audience.

String changes made/needed

None

Flags: needinfo?(rhunt)
Attachment #9039199 - Flags: approval-mozilla-beta?

Yes we do. I should have requested uplift before PTO.

Comment on attachment 9039199 [details]
Bug 1522964 - Re-enable scroll anchoring on Fennec with gofaster intervention. r?dholbert

Fennec scroll anchoring fix, let's uplift for beta 6.

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