For (some?) items with opacity:0, the code to build display items is not run as an optimization [1]. This also means that any event regions that would normally be produced by those frames don't get added. This is one of the contributing factors that makes the edge-gesture-swipe touch areas in the b2g chrome process [4] have improper hit regions, because those divs have opacity:0 [1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=b539dc005423#1949 [2] https://github.com/mozilla-b2g/gaia/blob/26d479f0fccb7174e06255121e4e938c1b280d67/apps/system/index.html#L1213-L1214

We will need this (and bug 1116586) before we can land bug 950934, otherwise it becomes possible to scroll content while in the middle of an edge swipe gesture which (while kinda neat) is a regression.

Try is green https://treeherder.mozilla.org/#/jobs?repo=try&revision=a864c2e776f5

Comment on attachment 8551869 [details] [diff] [review] Build event regions for opacity:0 stacking contexts I don't understand how this fixes the bug you described. You create a new layer event regions item, but the rects that get added to layer event regions items (this new one or existing ones) don't seem to change (I don't see how new ones get added.) Can you explain in a little more detail what this is doing and how it works?

The code passes in "this" (i.e. the opacity: 0 frame) to the nsDisplayEventRegions constructor which calls AddFrame [1] and populates the regions in the event regions item. Those then get accumulated using the normal flow in FrameLayerBuilder. Does that answer your question? [1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.h?rev=37a5836d9779#2619

Oh I guess my patch doesn't deal with all the children of the opacity:0 frame so that's probably wrong. I'll see if I can update the patch to deal with those without killing perf too much.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6) > The code passes in "this" (i.e. the opacity: 0 frame) to the > nsDisplayEventRegions constructor which calls AddFrame [1] and populates the > regions in the event regions item. Those then get accumulated using the > normal flow in FrameLayerBuilder. Does that answer your question? Ah, I missed that the constructor called AddFrame.

For this case maybe we can be smarter. The visual overflow rect of the frame should contain all in flow children and out-of-flow for which the frame is a geometric ancestor. For out of flows that the current frame is not an ancestor if they should be included in the display list they will have set the NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO bit on its parent frame chain. Even if we still need to descend we could do a quick pass just looking for out of flows and adding their rects, if we needed better performance.

Comment on attachment 8551869 [details] [diff] [review] Build event regions for opacity:0 stacking contexts Dropping flag for now as I need to write a new patch for this to consider the children of the opacity:0 item.

(In reply to Timothy Nikkel (:tn) from comment #9) > For this case maybe we can be smarter. The visual overflow rect of the frame > should contain all in flow children and out-of-flow for which the frame is a > geometric ancestor. For out of flows that the current frame is not an > ancestor if they should be included in the display list they will have set > the NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO bit on its parent frame chain. > > Even if we still need to descend we could do a quick pass just looking for > out of flows and adding their rects, if we needed better performance. I'm not sure I understand all this but I'll look at the code tomorrow and ping you if I need help with it. Thanks for the pointer!

Ok, so I think I understand what you're getting at in comment 9. But it's possible to have a regular in-flow child with a touch event listener, and unless we process that child specifically we're not going to know that the area for that child needs to be in the dispatch-to-content region. I made a test case at http://people.mozilla.org/~kgupta/bug/1116588.html that illustrates this. There's an opacity:0 div but a subsection of it has a touchstart listener and unless we process the children we won't know to split up that area. That being said we could just err on the safe side and consider the entire opacity:0 div to be in the dispatch-to-content region always, I doubt we'll encounter those very often in normal use cases.

This should just add the entire opacity:0 thing to the dispatch-to-content region, and seems to fix the test case I had.

This is my attempt at adding out-of-flow descendants but it doesn't work at all. I will look into it more later.

This is pretty far out of my comfort zone so requesting moar review. It seems to work on the basic test case that I used at http://people.mozilla.org/~kgupta/bug/1116588-2.html which has a position:absolute child of a opacity:0 div and doesn't seem to break anything too horribly.

Updated version to deal with the fact that some of those out-of-flow descendants may have pointer-events:none in which case we can omit them from the dispatch-to-content region. Without this correction there's a bug on B2G where after bringing up the rocketbar search results screen and dismissing it the main content becomes unscrollable.

How about we just abandon this optimization altogether? It's getting harder and harder to maintain, and I honestly doubt that it's worth having at this point. It's not a huge win for any content AFAIK.

(In reply to Robert O'Callahan (:roc) (offline Jan 17-22) (Mozilla Corporation) from comment #17) > How about we just abandon this optimization altogether? It's getting harder > and harder to maintain, and I honestly doubt that it's worth having at this > point. It's not a huge win for any content AFAIK. I'm fine with that. I had assumed that it was a big enough win that we wanted to try to keep it, but I didn't look into it.

Cool, I'll update the patch to just remove the optimization then. Thanks!

I had to remove a corresponding optimization in the BuildLayer function for the event regions to actually end up on a layer.

Comment on attachment 8553957 [details] [diff] [review] Remove optimization that prevents building event regions for opacity:0 frames Hmm, the check in nsDisplayOpacity::BuildLayer is relatively new, coming from bug 811831. Are we sure it's not important?

I have no reason to believe it's important.

Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4059ebfcf7

This does appear the cause a number of regressions. At least, I got a slew of emails about regressions, I haven't verified that they were the result of this patch but it's quite likely. SVG, Opacity Row Major - Ubuntu HW 12.04 - 37.2% tscroll-ASAP - Ubuntu HW 12.04 - 4.99% TP5 Scroll - Ubuntu HW 12.04 - 13.4% tscroll-ASAP - Ubuntu HW 12.04 x64 - 8.57% TP5 Scroll - Ubuntu HW 12.04 x64 - 11.6% TResize - Ubuntu HW 12.04 x64 - 5% CanvasMark - Ubuntu HW 12.04 x64 - 2.99% TResize - Ubuntu HW 12.04 - 3.68%

Oh, the CanvasMark one above was actually an improvement, not a regression. But there's more regressions: TResize - WINNT 6.2 x64 - 7.23% Tab Animation Test - WINNT 5.1 (ix) - 4.4% TResize - WINNT 6.1 (ix) - 11.2% (the ones in comment 24 and these are all from Mozilla-Inbound-Non-PGO)

Comment on attachment 8553873 [details] [diff] [review] Build event regions for opacity:0 stacking contexts and include out-of-flow descendants I think we're going to need to pursue this. It's not clear to me that it's sufficient to add just out-of-flow descendants here. We probably need to include the areas of in-flow descendants as well.

In the interest of not blocking B2G work I'd like to uplift the patch I already landed to b2g37, and work on this other approach over in bug 1126314 as a way of "fixing" the regression. If the fix is landed soon enough and is low-risk enough we can uplift it to b2g37 as well; if not we can definitely make sure no desktop release ships with the regression (which is 38+).

I ended up backing this out after all to fix bug 1126427. https://hg.mozilla.org/integration/mozilla-inbound/rev/74f29dfebd9c

... and I realized that actually there's a simple fix we can do which should take care of this bug without introducing talos regressions on desktop and without breaking bug 1126427. In almost all cases I've seen on B2G, opacity:0 is accompanied by pointer-events:none because it's Generally Not Done to have things invisible but responding to input. This version of the patch lets us keep the optimizations in that case. I did try pushes to see if the BuildContainerLayer change alone causes talos regressions; if it does then I can add a corresponding event regions check there too instead of removing the entire block.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #30) > I did try pushes to see if the BuildContainerLayer change alone causes talos > regressions; if it does then I can add a corresponding event regions check > there too instead of removing the entire block. Try push without this new patch (but with the original patch backed out): https://treeherder.mozilla.org/#/jobs?repo=try&revision=150b0418db47 Try push with the new patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01b10b739580 Talos-compare: http://perf.snarkfest.net/compare-talos/?oldRevs=150b0418db47&newRev=01b10b739580&submit=true There's only a few regressions indicated there and they all seem to be a result of high variance. I can do a few more retriggers to collect more samples but on the whole this patch looks like it should be fine perf-wise.

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/74f29dfebd9c

Comment on attachment 8555853 [details] [diff] [review] Remove optimization, if we need to build event regions Are we still planning to pursue the more complicated approach?

(In reply to Timothy Nikkel (:tn) from comment #33) > Are we still planning to pursue the more complicated approach? If we need it, yes. I realize that this patch might just increase the talos burden on enabling event-regions on desktop, so I did another try push with this patch simulating event-regions enabled to isolate the perf impact here. I'm hoping that most places that use opacity:0 will also use pointer-events:none and so the perf impact will be negligible, and we don't need to pursue the more complicated approach. If that turns out to not be the case then we can look at the complicated approach.

Re: comment 34, my new try push is at [1] and exercises the opacity:0 code as though event regions were enabled. The compare-talos data at [2] shows insigificant differences except for in tsvgr_opacity, and that too only on Linux. I looked at the tests in question [3] and they both use a bunch of opacity=".5" but no opacity="0" so I'm surprised that the code I modified is even getting hit. I'll file a follow-up to investigate what's going on here but for the moment I see no need to pursue the more complicated approach. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=71d35f2e2a35 [2] http://perf.snarkfest.net/compare-talos/?oldRevs=01b10b739580&newRev=71d35f2e2a35&submit=true [3] http://hg.mozilla.org/build/talos/file/9e02bbf80240/talos/page_load_test/svg_opacity

Filed bug 1127327 for it.

Comment on attachment 8555853 [details] [diff] [review] Remove optimization, if we need to build event regions NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 920036 User impact if declined: regions that have opacity:0 but contain items that listen for touchevents will not behave as expected. A specific example of this is bug 1123599, where it is possible to scroll apps while simultaneously performing an app-switch edge gesture. Testing completed: locally Risk to taking this patch (and alternatives if risky): low risk, this version of the patch is very limited in the impact it has. String or UUID changes made by this patch: none