Closed
Bug 1265237
Opened 8 years ago
Closed 8 years ago
Mouse events ignored in overflow:hidden regions for elements nested under multiple levels of position:fixed
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
e10s | m9+ | --- |
firefox45 | --- | unaffected |
firefox46 | --- | unaffected |
firefox47 | --- | wontfix |
firefox48 | --- | fixed |
firefox49 | --- | fixed |
People
(Reporter: till, Assigned: mattwoodrow)
References
()
Details
(Keywords: regression, reproducible, testcase)
Attachments
(3 files)
93.35 KB,
application/x-zip-compressed
|
Details | |
1.57 KB,
patch
|
mstange
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
2.36 KB,
patch
|
Details | Diff | Splinter Review |
This is kinda hard to describe, but easy to observe. STR: 1. visit http://markgrafenkarree.de/ 2. click on the hamburger menu in the top-left 3. move mouse over the entries in the now-shown menu 4. observe how they're all inactive 5. disable e10s, repeat steps 1-3 6. observe how the entries are active I tried reducing this[1], but ended up with a test case that shows different behavior from other browsers regardless of whether e10s is enabled or not. Will file another bug for that. [1] http://codepen.io/anon/pen/mPLpdP
Reporter | ||
Comment 1•8 years ago
|
||
Slight complication: the menu works in beta 46 regardless of whether e10s is enabled or not, but only works with e10s disabled in dev edition and nightly.
Version: Trunk → 47 Branch
Comment 2•8 years ago
|
||
So this is a regression, right?
Keywords: regression,
regressionwindow-wanted
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c525c5164cdde373a636d525e53c77f52d8fd693&tochange=c99e8abb5e00eaa439a3be7864df0e4135326349 Bug 1231538 - Build a ContainerLayer for position:fixed and background-attachment:fixed content. r=roc
Blocks: 1231538
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox45:
--- → unaffected
status-firefox46:
--- → unaffected
status-firefox47:
--- → affected
Updated•8 years ago
|
tracking-e10s:
--- → ?
Comment 5•8 years ago
|
||
Based on comment 3, I'm moving this to Layout. Matt, WDYT?
Component: DOM: Content Processes → Layout
Flags: needinfo?(matt.woodrow)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 6•8 years ago
|
||
This webpage has two nested position:fixed elements, so both appear as siblings in the FixedList for the viewport frame. All the sidebar content is within the inner position:fixed frame, and the overflow areas of fixed position frames doesn't contribute to that of the parent, so the overflow area for the outer fixed position frame doesn't intersect the sidebar area. When we're building a hit-test display list for a point in the sidebar, we fail the intersection between dirty rect and overflow rect in nsDisplayListBuilder::MarkOutOfFlowFrameForDisplay, so we don't store any out of flow data for the outer frame. When we come to building display lists for the outer position fixed frame, we don't have any out of flow data, so we don't override the scroll container clip. The outer nsDisplayFixedPosition ends up clipped to the scrollable area, and all hit tests fail. Tim, any idea what the best fix is here? I'm not sure if we should just be skipping this intersection (since it's not necessarily valid), or if we should instead be letting the overflow area of nested position:fixed items contribute to their parent. I also suspect we don't need to build nsDisplayFixedPosition for the nested frames, but that's probably out of the scope of this bug.
Flags: needinfo?(tnikkel)
Comment 7•8 years ago
|
||
This seems like a fundamental problem with our current approach to saving clips for out of flow frames. :( In general it is not enough to rely on the stored out of flow data, which is why we also have the NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO bit. The reason is that an out of flow frame that is not visible can contain an out of flow frame that is visible. (The situation you described is such an example.) (The nested fixed item should contribute to the overflow of it's true parent, which is the viewport frame, not the outer fixed frame. The placeholder frame for the nested fixed item contributes to the overflow of the outer fixed frame, it just contributes nothing because the placeholder frame has an empty rect.) What should happen is in the nsDisplayListBuilder::MarkOutOfFlowFrameForDisplay call for the inner fixed item, when we call MarkFrameForDisplay at the end of that function, it walks the "parent or placeholder" chain adding the NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO bit. It should also be saving out of flow data with the correct clip for the current frame if the current frame is out of flow and not out of flow data is stored for it. Unfortunately we don't have that clip at this point.
Flags: needinfo?(tnikkel)
Comment 8•8 years ago
|
||
The fact that we create display items while the incorrect clips are in place is what leads to the bug. If we didn't do that then there wouldn't be a bug. We also set the dirty rect to empty, that would affect the stored dirty rect on the created items.
Comment 9•8 years ago
|
||
Tim, this appears to be a e10s specific, is this something you feel we should block e10s rollout on? It's web facing so we're thinking it should.
Flags: needinfo?(tnikkel)
Comment 10•8 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #9) > Tim, this appears to be a e10s specific, is this something you feel we > should block e10s rollout on? It's web facing so we're thinking it should. I don't know where the bar is for e10s blockers, but it seems like we shouldn't break websites like this.
Flags: needinfo?(tnikkel)
Comment 11•8 years ago
|
||
This sounds like it will involve some work. I'm dropping this into the e10s blocking list based on comment 10 but I'd happily move it to follow up if the layout team feels we can. If not, it'll block rollout.
Flags: needinfo?(bugs)
Comment 12•8 years ago
|
||
We have other regressions from the position:fixed changes in bug 1231538 that should also go away when this one gets fixed. This one's on the front burner now.
Flags: needinfo?(bugs)
Assignee | ||
Comment 13•8 years ago
|
||
Tim suggested this on irc.
Attachment #8745191 -
Flags: review?(mstange)
Comment 14•8 years ago
|
||
Comment on attachment 8745191 [details] [diff] [review] nested-oof-frames Looks good but it absolutely needs a test.
Attachment #8745191 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 15•8 years ago
|
||
I'm having problems writing a test, it looks like we don't have a display port set when the test runs.
Flags: in-testsuite?
Comment 17•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/54e84fa84d6d for reftest failures: https://treeherder.mozilla.org/logviewer.html#?job_id=27182463&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=27182443&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=27182458&repo=mozilla-inbound
Comment 18•8 years ago
|
||
Comment on attachment 8745191 [details] [diff] [review] nested-oof-frames > if (savedOutOfFlowData) { > clipState.SetClipForContainingBlockDescendants( > &savedOutOfFlowData->mContainingBlockClip); > clipState.SetScrollClipForContainingBlockDescendants( > savedOutOfFlowData->mContainingBlockScrollClip); >+ } else if (GetStateBits() & NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO) { >+ // If we have nested out-of-flow frames and the outer one isn't visible >+ // then we won't have stored clip data for it. We can just clear the clip >+ // instead since we know we won't render anything, and the inner out-of-flow >+ // frame will setup the correct clip for itself. >+ clipState.Clear(); Probably need to make this } else if (force_descent_into AND we went into the "childType == nsGkAtoms::placeholderFrame" if above)
Comment 19•8 years ago
|
||
Matt, can you attach the non-working test you created? Sounds like we'll also need a test for comment 18.
Comment 20•8 years ago
|
||
I think/hope that comment 18 is the reason this was backed out. So if that is true then existing tests would have caught that.
Assignee | ||
Comment 21•8 years ago
|
||
Assignee | ||
Comment 22•8 years ago
|
||
Testing out Tim's suggestion: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b015af6d12e
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1ee604ae873f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 25•8 years ago
|
||
Hey matt, do you feel comfortable uplifting this to 48 and 47?
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8745191 [details] [diff] [review] nested-oof-frames Approval Request Comment [Feature/regressing bug #]: Bug 1231538 [User impact if declined]: Potentially incorrect clipping of hit tests for nested positioned content. [Describe test coverage new/current, TreeHerder]: Manually tested. [Risks and why]: Low risk. [String/UUID change made/needed]: None
Flags: needinfo?(matt.woodrow)
Attachment #8745191 -
Flags: approval-mozilla-beta?
Attachment #8745191 -
Flags: approval-mozilla-aurora?
Hello Till, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(till)
Hi Jim, from this point on (post-Beta6), I would like to minimize code churn by approving uplifts that are fixing release blocking bugs (e.g. critical recent regression, crashes, sec issues, perf, mlks). This seems to be an e10s only issue. I would like to resolve this as wontfix for Fx47 and uplift to Aurora48. Please let me know if you have any concerns. Thanks!
Flags: needinfo?(jmathies)
Comment on attachment 8745191 [details] [diff] [review] nested-oof-frames Post-Beta6 uplift bar changes to only release blocking issues, let's uplift to Aurora48 only.
Attachment #8745191 -
Flags: approval-mozilla-beta?
Attachment #8745191 -
Flags: approval-mozilla-beta-
Attachment #8745191 -
Flags: approval-mozilla-aurora?
Attachment #8745191 -
Flags: approval-mozilla-aurora+
Reporter | ||
Comment 30•8 years ago
|
||
I can confirm that the site I observed this on works now, yes.
Flags: needinfo?(till)
Comment 31•8 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #28) > Hi Jim, from this point on (post-Beta6), I would like to minimize code churn > by approving uplifts that are fixing release blocking bugs (e.g. critical > recent regression, crashes, sec issues, perf, mlks). This seems to be an > e10s only issue. I would like to resolve this as wontfix for Fx47 and uplift > to Aurora48. Please let me know if you have any concerns. Thanks! sounds good.
Flags: needinfo?(jmathies)
Comment 32•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/ad804098d30c
You need to log in
before you can comment on or make changes to this bug.
Description
•