Closed Bug 1462961 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: WebRender, defect)

Product:
Core
Component:
Graphics: WebRender
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 --- fixed

People

(Reporter: mayankleoboy1, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(5 files)

bandicam 2018-05-20 09-05-32-056.mp4
997.19 KB, video/mp4
Details
Reduced test case
287 bytes, text/html
Details
Bug 1462961 - Dump the WebRenderScrollData as well when DUMP_LISTS is defined.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1462961 - Refactor to save a nsDisplayTransform* instead of just the transform matrix.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1462961 - Ensure the WebRenderLayerScrollData structure matches the APZ requirements.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details 
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
Component: General → Graphics: WebRender
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?
Blocks: 1449478, 1423370
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bugmail)
Keywords: regression
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
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 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
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 on attachment 8980082 [details]
Bug 1462961 - Dump the WebRenderScrollData as well when DUMP_LISTS is defined.

https://reviewboard.mozilla.org/r/246240/#review252352
Attachment #8980082 - Flags: review?(jmuizelaar) → review+
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
Attachment #8980083 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8980084 [details]
Bug 1462961 - Ensure the WebRenderLayerScrollData structure matches the APZ requirements.

https://reviewboard.mozilla.org/r/246244/#review252358
Attachment #8980084 - Flags: review?(jmuizelaar) → review+
Pushed by kgupta@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.