1403293 - Firefox 57 Beta: Scrolling inside JS expanded element not working

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Samuel Illo]

Attachment: Problem.mp4

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170925150345

Steps to reproduce:

I opened my website to test it in the new FF and found out that scrolling is not working. I can't move scrollbar, arrows does not work too.

Here is short video of my problem: https://youtu.be/kTsv21qsVlU


Actual results:

It's impossible to get the scrollbar and scrolling to work, when the elements' height is expanded by JS. So if you for example click on some button "Show more" and element expands, you will not be able to scroll inside this element.


Expected results:

Scrolling is supposed to work inside every element with corrent overflow.

Comment 1

[:Gijs (he/him)]

Can you provide a minimal testcase? Or at least a link to the website where you're seeing this? Just the video is not enough to know what's happening or to do any kind of debugging.

Comment 2

[Samuel Illo]

I installed my system on server, so you can try it yourself.

Visit this URL > http://firefox-test.samuelillo.com/?route=757731bc6c353376/write/article > LOGIN and PASS: firefox57 > when you login, enter the same URL again and click on "Show all settings" button (the content will expand).

Then you will see, it's not possible to scroll while in any other browser and even FF55 it's fine. Also console is clear, so my website is OK. Sometimes arrows work, but not always. Mousewheel and scrollbar are dead all the time.

Comment 3

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
webcompat regression

Trying to find a regression window, but it's clear this is a core bug, so moving, and CC'ing some folks who might know more.

Comment 4

[:Gijs (he/him)]

bz, looks like a regression from bug 1394662.

In case it's relevant, I can make the issue go away by removing:

perspective: 1000px

from the root element using the inspector.

Comment 5

[:Gijs (he/him)]

It also looks like this has something to do with the two <div class="fullscreen-wrapper">, who have styles like this:

.fullscreen-wrapper {
	display: table;
	visibility: hidden;
	width: 100%;
	height: 100%;
	position: fixed;
	top: 0;
	left: 0;
	overflow: hidden;
	z-index: 9999;
	background: transparent;
	perspective: 1000px;
	transition: .2s ease-in-out 0s;
}

given the visibility: hidden, I expect they're not supposed to interfere with scrolling, but removing them the problem goes away, so clearly they are, somehow, interfering... In any case, this helps with the question I'd been asking myself, namely how changes to table stuff had anything to do with this problem...

Comment 6

[:Gijs (he/him)]

Attachment: Minimal-ish testcase

I reduced the website testcase by basically getting rid of most of the stuff that didn't seem necessary to reproduce, and putting the relevant styles in a <style> instead of requiring more than 1 file. I forced the content of the scrollable container to have a high min-height so it's guaranteed to scroll even on large screens, and added dummy content so you can easily see if it's scrolling or not (on osx with its disappearing scrollbars).

There's probably more that could be removed to make it *really* minimal, but I'm not a domain expert so I left it like this to ensure we don't accidentally focus on the wrong thing.

Comment 7

[Samuel Illo]

I can confirm, that this bug is not related directly to servo, because when I turn off servo engine, it's still buggy.

Comment 8

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

Attachment: Some output with ENABLE_APZCTM_LOGGING and layers.dump-host-layers enabled

Note that this seems to only happen with APZ (pref layers.async-pan-zoom.enabled) enabled. I took a quick look and the hit-testing/layer tree shows a layer with a large hit region sitting on top of everything else. See attached output - the layer 0x7f7efb856c00 has a hit region of (x=0, y=0, w=1280, h=964) (i.e. everything). This results in the HitTestingTreeNode 0x7f7efe040060 having the same hit region, and that's what gets hit when you try to mousewheel over the scrollframe (per the last line of the attached log output).

The hitregion itself comes from the nsDisplayLayerEventRegions display item, which in turn builds the hit region from frames in the nsDisplayLayerEventRegions::AddFrame function. Inside that function is a visibility check [1] which I think should be getting hit and should be causing the visibility:hidden element to not contribute to the hit region. I haven't yet looked into what's not working there.

[1] http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/layout/painting/nsDisplayList.cpp#4546

Comment 9

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

It looks like the nsTableColGroupFrame frame, when passed to the nsDisplayLayerEventRegions::AddFrame function, is what causes the mHitRegion to go from empty to big. Based on the regressing patch I'm guessing this frame used to inherit visibility:hidden but now it doesn't. I'm not sure what the best way forward here so I'll wait for bz to chime in.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Right, the changes in bug 1394662 made it so the anonymous table column no longer inherits visibility style (and neither does the anonymous table column group, as of bug 1395650.

But fundamentally, from the point of view of hit-testing and whatnot, the anonymous columns/colgroups should just act like they don't exist.  We can't _quite_ just skip them altogether in display list construction, because they do need to add their background items.  However those get added using the visibility of the cell, not the column/colgroup anyway; see bug 1395312.

Given that, maybe what we should do is just style anonymous cols/colgroups with "visibility:hidden" to start with.  That way they will never interfere with hit testing.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Actually, I guess anonymous columns can't have backgrounds anyway, so it shouldn't be an issue.  So yeah, just marking them visibility:hidden seems like the best idea all around.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, so I have a patch that works on the reduced testcase and site linked in comment 2.  But I don't know how to write a test for this.  In particular, whatever hit region stuff is going on does NOT affect things like elementFromPoint or actual clicks in terms of hit-testing, as far as I can tell.  I'm not sure why it doesn't, exactly...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure that anonymous table columns and column groups never affect hit testing

Per spec, these boxes shouldn't even exist.

MozReview-Commit-ID: 9tQdqSgXg76

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8913442 [details] [diff] [review]
Make sure that anonymous table columns and column groups never affect hit testing

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1394662
[User impact if declined]: Some sites can't be scrolled with mousewheel
[Is this code covered by automated tests?]: Not yet.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Yes. See comment 2 and comment 6.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Somewhat.
[Why is the change risky/not risky?]:  I _think_ there shouldn't be any obvious fallout, but I just can't tell for sure.  :(
[String changes made/needed]: None.

The other option might be to back out the non-inheriting bits for columns/colgroups, but that leads to a noticeable memory usage regression, so I would really prefer not to do that.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8913442 [details] [diff] [review]
Make sure that anonymous table columns and column groups never affect hit testing

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

I think cam is on PTO, but this patch looks good to me, feel free to land it if you think my review is sufficient (I followed the "make them a non-inheriting anon-box" closely enough to be confident about it).

A test would be nice, but I have no idea on how to test it without getElementsFromPoint either...

::: layout/style/res/ua.css
@@ +76,5 @@
>  *|*::-moz-table-column-group {
>    display: table-column-group !important;
> +  /* Make sure anonymous colgroups don't interfere with hit testing.  Basically,
> +   * they should pretend as much as possible to not exist (since in the spec
> +   * they do not exist). */

Maybe point to this bug in case someone tries to remove it and see it's untested?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/294dfd15f0e4
Make sure that anonymous table columns and column groups never affect hit testing.  r=heycam

Comment 18

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

If you want to write a test, it should be pretty straightforward using the APZ mochitest helpers. You can use gfx/layers/apz/test/mochitest/helper_scroll_inactive_perspective.html as a starting point; clone it into a new helper file, modify as needed, and add it to the list of subtests in test_group_wheelevents.html (and the helpers list in mochitest.ini).

Comment 19

[Boris Zbarsky [:bzbarsky]]

kats, thanks!  I'll give that a shot.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: followup. Add a test

I did check that this test times out without the fix for this bug

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/294dfd15f0e4

Comment 22

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

Comment on attachment 8913547 [details] [diff] [review]
followup.  Add a test

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

LGTM, thanks!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d28844bbef
followup.  Add a test.  r=kats

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d6d28844bbef

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8913442 [details] [diff] [review]
Make sure that anonymous table columns and column groups never affect hit testing

Fix a recent regression, taking it. Should be in 57b5

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/282e28b451d5
https://hg.mozilla.org/releases/mozilla-beta/rev/154153211b83

Comment 27

[Vlad Bacia-Mociran [:VladB]]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171005100211

This issue has been verified on latest Firefox Nightly Build ID: 20171005100211 on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04. The scroll from the webpage and also from the testcase is working.

This issue has been also verified on latest Firefox Beta 57.0b5 (Build ID: 20171002181526).