Closed Bug 1328065 Opened 7 years ago Closed 7 years ago

It's not possible to scroll html pages in parent process using mouse wheel when mouse is placed over scrollbar, if touch events are disabled

Categories

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

48 Branch
Unspecified
All
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: arni2033, Assigned: botond)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(5 files)

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
0. Set mouse option "when I rotate mouse wheel" in your OS to "scroll by 1 page"
1. Open about:support , wait until it's loaded
2. Hover mouse over vertical scrollbar
3. Rotate mouse wheel down

STR_2:
0. Set mouse option "when I rotate mouse wheel" in your OS to "scroll by 1 page"
1. Open url  data:text/html,<body style="height:10000px">  in non-e10s window, wait until it's loaded
2. Hover mouse over vertical scrollbar
3. Rotate mouse wheel down

AR:  No visible action
ER:  The page should scroll to the bottom

This was regressed twice:   [1] (2015-12-02 - 2015-12-03), then [2] (2016-05-24 - 2016-05-25)
[1] Regression from bug 1228028:
    Scrolling stopped working when mouse is placed above scrollbar, but not above scrollbar thumb.
> regression range:   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0dbac1acaca7ae2a767604fdbcf5105853518c88&tochange=f7ee42669b87f05416eb785ab9db27b5c66de1a3

[2] Regression from bug 1203140:
    Scrolling stopped working completely when mouse is placed above scrollbar
> regression range:   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=16b370b80a93eaf19a255945002c9a0c617c76db&tochange=7d77ceeacfa6f625623f49cca04270e9e9a4b282
No longer blocks: 1277113
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
(In reply to arni2033 [Please stop 'improving' Firefox] from comment #0)
> 2. Hover mouse over vertical scrollbar

Are you hovering over the scroll thumb? Or the scrollbar track outside of the thumb? I can scroll fine if I hover the scroll thumb but not if I hover the scrollback track.
Flags: needinfo?(arni2033)
>>>   My Info:   Win7_64, Nightly 53, 32bit, ID 20161229030206 (2016-12-29)
Reproducible in the same way I reported it (i.e. scrolling doesn't work when I hover mouse over any part of scxrollbar).  (I occasionally found mouse here)
However, you haven't provided any info about OS + version you are testing.
Note that I can't randomly test any of my 1900 bugs for you at any time.
Flags: needinfo?(arni2033)
(In reply to arni2033 [Please stop 'improving' Firefox] from comment #2)
> However, you haven't provided any info about OS + version you are testing.

I was testing on Windows 10, latest Firefox Nightly 53 build.

> Note that I can't randomly test any of my 1900 bugs for you at any time.

In this case I wasn't asking you to retest, just trying to get clarification on the STR. It's not obvious to me why it reproduces for you but not for me - I must be doing something different, and I'd like to figure out what that is.
OS: Unspecified → Windows 7
Priority: -- → P3
Whiteboard: [gfx-noted]
Version: Trunk → 48 Branch
Ah, the difference between our setups is that I'm on a touch enabled device and you are (probably) not. If I set dom.w3c_touch_events.enabled=0 then I can reproduce the problem.
OS: Windows 7 → All
Summary: [APZ] It's not possible to scroll html pages in parent process (e.g. about:support - Troubleshooting Information) using mouse wheel when mouse is placed above scrollbar → It's not possible to scroll html pages in parent process using mouse wheel when mouse is placed over scrollbar, if touch events are disabled
One more to take a look at :)
Flags: needinfo?(botond)
It looks like on the affected page, APZ hit testing is wrong (and has been all along), and main-thread hit testing is correct.

The "regressing" bugs are changes that caused us to hit the APZ codepath in favour of the main thread codepath in more cases.
The APZ hit testing algorithm is basically:

  (1) Do a hit test based on the hit testing tree (based on the layer tree)
  (2) Walk up the hit testing tree from the target node until you find a node
      with an APZC, you leave the layers id.
  (3) If (2) didn't produce an APZC, use the root APZC for the target node's
      layers id.

Scrollbar layers do not carry the scroll metadata of their scroll target, so at step (2), we do not find the scroll target's APZC.

For remote content, step (3) saves us, the root APZC for the layers id that we use as a fallback *is* the scroll target. For parent process content, step (3) makes us fall back to the parent process' root APZC, which is not scrollable, so no scrolling occurs.

I think the appropriate fix here is to adjust the hit testing algorithm on the APZ side to retarget a hit on a scrollbar layer to the scroll target's APZC.
Assignee: nobody → botond
Flags: needinfo?(botond)
(In reply to Botond Ballo [:botond] from comment #7)
>   (2) Walk up the hit testing tree from the target node until you find a node
>       with an APZC, you leave the layers id.

That should say " *or* you leave the layers id".
These patches fix the problem for me locally. Before requesting review, I'd like to do a Try push to make sure they don't break anything, and ideally, I'd like to try and write a new test for this as well.
Clarifying something from earlier:

(In reply to Botond Ballo [:botond] from comment #7)
> For remote content, step (3) saves us, the root APZC for the layers id that
> we use as a fallback *is* the scroll target.

This, of course, is only the case if the scrollbar you're over is the root content's scrollbar. In other words, subframes in the content process are also potentially affected.

In practice, subframes are only affected if the scrollbar track is layerized; otherwise, the scrollbar tracks just end up as dispatch-to-content regions in the enclosing scroll frame's PaintedLayer, and we take the main-thread codepath and do the right thing. However, subframe scrollbar tracks are only layerized on Mac, and even then only if we are using overlay scrollbars - so only those are affected.
(In reply to Botond Ballo [:botond] from comment #11)
> ideally, I'd like to try and write a new test for this

To summarize, the affected scroll frames are:

  1) RCD scroll frames in the parent process.
  2) On Mac with overlay scrollbars, all subframes.

Based on a discussion with Markus, it appears (1) is tricky to materialize in a test, and (2) is as well because the Mac testers do not use overlay scrollbars due to them making tests flaky.

So instead, I'm thinking of a hacky test approach where I add a test-only pref to always layerize subframe scrollbar tracks, and then write a plain mochitest where you try to scroll the wheel over the track of a subframe.
Attachment #8837811 - Flags: review?(mstange)
Comment on attachment 8837811 [details]
Bug 1328065 - Record the target scroll id of scrollbar containers in the layer tree.

https://reviewboard.mozilla.org/r/112810/#review115126
Attachment #8837811 - Flags: review?(mstange) → review+
Comment on attachment 8838748 [details]
Bug 1328065 - Add a pref to always layerize the scrollbar track, for test purposes.

https://reviewboard.mozilla.org/r/113550/#review115128
Attachment #8838748 - Flags: review?(mstange) → review+
Comment on attachment 8838747 [details]
Bug 1328065 - Better document scrollbar-related fields in HitTestingTreeNode.

https://reviewboard.mozilla.org/r/113548/#review115226
Attachment #8838747 - Flags: review?(bugmail) → review+
Comment on attachment 8837812 [details]
Bug 1328065 - Modify APZ hit testing to target the content scrolled by the scrollbar when a scrollbar is hit.

https://reviewboard.mozilla.org/r/112812/#review115228
Attachment #8837812 - Flags: review?(bugmail) → review+
Comment on attachment 8838749 [details]
Bug 1328065 - Add a mochitest for scrolling over the scrollbar of a subframe.

https://reviewboard.mozilla.org/r/113552/#review115230

::: gfx/layers/apz/test/mochitest/helper_scroll_over_scrollbar.html:14
(Diff revision 1)
> +function* test(testDriver) {
> +  var subframe = document.getElementById('scroll');
> +
> +  // scroll over the scrollbar, and make sure the subframe scrolls
> +  var scrollPos = subframe.scrollTop;
> +  yield moveMouseAndScrollWheelOver(subframe, 195, 100, testDriver);

I'd feel slightly better if you used dynamically placed the mouse on the scrollbar, so that it works regardless of how wide the scrollbar is. I think using x= "(200 + subframe.clientWidth) / 2" instead of "195" should do it. There's a couple of other APZ tests that do something similar (grep for clientWidth and you should find them).
Attachment #8838749 - Flags: review?(bugmail) → review+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ff84674e9e0
Record the target scroll id of scrollbar containers in the layer tree. r=mstange
https://hg.mozilla.org/integration/autoland/rev/a6eebf52669f
Better document scrollbar-related fields in HitTestingTreeNode. r=kats
https://hg.mozilla.org/integration/autoland/rev/b27f0473cd58
Modify APZ hit testing to target the content scrolled by the scrollbar when a scrollbar is hit. r=kats
https://hg.mozilla.org/integration/autoland/rev/2d924269745a
Add a pref to always layerize the scrollbar track, for test purposes. r=mstange
https://hg.mozilla.org/integration/autoland/rev/70da9e298571
Add a mochitest for scrolling over the scrollbar of a subframe. r=kats
Addressed review comment and landed.
I'm assuming this is too high-risk for uplift to 52 at this point. Did we want to consider backporting to 53 or let it ride the trains?
Flags: needinfo?(botond)
Flags: in-testsuite+
I'm happy to uplift this to 53 after letting it bake on nightly for a few days.

Leaving needinfo so I don't forget.
Comment on attachment 8837811 [details]
Bug 1328065 - Record the target scroll id of scrollbar containers in the layer tree.

Request for aurora approval, applies to the entire patch series.

Approval Request Comment
[Feature/Bug causing the regression]:
  Initially regressed by APZ.
  Bugs 1203140 and 1228028 made it worse by making the APZ codepath
  be taken in more situations.

[User impact if declined]:
  With the mouse placed over a vertical scrollbar, mousewheel scrolling
  will not work in two situations:

    (1) Pages that run in the parent process, such as about:support.

    (2) On Mac with overlay scrollbars only, all scrollable subframes.

[Is this code covered by automated tests?]:
  Yes, a new mochitest was added.

[Has the fix been verified in Nightly?]:
  Yes, by me.

[Needs manual test from QE? If yes, steps to reproduce]: 
  No.

[List of other uplifts needed for the feature/fix]:
  None that I'm aware of.

[Is the change risky?]:
  Low to moderate risk.

[Why is the change risky/not risky?]:
  Regressions in APZ hit testing can have highly user-visible effects.
  While that means changes to it are risky, it also means bad regressions
  would likely have been encountered and caught already. Moreover, the
  change made in this bug is a targeted change affecting scrollbars only.

[String changes made/needed]:
  None.
Flags: needinfo?(botond)
Attachment #8837811 - Flags: approval-mozilla-aurora?
We're in the last week (last couple of days) of the aurora cycle, so I think this long standing regression can wait to ship with 54. Good that we're fixing it but since the bug doesn't have a lot of duplicates let's give it a bit more baking time.
Successfully reproduce this bug on Nightly 49.0a1 (2016-05-26) (Build ID: 20160526030223) by the following Comment 0's instruction!

This Bug is now Verified as Fixed on Latest Firefox Nightly 54.0a1 (2017-03-02) (64-bit)

Build ID: 20170302110226
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
OS: Linux 4.8.0-39-generic; Ubuntu 16.04.2 (64 Bit)
QA Whiteboard: [bugday-20170301]
Attachment #8837811 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.