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)

47 Branch
defect
Not set
normal

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)

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
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
So this is a regression, right?
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
tracking-e10s: --- → ?
Based on comment 3, I'm moving this to Layout.

Matt, WDYT?
Component: DOM: Content Processes → Layout
Flags: needinfo?(matt.woodrow)
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
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)
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)
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.
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)
(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)
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)
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)
Tim suggested this on irc.
Attachment #8745191 - Flags: review?(mstange)
Comment on attachment 8745191 [details] [diff] [review]
nested-oof-frames

Looks good but it absolutely needs a test.
Attachment #8745191 - Flags: review?(mstange) → review+
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 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)
Matt, can you attach the non-working test you created?
Sounds like we'll also need a test for comment 18.
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.
Attached patch nested-oof-testSplinter Review
https://hg.mozilla.org/mozilla-central/rev/1ee604ae873f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Hey matt, do you feel comfortable uplifting this to 48 and 47?
Flags: needinfo?(matt.woodrow)
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?
Depends on: 1273346
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+
I can confirm that the site I observed this on works now, yes.
Flags: needinfo?(till)
(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)
Depends on: 1277123
Depends on: 1276467
Depends on: 1277251
Depends on: 1283829
No longer blocks: 1285123
No longer depends on: 1277251
You need to log in before you can comment on or make changes to this bug.