Firefox 57 Beta: Scrolling inside JS expanded element not working

VERIFIED FIXED in Firefox 57

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: lamka02sk, Assigned: bzbarsky)

Tracking

({regression, testcase})

57 Branch
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57+ verified, firefox58 verified)

Details

Attachments

(5 attachments)

Posted video 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.
Severity: normal → major
Iteration: --- → 57.3 - Sep 19
Component: Untriaged → General
Priority: -- → P5
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.
Severity: major → normal
Iteration: 57.3 - Sep 19 → ---
Component: General → Untriaged
Flags: needinfo?(lamka02sk)
Priority: P5 → --
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.
Flags: needinfo?(lamka02sk)
[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.
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.
Blocks: 1394662
Component: Untriaged → CSS Parsing and Computation
Flags: needinfo?(bzbarsky)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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...
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression, 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.
I can confirm, that this bug is not related directly to servo, because when I turn off servo engine, it's still buggy.
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
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.
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.
Flags: needinfo?(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.
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...
Per spec, these boxes shouldn't even exist.

MozReview-Commit-ID: 9tQdqSgXg76
Attachment #8913442 - Flags: review?(cam)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8913442 - Flags: approval-mozilla-beta?
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?
Attachment #8913442 - Flags: review+
Attachment #8913442 - Flags: review?(cam)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/294dfd15f0e4
Make sure that anonymous table columns and column groups never affect hit testing.  r=heycam
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).
kats, thanks!  I'll give that a shot.
I did check that this test times out without the fix for this bug
Attachment #8913547 - Flags: review?(bugmail)
https://hg.mozilla.org/mozilla-central/rev/294dfd15f0e4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8913547 [details] [diff] [review]
followup.  Add a test

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

LGTM, thanks!
Attachment #8913547 - Flags: review?(bugmail) → review+
Attachment #8913547 - Flags: approval-mozilla-beta?
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
Attachment #8913442 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8913547 - Flags: approval-mozilla-beta?
Flags: qe-verify+
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.