1116588 - Event regions do not get properly generated for items with opacity:0

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Build event regions for opacity:0 stacking contexts

Comment 4

[Kartikaya Gupta (email:[e-mail])]

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

Comment 5

[Timothy Nikkel (:tnikkel)]

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?

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 9

[Timothy Nikkel (:tnikkel)]

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 10

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(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!

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Build event regions for opacity:0 stacking contexts (v2)

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

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Add out-of-flow descendants

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

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Build event regions for opacity:0 stacking contexts and include out-of-flow descendants

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.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Build event regions for opacity:0 stacking contexts and include out-of-flow descendants

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.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 18

[Timothy Nikkel (:tnikkel)]

(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.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

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

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Attachment: Remove optimization that prevents building event regions for opacity:0 frames

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

Comment 21

[Timothy Nikkel (:tnikkel)]

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?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have no reason to believe it's important.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Thanks!

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

Comment 24

[Kartikaya Gupta (email:[e-mail])]

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%

Comment 25

[Kartikaya Gupta (email:[e-mail])]

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 26

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/7c4059ebfcf7

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

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+).

Comment 29

[Kartikaya Gupta (email:[e-mail])]

I ended up backing this out after all to fix bug 1126427.

https://hg.mozilla.org/integration/mozilla-inbound/rev/74f29dfebd9c

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Attachment: Remove optimization, if we need to build event regions

... 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.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 32

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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

Comment 33

[Timothy Nikkel (:tnikkel)]

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?

Comment 34

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/b2g-inbound/rev/cb09c270e0e4

Comment 36

[Kartikaya Gupta (email:[e-mail])]

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

Comment 37

[Kartikaya Gupta (email:[e-mail])]

Filed bug 1127327 for it.

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb09c270e0e4

Comment 39

[Kartikaya Gupta (email:[e-mail])]

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

Comment 40

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b871a092c90a