Closed
Bug 1403293
Opened 7 years ago
Closed 7 years ago
Firefox 57 Beta: Scrolling inside JS expanded element not working
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
VERIFIED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | + | verified |
firefox58 | --- | verified |
People
(Reporter: lamka02sk, Assigned: bzbarsky)
References
Details
(Keywords: regression, testcase)
Attachments
(5 files)
(deleted),
video/mp4
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
emilio
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•7 years ago
|
Severity: normal → major
Iteration: --- → 57.3 - Sep 19
status-firefox57:
--- → ?
Component: Untriaged → General
Priority: -- → P5
Comment 1•7 years ago
|
||
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 → --
Reporter | ||
Comment 2•7 years ago
|
||
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)
Comment 3•7 years ago
|
||
[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.
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox58:
--- → affected
tracking-firefox57:
--- → ?
Product: Firefox → Core
Comment 4•7 years ago
|
||
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.
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 5•7 years ago
|
||
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...
Updated•7 years ago
|
Comment 6•7 years ago
|
||
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.
Reporter | ||
Comment 7•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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.
Assignee | ||
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
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.
Assignee | ||
Comment 12•7 years ago
|
||
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...
Assignee | ||
Comment 13•7 years ago
|
||
Per spec, these boxes shouldn't even exist.
MozReview-Commit-ID: 9tQdqSgXg76
Attachment #8913442 -
Flags: review?(cam)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 14•7 years ago
|
||
Assignee | ||
Comment 15•7 years ago
|
||
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 16•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Attachment #8913442 -
Flags: review?(cam)
Comment 17•7 years ago
|
||
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
Comment 18•7 years ago
|
||
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).
Assignee | ||
Comment 19•7 years ago
|
||
kats, thanks! I'll give that a shot.
Assignee | ||
Comment 20•7 years ago
|
||
I did check that this test times out without the fix for this bug
Attachment #8913547 -
Flags: review?(bugmail)
Comment 21•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 22•7 years ago
|
||
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+
Comment 23•7 years ago
|
||
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d28844bbef
followup. Add a test. r=kats
Assignee | ||
Updated•7 years ago
|
Attachment #8913547 -
Flags: approval-mozilla-beta?
Comment 24•7 years ago
|
||
bugherder |
Comment 25•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8913547 -
Flags: approval-mozilla-beta?
Comment 26•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/282e28b451d5
https://hg.mozilla.org/releases/mozilla-beta/rev/154153211b83
Flags: in-testsuite+
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Updated•7 years ago
|
Flags: qe-verify+
Comment 27•7 years ago
|
||
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).
Updated•7 years ago
|
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•