Closed Bug 1582585 Opened 5 years ago Closed 5 years ago

Middle-mouse autoscroll scrolls non-scrollable iframes

Categories

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

Product:
Core
Component:
Layout: Scrolling and Overflow
Version:
60 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: me, Assigned: emilio)

References

Details

Attachments

(6 files, 1 obsolete file)

Testcase
255 bytes, text/html
Details
Bug 1582585 - Factor out a bit of the logic in AutoScrollController.jsm.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1582585 - Use flattenedTreeParentNode in AutoScrollController.jsm.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1582585 - Factor out a bit more code from AutoScrollController.jsm.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1582585 - Make AutoScrollController not scroll non-scrollable windows.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1582585 - Test autoscroll for scrollable and non-scrollable iframes.
47 bytes, text/x-phabricator-request
Details | Review
Attached video Unnamed screen capture.webm (obsolete) —

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/78.0.3904.9 Safari/537.36

Steps to reproduce:

  1. Create a new Google Docs doc (docs.google.com) using any Google account
  2. Enter some text into the doc, and right click to add a comment on the text
  3. +mention your Google account in the comment (e.g., "+myaccount@gmail.com") so that you receive a comment notification in your Gmail inbox
  4. Wait ~15 minutes for the async notification to arrive in Gmail
  5. Open the Docs notification email sent from comments-noreply@docs.google.com
  6. Make sure the window height is small enough so that the email content is scrollable
  7. Click on the reply input text field to focus on the input
  8. Hit the middle mouse button to trigger auto-scroll mode

Actual results:

Autoscroll doesn't work (only seems to be scrolling by 1 pixel up and down)

See also the screencast attached as part of this bug.

(This works in other browsers like Chrome on Windows)

Expected results:

Autoscroll should work

Forgot to mention that somehow the cursor position matters whether it is inside an iframe. In the first half of the video, you can see that the mouse cursor is inside the iframe, and middle button autoscroll doesn't work; if I move the cursor out of the iframe region (in the email sender avatar column), then autoscroll works.

Component: Untriaged → Layout: Scrolling and Overflow
Product: Firefox → Core

With middle-mouse button, you mean scrolling with the wheel?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

With middle-mouse button, you mean scrolling with the wheel?

No, I mean the "autoscroll" feature Firefox has on Windows and Linux (I had to enable it in the setting on Linux: https://askubuntu.com/questions/908/firefox-middle-mouse-button-scroll).

Single press on the middle mouse button, then an anchor appears at the cursor position, and moving the cursor away from the anchor would scroll the page in the direction of from the anchor to the cursor. The anchor unfortunately doesn't appear in the screencast for some reason, but it looks like this:

https://www.howtogeek.com/howto/internet/firefox/disable-that-irritating-autoscroll-feature-in-firefox/

Ah, I see, thanks, so you need general.autoScroll=true.

I tried to construct a simple test-case based on the description and couldn't, so probably it's specific to GMail (or I wasn't using the right Firefox version, I'm using Nightly).

I tried to repro on a recent docs conversation from this week, and failed, but I'd note that the "Reply" textarea doesn't show a scrollbar for me (which may make the bug reproducible or something).

A few more questions, and one request if you have the time (as I'll be traveling today so probably won't have a good enough connection to investigate this properly):

  • Could you tell us which version of Firefox can you reproduce the bug on?
  • Could you attach your about:support, just in case it depends on the compositor and such?
  • Does this reproduce with apz.autoscroll.enabled=false?
  • If you have the time: Is there any chance that you could check whether this works in older Firefox versions either downloading them from using mozregression? You can do that with, e.g., mozregression --launch 60 to launch Firefox 60. If you have more time and it works in older versions, running a mozregression --good <version> may help in identifying what broke it.

Thanks!

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Ah, I see, thanks, so you need general.autoScroll=true.

I tried to construct a simple test-case based on the description and couldn't, so probably it's specific to GMail (or I wasn't using the right Firefox version, I'm using Nightly).

Yeah, it's specific to the Gmail DOM. I tried to come up with a minimal repro too but failed.

I tried to repro on a recent docs conversation from this week, and failed, but I'd note that the "Reply" textarea doesn't show a scrollbar for me (which may make the bug reproducible or something).

The scrollbar issue seems like a different bug. I can only reproduce that in Firefox ESR 60 on Linux but not latest Firefox on other platforms. I'm still not sure if that's a Firefox bug yet, but I may file a bug later if I think that's the case.

A few more questions, and one request if you have the time (as I'll be traveling today so probably won't have a good enough connection to investigate this properly):

  • Could you tell us which version of Firefox can you reproduce the bug on?

When I took the screencast, I was using Firefox ESR 6.0 on Linux. I just tried using Firefox 68.0.2 on WIndows and can also reproduce this bug (with no scrollbar). However, I just upgraded to Firefox 69 on Windows, and I couldn't repro anymore! It looks like it wasn't recently fixed between 68 and 69.

  • Could you attach your about:support, just in case it depends on the compositor and such?
  • Does this reproduce with apz.autoscroll.enabled=false?
  • If you have the time: Is there any chance that you could check whether this works in older Firefox versions either downloading them from using mozregression? You can do that with, e.g., mozregression --launch 60 to launch Firefox 60. If you have more time and it works in older versions, running a mozregression --good <version> may help in identifying what broke it.

Looks like we narrowed this down to between 68 and 69 by accident. :)

Thanks!

s/it wasn't recently fixed/it was recently fixed

If you could find what fixed it with mozregression --good 69 --bad 68 --find-fix it'd be useful. It may be that you're, e.g, getting a different compositor on 69 (as webrender could be auto-enabled), or such.

Oh, a better command may be mozregression --pref general.autoScroll:true [...], which saves the hassle of having to enable it everywhere.

I'll try to repro on 68 here on linux and see if I can narrow it down.

Ah, I could repro on 68 indeed. Will try to do the above myself.

So this is the result I get from the bisection: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=237163b50c8cd1d309592b3a8c30db6ff7452e3d&tochange=5277a23512a6bd556d25531a67d5e62da9f30948

But that's an odd one... Can anybody reproduce the same?

This is the command I used:

$ mozregression --find-fix --pref general.autoScroll:true -a https://gmail.com --repo=mozilla-central --good=69 --bad=68

And if you want something faster to avoid logging in so many times as I had to:

$ mozregression --find-fix --pref general.autoScroll:true -a https://gmail.com --repo=mozilla-central --good=2019-07-08 --bad=2019-06-14
Depends on: 1561440

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

And if you want something faster to avoid logging in so many times as I had to:

Note, you can use --profile-persistence clone-first to get mozregression to reuse the same profile across runs and remember credentials.

I did try that (well, --profile-persistence reuse), but that allows downgrading the profile, and I was hitting broken builds due to it.

Ok, so by running this on all the relevant iframes and documents in gmail, I think that's plausible. In particular:

for (let element of document.querySelectorAll("*"))
  for (let attr of element.attributes)
    if (/^[0-9]+\.[0-9]*%?$/.test(attr.value.trim()))
      console.log(element, attr);

I don't know why or how, but, for example, if on the top document, before focusing the textarea, you run it, you don't get anything interesting (only one <svg> with version="1.1".

Once you focus the textarea, however, it yields the iframe hosting the email, with a height="2460.800048828125" attribute. That is affected by bug 1561440, and if I remove the fractional part, then I can reproduce the bug on Nightly.

Losing the fractional part makes the iframe scrollable, and that's what's happening here... So the underlying bug is still present, I think.

Here's a reduced test-case. Using middle-finger scrolling shouldn't scroll the iframe, it should instead scroll the top level document. But it does.

<!doctype html>
<style>
  .padding { height: 200vh; }
</style>
<iframe height=50 style="width: 100%; overflow: hidden;" scrolling="no" srcdoc="<div style='height: 50.8px'>Foo"></iframe>
<div class="padding"></div>

Botond, do you know how this feature differs from the regular scrolling mechanisms, and where does it live?

This is probably an easy fix, but I have no clue where the code for this lives. Feel free to just return the needinfo if you don't know it off-hand and I'll do the digging when I get to it.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(botond)
Summary: Middle-mouse autoscroll is broken after focusing on input inside iframe → Middle-mouse autoscroll scrolls non-scrollable iframes
Attached file Testcase —

(for easier viewing)

Attachment #9094046 - Attachment is obsolete: true

The autoscrolling implementation lives in AutoScrollController.jsm. The scrolling animation itself can be done either via APZ (AutoscrollAnimation.cpp) or on the main thread via requestAnimationFrame(), but the choice of which element to scroll is made in AutoScrollController.jsm in either case. That choice is made using some custom JS logic, it's possible that gives different results in some scenarios than the platform hit test we do for e.g. wheel scrolling.

To clarify, is there a bug here even with bug 1561440 is fixed?

Flags: needinfo?(botond)

(In reply to Botond Ballo [:botond] from comment #16)

The autoscrolling implementation lives in AutoScrollController.jsm. The scrolling animation itself can be done either via APZ (AutoscrollAnimation.cpp) or on the main thread via requestAnimationFrame(), but the choice of which element to scroll is made in AutoScrollController.jsm in either case. That choice is made using some custom JS logic, it's possible that gives different results in some scenarios than the platform hit test we do for e.g. wheel scrolling.

Ehmm, wow, that's a bit convoluted...

To clarify, is there a bug here even with bug 1561440 is fixed?

Yes. We shouldn't be scrolling overflow: hidden iframes. Bug 1561440 made the <iframe> not scrollable on GMail.

I wrote something.

Assignee: nobody → emilio

This makes it work with both Shadow DOM and XBL.

That is, avoid scrolling <iframe>s that have scrolling="no", for example.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b208c167f36
Factor out a bit of the logic in AutoScrollController.jsm. r=botond
https://hg.mozilla.org/integration/autoland/rev/523ac2d790f3
Use flattenedTreeParentNode in AutoScrollController.jsm. r=botond
https://hg.mozilla.org/integration/autoland/rev/8b0a53915fcb
Factor out a bit more code from AutoScrollController.jsm. r=botond
https://hg.mozilla.org/integration/autoland/rev/47a831cbcebe
Make AutoScrollController not scroll non-scrollable windows. r=botond
https://hg.mozilla.org/integration/autoland/rev/7099f0e6f0fb
Test autoscroll for scrollable and non-scrollable iframes. r=botond
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: