1462961 - The scroll-bar in youtube's subscription panel jumps upwards when selcted+dragged

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Mayank Bansal]

Attachment: bandicam 2018-05-20 09-05-32-056.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180519220058

Steps to reproduce:

1. use nightly. Enable Wr. Restart
2. Go to youtube
3. The left panel is of the subscriptions. 
4. Click-and-hold the panel scrollbar
5. While holding, drag the scrollbar


Actual results:

scrollbar jumps up


Expected results:

not so

Comment 1

[Alice0775 White]

there are 2 regression.

#1 regression window: Scroll thumb is drawn shifted upward by approx 100px.
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7a6d3d03bb20c881d25ae7e6fdb2fbb981e335b7&tochange=cf1fdb432d46e52f795b3e253e851fbf775b2af1

Regressed by:  cf1fdb432d46	Kartikaya Gupta — Bug 1423370 - Create fewer WebRenderLayerScrollData items for transformed items. r=jrmuizel


#2 regression window: In addition to the problem of #1regression, the scroll thumb jumps to the top when drag start.
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e9ff0123aa27dcff653db1a5623d53a42aa03b9d&tochange=57fe2d9ec5549d45b33fc94389de27ca7d4e2c53

Regressed by: Bug 1449478


@Kartikaya Gupta,
The bunch of patch causes the regression. Can you please look into this?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Reduced test case

Comment 3

[Kartikaya Gupta (email:[e-mail])]

So the check at [1] is ensuring that the ancestor transform doesn't end up on the same layer as the scroll thumb data. This is desirable. But the check also ends up dropping the ancestor transform from the scroll thumb's ancestor layers (it ends up on the scroll thumb's siblings instead) and that's not desirable. We still want that transform on the ancestor chain somewhere.

i.e. it's doing this:

fixed-pos node
 |-- scrollthumb node
 |-- content node with transform

but we want something like this:
fixed-pos node
 |-- dummy node with transform
 |    |-- scrollthumb node
 |-- content node with transform

Making that happen is a little tricky :/

[1] https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/gfx/layers/wr/WebRenderScrollData.cpp#84

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment 3 isn't quite right. It's not the scrollthumb that needs fixing, it's the content. So what we want is this:

fixed-pos node
 |-- scrollthumb node
 |-- dummy node with transform
      |-- content node

Comment 5

[Kartikaya Gupta (email:[e-mail])]

To be a bit more clear, the asciigrams in comment 3 and 4 are referring to the structure of the HitTestingTreeNode on the APZ side, which is derived from the structure of the WebRenderScrollData that gets passed over. The "content node" is a HTTN with an APZC attached and the "scrollthumb node" is just a HTTN that corresponds to a scrollthumb. The key here is that when we untransform the mouse event in APZ, we use the GetTransformToThis() of the content node, which does *not* include the transform on the content node itself. But we do want the untransformation to include that transform, which is why we want to extract it out into a dummy node that's a parent of the content node. This makes the tree structure more like what it is without WR.

I have a try push with the fix going at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d82f404047a077d05a8432342fc6b80d3c6384b. I'd really like to add a test for this and other scrollbar-dragging bugs that I've fixed in this code, but until we turn on async scene building in CI the hit-testing codepath is going to be plagued by intermittent failures so I don't want to add tests until that's done. I'll file a follow-up for it.

[1] https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/gfx/layers/apz/src/APZCTreeManager.cpp#1815

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Previous attempt had some assertion failures. This one should be better:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc22adadba8adf95020e025780b470e442db2cac

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1462961 - Dump the WebRenderScrollData as well when DUMP_LISTS is defined.

Review commit: https://reviewboard.mozilla.org/r/246240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246240/

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1462961 - Refactor to save a nsDisplayTransform* instead of just the transform matrix.

Instead of just storing the gfx::Matrix from the nsDisplayTransform on
the stacking context for scrolldata use, we now store a pointer to the
entire nsDisplayTransform item. We will need this in the next patch.
This patch should not have any functional changes.

Review commit: https://reviewboard.mozilla.org/r/246242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246242/

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1462961 - Ensure the WebRenderLayerScrollData structure matches the APZ requirements.

In cases where we were carrying down a transform from a
nsDisplayTransform and then applying it to a child item's WRLSD, we
could get incorrect behaviour when that child item had a different ASR
(and therefore had a different APZ instance). This patch corrects that
behaviour by ensuring that when we run into this case we fall back to
creating two WRLSD items instead of one, where the new WRLSD is the
parent of the other one, and holds the transform. The child WRLSD has
the data from the child item's ASR.

Review commit: https://reviewboard.mozilla.org/r/246244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246244/

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8980082 [details]
Bug 1462961 - Dump the WebRenderScrollData as well when DUMP_LISTS is defined.

https://reviewboard.mozilla.org/r/246240/#review252352

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8980083 [details]
Bug 1462961 - Refactor to save a nsDisplayTransform* instead of just the transform matrix.

https://reviewboard.mozilla.org/r/246242/#review252354

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8980084 [details]
Bug 1462961 - Ensure the WebRenderLayerScrollData structure matches the APZ requirements.

https://reviewboard.mozilla.org/r/246244/#review252358

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55136cd0a8eb
Dump the WebRenderScrollData as well when DUMP_LISTS is defined. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/6c722b22ef5e
Refactor to save a nsDisplayTransform* instead of just the transform matrix. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/10409544368a
Ensure the WebRenderLayerScrollData structure matches the APZ requirements. r=jrmuizel

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/55136cd0a8eb
https://hg.mozilla.org/mozilla-central/rev/6c722b22ef5e
https://hg.mozilla.org/mozilla-central/rev/10409544368a

Comment 15

[Mayank Bansal]

confirmed fixed as of https://hg.mozilla.org/mozilla-central/rev/043e4ab6e72469ed8121f4da98dcdfef983a49d9