1241394 - "Scroll of Lorem Ipsum" demo does not scroll

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Load http://codepen.io/anon/pen/obpepj
 2. Try to scroll (mouse over the old-fashioned looking text, and use scrollwheel)

EXPECTED RESULTS:
Text should scroll.

ACTUAL RESULTS:
Text does not scroll.

Tracking down a regression range...

(Note: the original source of this demo is http://codepen.io/browles/pen/pgdMbK -- I forked it to create the "anon" version linked above in my STR, just in case browles's version changes out from under us [as I believe codepens can do])

Comment 1

[Daniel Holbert [:dholbert]]

Mozregression gives me this range:
19:56.09 LOG: MainThread main INFO Got as far as we can go bisecting nightlies...
19:56.09 LOG: MainThread Bisector INFO Last good revision: 35b211eaad1fa828064514c547057e4400e24459 (2015-12-25)
19:56.09 LOG: MainThread Bisector INFO First bad revision: 4a559a618d6798eb9a8fdc559f5a7a00085e2062 (2015-12-26)
19:56.09 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=35b211eaad1fa828064514c547057e4400e24459&tochange=4a559a618d6798eb9a8fdc559f5a7a00085e2062


...and then after switching to inbound builds:
23:17.73 LOG: MainThread Bisector INFO Narrowed inbound regression window from [35b211ea, 3c5e7567] (3 revisions) to [35b211ea, 691f2e68] (2 revisions) (~1 steps left)
23:17.73 LOG: MainThread main INFO Oh noes, no (more) inbound revisions :(
23:17.73 LOG: MainThread Bisector INFO Last good revision: 35b211eaad1fa828064514c547057e4400e24459
23:17.73 LOG: MainThread Bisector INFO First bad revision: 691f2e687e46d1b166b88b0a3f9dbd480bff9afa
23:17.73 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=35b211eaad1fa828064514c547057e4400e24459&tochange=691f2e687e46d1b166b88b0a3f9dbd480bff9afa

This is still a pretty wide range. Alice, if you can reproduce: could you see if you can get mozregression to cooperate & produce a smaller range?  (I'm suspicious of my mozregression install, given that it gave me a mozilla-central link, after supposedly bisecting mozilla-inbound builds...)

Comment 2

[Daniel Holbert [:dholbert]]

(Given that this testcase makes heavy use of 3d transforms, I wonder if bug 1226904 might be the source of the regression...)

Comment 3

[Alice0775 White]

m-i regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6c49bb716593014c07d3736ea2124f74bfe3bb06&tochange=691f2e687e46d1b166b88b0a3f9dbd480bff9afa

Via local build,
Last Good: f40c0da4e4ce (w/ patch 691f2e687e46)
First Bad: f950b7a04741 (w/ patch 691f2e687e46)

Triggered by: f950b7a04741	Thinker K.F. Li — Bug 1226904 - Fix boundary checking for leaves collecting. r=roc

Comment 4

[Alice0775 White]

Attachment: standalone.html

Standalone html works as expected....

Comment 5

[Alice0775 White]

Attachment: Iframe version

Iframe version is reproduced the problem

Comment 6

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 7

[Alice0775 White]

Aurora45. 0a2 is not affected, because the bunch of offending patch was backed out(m-a cset 64ec448f156d) now.

Comment 8

[Daniel Holbert [:dholbert]]

Thanks, Alice!

Also, FWIW -- you said "Standalone html works as expected....", but I can't scroll that attachment on my machine (w/ either scrollwheel or scrollbar).  I'm using Linux Nightly 46.0a1 (2016-01-20).  So if you can scroll that attachment w/ scrollwheel (sounds like it?), maybe this manifests slightly differently on different platforms.

Comment 9

[Daniel Holbert [:dholbert]]

ni=thinker per comment 3

Comment 10

[Matt Woodrow (:mattwoodrow)]

Do you have e10s/APZ enabled Daniel?

With it enabled I see the same as Alice: standalone.html works with the scrollwheel (but not scrollbars), iframe version doesn't scroll at all.

Without it enabled I see the same as you: Neither version scroll with the scrollwheel, can't test scrollbars as they are hidden on OSX until you scroll.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Reduced testcase

Reduced the testcase a fair bit and made it static.

It scrolls in some mouse positions, but not others...

Comment 12

[Matt Woodrow (:mattwoodrow)]

Hit test when we fail to scroll:

https://pastebin.mozilla.org/8857319

The first frame we mark as hit is line 25, the BackgroundColor for 'container'. Then after that we hit the text item (line 48).

'container' is outside of the scroll container, so that's why scrolling isn't working.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> Do you have e10s/APZ enabled Daniel?

Ah, good question. I was using a profile that had e10s disabled. The "standalone" version is WFM when I enable e10s -- so, my results match what you describe in comment 10.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: fuzzy-equal-depth

The computed depths for the content are: https://pastebin.mozilla.org/8857326

The calculation was coming up with a depth difference of 0.0000077, in favour of the non-scrolled background being in front.

We probably can't afford to be that accurate, given floating points limitations.

Using a fuzzy comparison of 0.1 fixes the reduced test case, but not the original.

We need a test for this, going to try figure out how hit test testing works.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix for reduced test case

This fixes the reduced test case, but not the original report. Test confirmed to fail without the patch, and passes with.

I may not get to that this week since it's after 5pm on Friday now.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Thinker, the code for hit testing based on depth is new isn't it?

Would it be possible to just back that out without backing the rest of the code out?

That might be a possible solution for reducing the number of regressions on Aurora if necessary (hopefully not!).

Comment 17

[Matt Woodrow (:mattwoodrow)]

Setting num=1 results in much simpler generated HTML that still reproduces a bug.

Hit testing: https://pastebin.mozilla.org/8857327

Comment 18

[Matt Woodrow (:mattwoodrow)]

This page creates two copies (top and bottom) of the scrolled content for each segment of the curled ends.

'num' controls the number of segments in each curled end.

Hit testing is hitting the copy content before it hits the normal content, so we don't scroll.

The copied content should be overflow:hidden so that you only see a small bit of it (which is how it renders), so hit testing should be working with this too.

Comment 19

[Matt Woodrow (:mattwoodrow)]

There are two nsDisplayTransforms created for the clipped copied content, lines 75 and 76.

Clipping for preserve-3d contexts must always be on the root of the context (anything that triggers clipping on intermediate/leaf elements also disables preserve-3d).

The code for doing hit testing on preserve-3d leaf elements [1] looks like it checks the clip for the leaf element (which should always be empty).

Instead I think we want to check the clip on the preserve-3d root to get correct hit testing.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1939

Comment 20

[Matt Woodrow (:mattwoodrow)]

Thinker, can you please try write a patch for this?

Comment 21

[Thinker Li [:sinker]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> There are two nsDisplayTransforms created for the clipped copied content,
> lines 75 and 76.
> 
> Clipping for preserve-3d contexts must always be on the root of the context
> (anything that triggers clipping on intermediate/leaf elements also disables
> preserve-3d).
> 
> The code for doing hit testing on preserve-3d leaf elements [1] looks like
> it checks the clip for the leaf element (which should always be empty).

The list includes not only leaves, it also include elements not participating any preserve-3d rendering context.  Checking the clip is applied to no participating ones, for leaves their |alwaysIntersect| is always true.

By the way, I will look into it.

Comment 22

[Thinker Li [:sinker]]

Attachment: Check clip for the children of the establisher

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 46, recent regression.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8711073 [details] [diff] [review]
Check clip for the children of the establisher

Review of attachment 8711073 [details] [diff] [review]:
-----------------------------------------------------------------

Adding another test would be good. Should be able to copy my test for the basic structure.

Does the test case work properly now?

Comment 26

[Thinker Li [:sinker]]

Attachment: Check clip for the children of the establisher, v2

--
Attachment #8711073 [details] [diff] - Attachment is obsolete: true

Comment 27

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #26)
> Created attachment 8711329 [details] [diff] [review]
> Check clip for the children of the establisher, v2
> 
> --
> Attachment #8711073 [details] [diff] [diff] - Attachment is obsolete: true

Add a testcase.
https://bugzilla.mozilla.org/attachment.cgi?id=8710335 work correctly.  But it is too complicated, I made a new testcase for the root cause.

Comment 28

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8711329 [details] [diff] [review]
Check clip for the children of the establisher, v2

Review of attachment 8711329 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/tests/mochitest/general/test_pointerPreserves3DClip.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

Needs the license header

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26b29cf7a373
https://hg.mozilla.org/integration/mozilla-inbound/rev/f41f716ccf4f

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0df7afe205c5

Comment 31

[Thinker Li [:sinker]]

Comment on attachment 8711329 [details] [diff] [review]
Check clip for the children of the establisher, v2

Review of attachment 8711329 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/tests/mochitest/general/test_pointerPreserves3DClip.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

I don't see any mochitest having license header.  So, do we really need it for mochitest?

Comment 32

[Matt Woodrow (:mattwoodrow)]

(In reply to Thinker Li [:sinker] from comment #31) 
> I don't see any mochitest having license header.  So, do we really need it
> for mochitest?

You're right. I landed it without the header.

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26b29cf7a373
https://hg.mozilla.org/mozilla-central/rev/f41f716ccf4f
https://hg.mozilla.org/mozilla-central/rev/0df7afe205c5

Comment 36

[Matt Woodrow (:mattwoodrow)]

This still looks quite bad with APZ, I've filed bug 1242821 for that.