Closed Bug 1137890 (containerless-scrolling) Opened 9 years ago Closed 5 years ago

Ship containerless scrolling on Android

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla67

Tracking Flags:

Tracking

Status

firefox67

---

fixed

People

(Reporter: kats, Assigned: botond)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Patch for fennec 9 years ago Kartikaya Gupta (email:kats@mozilla.staktrace.com) (deleted), patch		Details \| Diff \| Splinter Review
Patch for fennec 9 years ago Kartikaya Gupta (email:kats@mozilla.staktrace.com) (deleted), patch		Details \| Diff \| Splinter Review
Bug 1137890 - Let containerless scrolling ride the trains. r=kats 5 years ago Botond Ballo [:botond] (deleted), text/x-phabricator-request		Details

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Reporter

Description

•

9 years ago

+++ This bug was initially created as a clone of Bug #1123921 +++

Bug 1123921 turned it on for desktop, but we should turn it on everywhere and then get rid of the pref.

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Reporter

Comment 1

•

9 years ago

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

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Reporter

Comment 2

•

9 years ago

Er.. that try push was not what I meant to do. Here's a better one:

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

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Reporter

Comment 3

•

9 years ago

On Fennec if I enable the layout.scroll.root-frame-containers pref it looks like the root layer (the one corresponding to the XUL window) does still get a metrics. Furthermore whereas before the metrics had scrollId=0 and was being ignored, it now gets scrollId > 0 and so the compositor ends up putting the shadow transform on that layer rather than the actual content. I suspect the code in nsDisplayList::PaintRoot needs to be adjusted to fix this, trying that out now.

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Reporter

Comment 4

•

9 years ago

Attached patch Patch for fennec (obsolete) (deleted) — Details — Splinter Review

This fixes the obviously-wrong behaviour on my local build. Not sure if it will fix the tests but I'll push to try and see.

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Reporter

Comment 5

•

9 years ago

Attached patch Patch for fennec (obsolete) (deleted) — Details — Splinter Review

Slightly updated, so the pref takes precedence. Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a91d24ddb98d

Attachment #8575554 - Attachment is obsolete: true

Comment hidden (obsolete)

The try push above shows lots of reftest failures, which I can (suprisingly!) reproduce locally. The layer trees being generated look something like this:

03-11 10:43:53.976 I/Gecko   (30062): LayerManager (0x85806d00)
03-11 10:43:53.976 I/Gecko   (30062):   ContainerLayerComposite (0x85c37280) [shadow-transform=[ 0 0; 0 0; 0 0; ]] [shadow-visible=< (x=0, y=0, w=768, h=1134); >] [visible=< (x=0, y=0, w=768, h=1134); >] [opaqueContent]
03-11 10:43:53.976 I/Gecko   (30062):     PaintedLayerComposite (0x85c38900) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [not visible]
03-11 10:43:53.976 I/Gecko   (30062):     ColorLayerComposite (0x85c38b80) [shadow-visible=< (x=0, y=1000, w=768, h=134); >] [bounds=(x=0, y=0, w=768, h=1134)] [visible=< (x=0, y=0, w=768, h=1134); >] [opaqueContent] [color=rgba(255, 255, 255, 1)]
03-11 10:43:53.976 I/Gecko   (30062):     ContainerLayerComposite (0x866f7f00) [shadow-clip=(x=0, y=0, w=768, h=1000)] [shadow-visible=< (x=0, y=0, w=768, h=1000); >] [clip=(x=0, y=0, w=768, h=1000)] [visible=< (x=0, y=0, w=768, h=1000); >]
03-11 10:43:53.976 I/Gecko   (30062):       PaintedLayerComposite (0x866f8180) [shadow-clip=(x=0, y=0, w=800, h=1000)] [shadow-visible=< (x=0, y=0, w=768, h=1000); >] [clip=(x=0, y=0, w=800, h=1000)] [bounds=(x=0, y=0, w=800, h=1000)] [visible=< (x=0, y=0, w=768, h=1000); >] [opaqueContent] [valid=< (x=0, y=0, w=768, h=1000); >]
03-11 10:43:53.976 I/Gecko   (30062):         TiledContentHost (0x86654a00)
03-11 10:43:53.976 I/Gecko   (30062):

which is triggering the warning at [1] and presumably bailing out on the paint. On the screen I basically see garbage getting drawn (although it's the same garbage every time so maybe it's not just random garbage..)

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientTiledPaintedLayer.cpp?rev=89120ad0518f#151

Comment hidden (obsolete)

Turns out it's not tiling related, and the warning can be safely removed. Even disabling tiling entirely gives me the same results, so there's something else going on here.

Comment hidden (obsolete)

.. and it also turns out I'm not actually running reftests properly, so disregard comments 6 and 7.

Timothy Nikkel (:tnikkel)

Comment 9

•

9 years ago

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

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Reporter

Comment 10

•

9 years ago

Looks like we can turn it on for B2G right away. Maybe we should do that!

Timothy Nikkel (:tnikkel)

Comment 11

•

9 years ago

Has anyone tested it on an actual device to make sure there aren't obvious problems?

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Reporter

Updated

•

9 years ago

Depends on: 1153589

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Reporter

Comment 12

•

9 years ago

I did some testing just now and everything seemed fine. Filed bug 1153589 with a patch to enable it on B2G.

Timothy Nikkel (:tnikkel)

Comment 13

•

9 years ago

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

Timothy Nikkel (:tnikkel)

Updated

•

9 years ago

Depends on: 1155025

Timothy Nikkel (:tnikkel)

Updated

•

9 years ago

Attachment #8575560 - Attachment is obsolete: true

Timothy Nikkel (:tnikkel)

Updated

•

9 years ago

Depends on: 1157066

Timothy Nikkel (:tnikkel)

Updated

•

9 years ago

Depends on: 1158392

Botond Ballo [:botond]

Assignee

Comment 14

•

6 years ago

Commandeering this bug to let containerless scrolling on Android ride the trains.

Component: Layout → Panning and Zooming

Priority: -- → P3

Summary: Turn on containerless scrolling for root scroll frames by default → Ship containerless scrolling on Android

Botond Ballo [:botond]

Assignee

Comment 15

•

6 years ago

Moving over bugs blocked by containerless scrolling from bug 1459312.

Blocks: android-rdl, desktop-zoom-xp, 1480261, 1489653, 1479757

Botond Ballo [:botond]

Assignee

Updated

•

6 years ago

Alias: containerless-scrolling

Botond Ballo [:botond]

Assignee

Updated

•

5 years ago

Blocks: 1523405

Botond Ballo [:botond]

Assignee

Updated

•

5 years ago

Depends on: 1527511

Botond Ballo [:botond]

Assignee

Updated

•

5 years ago

Depends on: 1527675

Botond Ballo [:botond]

Assignee

Updated

•

5 years ago

Depends on: 1533607

Botond Ballo [:botond]

Assignee

Updated

•

5 years ago

Assignee: nobody → botond

Botond Ballo [:botond]

Assignee

Updated

•

5 years ago

Blocks: 1534458

Botond Ballo [:botond]

Assignee

Comment 16

•

5 years ago

I discussed this with Jessie today, and we have a good feeling about letting this ride the 67 train.

If an unexpected serious regression does come up, we can flip the pref back off.

Botond Ballo [:botond]

Assignee

Comment 17

•

5 years ago

Attached file Bug 1137890 - Let containerless scrolling ride the trains. r=kats (deleted) — Details

Botond Ballo [:botond]

Assignee

Comment 18

•

5 years ago

I'm going to go ahead and land this. The remaining open dependency, bug 1527511, is an edge case that can be uplifted post-merge.

Pulsebot

Comment 19

•

5 years ago

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/007b5a2e825a
Let containerless scrolling ride the trains. r=kats

Andrei Ciure[:aciure]

Comment 20

•

5 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/007b5a2e825a

Status: NEW → RESOLVED

Closed: 5 years ago

status-firefox67: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla67

Chris Peterson [:cpeterson]

Updated

•

5 years ago

See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1500719

You need to log in before you can comment on or make changes to this bug.