Closed Bug 1101627 Opened 5 years ago Closed 5 years ago

Add touch-action regions to the layer EventRegions

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: kats, Assigned: kepta, Mentored)

References

()

Details

Attachments

(2 files, 14 obsolete files)

4.47 KB, text/html
Details
20.11 KB, patch
kats
: review+
Details | Diff | Splinter Review
Part of the long-term plan in bug 928833 was to add regions to the EventRegions struct to represent different touch-action behaviours. The APZ code could then use these behaviours to support touch-action directly without having to wait for information from the main thread.

This bug tracks the work needed on the layout side to populate the new regions on the EventRegions struct.
Mentor: bugmail.mozilla
Hi kats, 
Can I work on this bug ?
Yup, definitely! I'm on vacation today and tomorrow but I can give you an idea of where to start. The specification that describes the overall goal is at http://www.w3.org/TR/pointerevents/#declaring-candidate-regions-for-default-touch-behaviors. It might also be useful for you to read the document at https://wiki.mozilla.org/Gecko:Overview (particularly the section on Layout) as it will give you a larger picture of what's going on. It doesn't matter if you don't understand all or most of it, just a general idea is fine for our purposes here.

There is already code in Gecko to parse the CSS property and store that information. It is disabled by default but you can enable it by going to about:config and setting the layout.css.touch_action.enabled property to true.

However right now that information is not propagated to the compositor, so that part of the code doesn't know which areas are covered by which touch-action property. That's what this bug is about. There are a few data structures that will need to be updated to make this work. One is the EventRegions structure at [1]. There is some documentation about this at [2].

Right now this structure has two regions, a mHitRegion and a mDispatchToContentHitRegion. We will need to add two more, a mHorizontalPanRegion and a mVerticalPanRegion. Elements that have the touch-action:pan-x property will end up in the mHorizontalPanRegion and elements that have the touch-action:pan-y property will end up in mVerticalPanRegion.

There are three other values for the touch-action property, "auto", "none", and "manipulation". In the case of "none" we will just exclude the area of the element from the mHitRegion. In the case of "auto" and "manipulation" the region will be in the mHitRegion (and possibly in the mDispatchToContentHitRegion, but that's already taken care of).

The EventRegions structure is created and populated at [3], using the regions stored on the PaintedLayerData [4]. Working backwards, these in turn are populated at [5] from the nsDisplayLayerEventRegions [6]. Working backwards once more, the nsDisplayLayerEventRegions structures are populated by calls to AddFrame [7].

This probably sounds really complicated right now but the actual code change that needs to be done is not that complicated. First you will need to add nsIntRegions to store the horizontal and vertical pan regions to the EventRegions struct, the PaintedLayerData struct, and the nsDisplayLayerEventRegions struct. Then you will need to modify nsDisplayLayerEventRegions::AddFrame to populate the regions from the frame. Finally you will need to update [5] and [3] to make sure these regions are propagated forward into the compositor. The second bit (modifying AddFrame) is the most complex bit so I would suggest doing the other two bits first, and then we can discuss the AddFrame changes in more detail.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayersTypes.h?rev=99a62dc7395a#161
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h?rev=99a62dc7395a#895
[3] http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=99a62dc7395a#2296
[4] http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=99a62dc7395a#409
[5] http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=99a62dc7395a#3056
[6] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.h?rev=99a62dc7395a#2586
[7] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=99a62dc7395a#2840
Assignee: nobody → 0o3ko0
Hi Kushan, how's it going on this bug? Just checking in to see if you're stuck or need any help.
Hi kats,
I am so sorry, I was traveling back to college. Give me 2 days to get back to you, if possible?
I have started working on this bug, I am really sorry for the long delay. 
Can you please help me with mHitregion and mDispatchToContentHitRegion , basically what they are and their properties ?
(In reply to 0o3ko0 from comment #6)
> I have started working on this bug, I am really sorry for the long delay.

No worries!

> Can you please help me with mHitregion and mDispatchToContentHitRegion ,
> basically what they are and their properties ?

These two fields represent areas of the page that are used in the hit-testing code for the async panning and zooming framework. It's a little hard to explain without describing how the async panning works at a high level, so it would help to read this document: https://github.com/mozilla/gecko-dev/blob/master/gfx/doc/AsyncPanZoom.md - in there, the stuff referred to as the "dispatch-to-content region" is stored in mDispatchToContentHitRegion, and the "hit area" or "hit region" is stored in mHitRegion.

I realize having to read all this documentation to understand what's going on might be somewhat intimidating, but if you're interested in learning about Gecko internals and how the different parts of the rendering engine work then it's quite helpful. Let me know if this answers your questions!
Yes it is tough to understand what is going on, but I am liking the experience. I understand that I have to add nsIntRegions to store the horizontal and vertical pan regions inthe EventRegions struct , what else 
is required in this struct?
Thanks for your quick and comprehensive replies.
(In reply to 0o3ko0 from comment #8)
> Yes it is tough to understand what is going on, but I am liking the
> experience.

Cool, glad to hear it :)

> I understand that I have to add nsIntRegions to store the
> horizontal and vertical pan regions inthe EventRegions struct , what else 
> is required in this struct?

That should be it for this bug. Note that you will have to add these regions to a few different structs as I outlined in comment 2.
Attached patch addtouch.patch (obsolete) — Splinter Review
Hi, I am stuck with the nsDisplayLayerEventRegions. I don't understand how to modify this sturcture. Can you please help me ?
Hey, the patch you have so far looks good. nsDisplayLayerEventRegions is defined at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.h?rev=650de89062f6#2613 and it also has nsIntRegion fields - you just need to add the two nsIntRegions there the same way you did for the other two structures.
Kats,I had a few doubts:-
1) Whats the difference between nsIntRegion and nsRegion?
2) What is this convention of starting variables with a or m ?
(In reply to 0o3ko0 from comment #12)
> Kats,I had a few doubts:-
> 1) Whats the difference between nsIntRegion and nsRegion?

Ah, this one is subtle. Most of layout is done in "app units" instead of pixels. Each app unit is 1/60 of a CSS pixel, so that we can do fractional layout using integers easily. nsRegion a region implementation that stores app units. nsIntRegion, on the other hand, contains pixel values. There's actually a lot of coordinate spaces involved but for the purposes of this bug you should probably not worry about them. The only conversion you need to know is that when converting from nsRegion to nsIntRegion you'll need to use the ScaleRegionToOutsidePixels as at [1]. If you *really* want to learn about some of the coordinate systems [2] is a good place to start.

> 2) What is this convention of starting variables with a or m ?

The "m" prefix is used on member variables of a class whereas the "a" prefix is used on arguments to a function. It just makes it a little easier when reading code to know where the variables are coming from.

Good questions, and please let me know if you have more! :)

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=650de89062f6#2324
[2] https://staktrace.com/spout/entry.php?id=800
Attached patch addtouch.patch (obsolete) — Splinter Review
Hi,
I have added the pan regions in  nsDisplayLayerEventRegions. I have used the nsRegion struct, I hope its the correct way.
After this, as proposed in the comment, we have to modify nsDisplayLayerEventRegions::AddFrame. Do I need to add in
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=99a62dc7395a#2868
Something like
mHorizontalPanRegion.or(mHorizontalPanRegion,borderBox) .

-Thanks for your help. :)
Yup this is looking good. Next I guess we can look at modifying the nsDisplayLayerEventRegions::AddFrame code to populate the regions. You have the right idea in comment 14 but it's a little bit more complicated than that. What you need do there is make a copy of the code at [1] and use that to decide whether a particular frame goes into the mHorizontalPanRegion or the mVerticalPanRegion, or neither. That code takes as input a nsIFrame* and produces one of the NS_STYLE_TOUCH_ACTION* properties defined at [2]. If the value produced is _PAN_X then you want to do mHorizontalPanRegion.OrWith(borderBox) and if it's _PAN_Y then you want use mVerticalPanRegion instead.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/ContentHelper.cpp#26
[2] http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleConsts.h?rev=5f96dee075de#786
Attached patch addtouch.patch , draft (obsolete) — Splinter Review
Hi kats,
In this patch I did a bit of guess work. I didn't understand what to do with touch  type auto and manipulation doesn't seem to be mentioned anywhere.
Please review my code. :)
-Thanks
For auto and manipulation you don't need to do anything; that is the default behaviour without any changes. However there is a 'none' case as well, where we are not supposed to allow any panning or zooming. Back in comment 2 I said that for the "none" case we would just exclude the region from the mHitRegion but now I realize that's wrong, and that we actually need a separate region to store that. So please add a third mNoActionRegion alongside the horizontal and vertical pan regions, and populate that if the touch action value is NS_STYLE_TOUCH_ACTION_NONE.

Finally, looking at the patch I realized it probably makes more sense to move some of that code into a helper method in nsLayoutUtils. So please move ContentHelper::GetTouchActionFromFrame into layout/base/nsLayoutUtils.cpp and then updated both ContentHelper and your new code to use that instead, it will be a little cleaner that way. Sorry for not thinking of this earlier.

We'll also need to make sure to propagate the regions along from nsDisplayLayerEventRegions struct into the PaintedLayerData and then into the layer's EventRegions struct, but we can look at that in the next iteration.

Thanks again for working on this!
I have a doubt.
Do I have to use code at
http://mxr.mozilla.org/mozilla-central/source/widget/ContentHelper.cpp#64
To determine if its a vertical/horizontal pan, or something else?
(In reply to 0o3ko0 from comment #18)
> I have a doubt.
> Do I have to use code at
> http://mxr.mozilla.org/mozilla-central/source/widget/ContentHelper.cpp#64
> To determine if its a vertical/horizontal pan, or something else?

Yes, you're right. I see that you used that code in your patch but it makes it a little more annoying to refactor the code into a helper method. Still I think putting the GetTouchActionFromFrame code into nsLayoutUtils is still useful, and then the code at http://mxr.mozilla.org/mozilla-central/source/widget/ContentHelper.cpp#64 can stay as-is, and you can also copy that bit into nsDisplayEventRegions::AddFrame like you did in your patch. Does that make sense?
(Also fyi when you upload a new version of a patch, you can mark the old attachments as 'obsolete' on the attachment upload page. It will help keep the bug less confusing)
Kats, When I Or the region, the compositor does the math and displays the frame.

But in the case of touch action none, I have added an if statement to detect none action but what am I supposed to do in this condition. I can't Or it with borderbox right?
If I'm understanding your question correctly then yeah you need to Or it with the borderbox. i.e. mNoActionRegion.OrWith(borderbox) in the case where the touch action value is NS_STYLE_TOUCH_ACTION_NONE.
Attached patch addtouch.patch (obsolete) — Splinter Review
Hi,
I am sorry I didn't know about the obsolete feature.Please review the patch, and I was wondering how does the Or function works when panning is disabled?
Attachment #8549090 - Attachment is obsolete: true
Attachment #8549690 - Attachment is obsolete: true
Attachment #8550444 - Attachment is obsolete: true
I'm afraid I don't really understand your question. The Or function is just a function that combines two regions; it will always work. What we are trying to do in this bug is just making sure that the correct regions are sent over to the compositor so that it knows where to allow panning and where not to.
Ok, I just wanted to know how the Or function works. I guess it is clear now. Can you please help me on how to proceed with the bug. 
Thanks :)
So the next step is this:

> We'll also need to make sure to propagate the regions along from
> nsDisplayLayerEventRegions struct into the PaintedLayerData and then into
> the layer's EventRegions struct

which you will need to do like so:

> Finally you will need to update [5] and [3] to make sure these regions are
> propagated forward into the compositor.
> 
> [3]
> http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=99a62dc7395a#2296
> [5]
> http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=99a62dc7395a#3056

You can see how the existing regions are propagated and more or less copy that behaviour for the new regions you are adding.
Hi, I had a doubt.
In EventRegions struct, how do I propagate the touch action regions?
Should I treat them similar to mHitRegion and do as it is done for mHitRegion or something else?

-Thanks
For the new regions you're adding you should just Or them into the structure you're propagating them into. Pretty similar to mHitRegion, yes, ignoring the stuff about the maybeHitRegion.
Attached patch addtouch.patch (obsolete) — Splinter Review
Hi kats,
I have added the code for propagation of the touch regions.
Please review the code and tell me what else is left?
-Thanks a lot.
Attachment #8552953 - Attachment is obsolete: true
Looks good so far. I can think of two other pieces that are missing. One is the GfxMessageUtils code at http://mxr.mozilla.org/mozilla-central/source/gfx/ipc/GfxMessageUtils.h?rev=7f7f003696ad#908 needs to also read and write the new regions. The other is the dumping code at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayersLogging.cpp?rev=f3df1027b8b2#145 should also dump the new regions you added.

Once you make those changes you can verify that your patch is working as intended by going to about:config and turning on the layout.css.touch_action.enabled, layout.event-regions.enabled, and layers.dump prefs, and then inspecting the console output from the browser. You should layer tree dumps being printed to the console which will include things like this:

PaintedLayerComposite (0xaa34bc00) [not visible] { hitregion=< (x=0, y=0, w=480, h=749); >} [opaqueContent]

If you create a test page which uses the touch-action properties, for example:

<html><body style="touch-action: pan-x"></body></html>

you should see the layer dump contain some output for the horizontal pan region like my example above has the hitregion.
Kats,
I was wondering which part of the code reads the property "pan-x", and decides it is horizontal panning.
That happens in the CSS parser. The table at http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProps.cpp?rev=74d5b7a1a448#1730 maps "pan-x" to the value that you're reading in nsLayoutUtils.
Attached patch addtouch.patch (obsolete) — Splinter Review
Hi,
I have added the code necessary for dumping the information in console. I also tried to run this build and it successfully shows dumps for touch action regions.
Please review the code.

Thanks a lot
Attachment #8554570 - Attachment is obsolete: true
Attachment #8555436 - Flags: review?(bugmail.mozilla)
Comment on attachment 8555436 [details] [diff] [review]
addtouch.patch

Review of attachment 8555436 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This looks pretty good. I'll also apply this patch locally and verify that it's doing what we expect. In the meantime there's a bunch of mostly style comments I had, see below. It might also be a good idea to rebase the patch on top of the latest mozilla-central code since there's a few things that have changed since then and this patch doesn't apply cleanly any more. No functional changes needed though. I'd like to see a new version with the style comments addressed.

::: layout/base/FrameLayerBuilder.cpp
@@ +319,5 @@
>      mMaybeHitRegion.Or(mMaybeHitRegion, aEventRegions->MaybeHitRegion());
>      mDispatchToContentHitRegion.Or(mDispatchToContentHitRegion, aEventRegions->DispatchToContentHitRegion());
> +    mNoActionRegion.Or(mNoActionRegion, aEventRegions->NoActionRegion());
> +    mHorizontalPanRegion.Or(mHorizontalPanRegion,aEventRegions->HorizontalPanRegion());
> +    mVerticalPanRegion.Or(mVerticalPanRegion,aEventRegions->VerticalPanRegion());

nit: add a space after the comma on these two lines

@@ +2347,5 @@
>      regions.mDispatchToContentHitRegion.Sub(maybeHitRegion, regions.mHitRegion);
>      regions.mDispatchToContentHitRegion.Or(regions.mDispatchToContentHitRegion,
>                                             ScaleRegionToOutsidePixels(data->mDispatchToContentHitRegion));
>  
> +    //Translation of event regions.

Remove this comment, since it doesn't really add anything that's not obvious from the variable names in the code.

::: layout/base/nsDisplayList.cpp
@@ +3087,5 @@
>    {
>      mDispatchToContentHitRegion.Or(mDispatchToContentHitRegion, borderBox);
>    }
> +
> +  //Touch action region

nit: add a space after //

@@ +3089,5 @@
>    }
> +
> +  //Touch action region
> +
> +  uint32_t aTouchActionValue = nsLayoutUtils::GetTouchActionFromFrame(aFrame);

rename this to "touchActionValue". The "a" prefix is only for arguments. Or even better, just rename it to "touchAction" since it's a bit shorter.

@@ +3091,5 @@
> +  //Touch action region
> +
> +  uint32_t aTouchActionValue = nsLayoutUtils::GetTouchActionFromFrame(aFrame);
> +  if (aTouchActionValue & NS_STYLE_TOUCH_ACTION_NONE) {
> +    mNoActionRegion.Or(mNoActionRegion,borderBox);

space after comma. same for the other two similar lines below.

@@ +3093,5 @@
> +  uint32_t aTouchActionValue = nsLayoutUtils::GetTouchActionFromFrame(aFrame);
> +  if (aTouchActionValue & NS_STYLE_TOUCH_ACTION_NONE) {
> +    mNoActionRegion.Or(mNoActionRegion,borderBox);
> +  }
> +  else if ((aTouchActionValue & NS_STYLE_TOUCH_ACTION_PAN_X) && !(aTouchActionValue & NS_STYLE_TOUCH_ACTION_PAN_Y)) {

move the else up a line so that it's on the same line as the close-brace. same for the other one below.

@@ +3097,5 @@
> +  else if ((aTouchActionValue & NS_STYLE_TOUCH_ACTION_PAN_X) && !(aTouchActionValue & NS_STYLE_TOUCH_ACTION_PAN_Y)) {
> +    mHorizontalPanRegion.Or(mHorizontalPanRegion,borderBox);
> +  }
> +  else if ((aTouchActionValue & NS_STYLE_TOUCH_ACTION_PAN_Y) && !(aTouchActionValue & NS_STYLE_TOUCH_ACTION_PAN_X)) {
> +	  mVerticalPanRegion.Or(mVerticalPanRegion,borderBox);

fix indentation

::: layout/base/nsLayoutUtils.cpp
@@ +7832,5 @@
> +/* static */ uint32_t
> +nsLayoutUtils::GetTouchActionFromFrame(nsIFrame* aFrame)
> +{
> +  // If aFrame is null then return default value
> +  if (!aFrame) {

Now that you have this function here, you can remove ContentHelper::GetTouchActionForFrame, and update the one callsite of that function to use this nsLayoutUtils version instead.

::: layout/base/nsLayoutUtils.h
@@ +2399,5 @@
>    AssertTreeOnlyEmptyNextInFlows(nsIFrame *aSubtreeRoot);
>  #endif
> +  /**
> +   * Helper method to get touch action from the frame
> +   */

nit: Add a blank line above this comment, and insert the word "behavior" after "action".
Attachment #8555436 - Flags: review?(bugmail.mozilla) → feedback+
Ah, while testing locally I found another place that needs updating. In FrameLayerBuilder inside the block at [1], we need to transform the vertical/horizonta/none pan regions similar to how the hit region is done. And then, if the transform is not "precise" then we need to add those areas to the dispatch-to-content region rather than the containingPaintedLayerData's vertical/horizontal/none regions.

In a nutshell what this code is doing is dealing with things like CSS transforms, where an area can be rotated by arbitrary angles. Since a region only stores x/y-aligned rectangles, once a region is rotated it could expand to the bounding box of the rotated shape. The bounding box now contains areas both inside the rotated shape and outside the rotated shape, and so the region becomes ambiguous. We cannot deal with such a shape in the compositor, which is why we need to put those areas into the dispatch-to-content region and then allow the main thread to determine what to do there.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=49b107ba5bee#2310
Attached patch addtouch.patch (obsolete) — Splinter Review
Hi,
I have updated the patch with a new fresh code.
Please review the code. 
Thanks :D
Attachment #8555436 - Attachment is obsolete: true
It looks like you addressed all the comments I had in comment 34, thanks! However it doesn't look like you addressed the things from comment 35, but instead added some extra region subtraction stuff like "regions.mDispatchToContentHitRegion.Sub(maybeHitRegion, regions.mHorizontalPanRegion);" which is incorrect. Note that this line is the equivalent of mDispatchToContentHitRegion = maybeHitRegion - mHorizontalPanRegion, and so it will completely rewrite what mDispatchToContentHitRegion used to be. So your code will actually overwrite mDispatchToContentHitRegion a few times. But regardless, we don't want to subtract the horizontal/vertical regions from the dispatch-to-content region. Please remove those lines and take a look a the code I linked to in comment 35 (lines 2310-2341 specifically) and see if you understand what needs to be changed there.
Attached patch addtouch.patch (obsolete) — Splinter Review
I am so sorry, I understood it wrong. 
So, I have to calculate if its precise and do similarly as its already done for mHitRegion, but if isPrecise returns false then add to dispatch to content.

Please help me,
Thanks a lot.
Attachment #8558077 - Flags: review+
Comment on attachment 8558077 [details] [diff] [review]
addtouch.patch

Review of attachment 8558077 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks! This is looking good to me. I'd like roc to do the final review on this patch though.

::: layout/base/nsDisplayList.cpp
@@ +3137,5 @@
> +    mNoActionRegion.Or(mNoActionRegion, borderBox);
> +  } else if ((touchAction & NS_STYLE_TOUCH_ACTION_PAN_X) && !(touchAction & NS_STYLE_TOUCH_ACTION_PAN_Y)) {
> +    mHorizontalPanRegion.Or(mHorizontalPanRegion, borderBox);
> +  } else if ((touchAction & NS_STYLE_TOUCH_ACTION_PAN_Y) && !(touchAction & NS_STYLE_TOUCH_ACTION_PAN_X)) {
> +	  mVerticalPanRegion.Or(mVerticalPanRegion, borderBox);

nit: please fix indentation here

::: layout/base/nsDisplayList.h
@@ +2644,5 @@
>    const nsRegion& MaybeHitRegion() { return mMaybeHitRegion; }
>    const nsRegion& DispatchToContentHitRegion() { return mDispatchToContentHitRegion; }
> +  const nsRegion& NoActionRegion() { return mNoActionRegion; }
> +  const nsRegion& HorizontalPanRegion() { return mHorizontalPanRegion;}
> +  const nsRegion& VerticalPanRegion() { return mVerticalPanRegion;}

nit: please add a space before } for the two lines above
Attachment #8558077 - Flags: review+ → review?(roc)
Attached patch addtouch.patch (obsolete) — Splinter Review
Hi,
I have fixed the indentation. Please review the patch
-Thanks
Attachment #8558077 - Attachment is obsolete: true
Attachment #8558077 - Flags: review?(roc)
Attachment #8558612 - Flags: review+
Attached patch addtouch.patch (obsolete) — Splinter Review
I by mistake, removed roc@ocallahan.org for reviewing. Sorry for that.
Attachment #8558612 - Attachment is obsolete: true
Attachment #8558613 - Flags: review?(roc)
Comment on attachment 8558613 [details] [diff] [review]
addtouch.patch

Review of attachment 8558613 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/LayersTypes.h
@@ +161,5 @@
>    nsIntRegion mHitRegion;
>    nsIntRegion mDispatchToContentHitRegion;
> +  nsIntRegion mNoActionRegion;
> +  nsIntRegion mHorizontalPanRegion;
> +  nsIntRegion mVerticalPanRegion;

Please add some documentation here. There should have been some already for mHitRegion and mDispatchToContentHitRegion --- my bad --- but please fix that now.

In particular, please document the required relationships between these regions, i.e. which regions must contain (or fully exclude) which other regions.

Also document whether these regions are "maybe-hit-regions" or not. I.e., does this contain the true "no-action region", or is this exactly the no-action region?

::: layout/base/FrameLayerBuilder.cpp
@@ +435,5 @@
>     * The dispatch-to-content hit region for this PaintedLayer.
>     */
>    nsRegion  mDispatchToContentHitRegion;
>    /**
> +   * The touch-action region when panning is disabled.

Please document whether these regions are "maybe-hit-regions" or not. I.e., does this contain the true "no-action region", or is this exactly the no-action region?

It's really important we track this accurately. For example, if the no-action region is approximated conservatively at any point, we might start ignoring events that we shouldn't. Basically, any points for which we're not sure which region they're in, should be added to the mDispatchToContentHitRegion.

@@ +2364,5 @@
> +        data->mNoActionRegion.GetBounds(),
> +        containingPaintedLayerData->mReferenceFrame);
> +      nsRegion* dest = isPrecise ? &containingPaintedLayerData->mNoActionRegion
> +                                 : &containingPaintedLayerData->mDispatchToContentHitRegion;
> +      dest->Or(*dest, rect);

Please factor out this code into a helper function so it doesn't get duplicated 3 times.

::: layout/base/nsDisplayList.cpp
@@ +3137,5 @@
> +    mNoActionRegion.Or(mNoActionRegion, borderBox);
> +  } else if ((touchAction & NS_STYLE_TOUCH_ACTION_PAN_X) && !(touchAction & NS_STYLE_TOUCH_ACTION_PAN_Y)) {
> +    mHorizontalPanRegion.Or(mHorizontalPanRegion, borderBox);
> +  } else if ((touchAction & NS_STYLE_TOUCH_ACTION_PAN_Y) && !(touchAction & NS_STYLE_TOUCH_ACTION_PAN_X)) {
> +    mVerticalPanRegion.Or(mVerticalPanRegion, borderBox);

What if the style has both PAN_X and PAN_Y? Is that impossible?
Attachment #8558613 - Flags: review?(roc)
Thanks roc for the review.
Kats I would need a bit of help in the documentation part. 
- I dont understand the difference between true and the actual region.
- If it is both pan_x and pan_y, wouldn't it be the case of auto and the hit region will take care of it?
Attached file Illustrations
Sorry for the late response; I was thinking about what roc said and I had to spend some time convincing myself that the code is ok.

(In reply to 0o3ko0 from comment #43)
> - I dont understand the difference between true and the actual region.

See the illustration and explanations I attached, hopefully it will clarify some things. The green rect is the "true" region that we care about but because that cannot represented in a nsRegion/nsIntRegion data structure (which only stores a set of axis-aligned rectangles) we need to store other regions that allows us to approximate as closely as possible. Let me know if the explanations there make sense. If it does, then it should make sense that all of the new regions you added will contain the true region, but may also include areas outside of the true region. However, any areas that get included outside of the true region will also be included in the dispatch-to-content region, and as long as give that priority everything should work.

In terms of documentation - in addition to what roc said to document, you can also update the comment at [1] to clarify the above, so that it's more explicit that the dispatch-to-content region takes priority over the touch-action regions.

> - If it is both pan_x and pan_y, wouldn't it be the case of auto and the hit
> region will take care of it?

Not quite, actually. If it is both pan_x and pan_y then both horizontal and vertical scrolling are allowed, but zooming is not. auto allows scrolling and zooming, so they are slightly different. I think what we want to do is like this:

if (touchAction & ACTION_NONE) {
  // add to mNoActionRegion
} else {
  if (touchAction & ACTION_PAN_X) {
    // add to mHorizontalPanRegion
  }
  if (touchAction & ACTION_PAN_Y) {
    // add to mVerticalPanRegion
  }
}

That way, if both pan_x and pan_y are set, it will get added to both mHorizontal and mVertical pan regions, and then in the compositor where we use this information we will know to allow both horizontal and vertical panning but not zooming. If you like you can also make a note of this in the comment at [1] as it will be part of the API contract between the layout code and the compositor code.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h?rev=674f5f49d0d8#903
Attached patch addtouch.patch (obsolete) — Splinter Review
I am sorry for a late reply.
I have implemented the changes and am a bit confused with the documentation part.

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.h#2664

Here it says dispatchtoContent is always a union go mhitRegion and mMayBeHitRegion, but in the illustration you mentioned it is dispatchToContentRegion = maybeHitRegion - hitRegion (has a hole).

I am a bit confused and please check the patch, I hope I put it correctly.
Attachment #8558613 - Attachment is obsolete: true
(In reply to 0o3ko0 from comment #45)
> I have implemented the changes and am a bit confused with the documentation
> part.
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> h#2664
> 
> Here it says dispatchtoContent is always a union go mhitRegion and
> mMayBeHitRegion, but in the illustration you mentioned it is
> dispatchToContentRegion = maybeHitRegion - hitRegion (has a hole).

The comment says mDispatchToContentRegion is *contained in* the union of mHitRegion and mMaybeHitRegion; that is, the union of mHitRegion and mMaybeHitRegion contains mDispatchToContentRegion.
Thanks botond for pointing that out, I didn't pay attention to that part.

Kate can you please review the patch.
Thanks :)
Comment on attachment 8564331 [details] [diff] [review]
addtouch.patch

Review of attachment 8564331 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/LayersTypes.h
@@ +161,5 @@
> +
> +  // The event region can be considered to be comprised of an abstract shape
> +  // the biggest rectangle completely inside this shape will be stored inside
> +  // the HitRegion and the smallest rectangle containing this shape will the
> +  // maybeBeHitRegion.

In this structure there is no maybeBeHitRegion so this comment doesn't make much sense. I would suggest replacing this comment with:

"The hit region for a layer contains all areas on the layer that are sensitive to events. This region is an over-approximation and may contain regions that are not actually sensitive, but any such regions will be included in the mDispatchToContentHitRegion."

@@ +168,1 @@
>    nsIntRegion mDispatchToContentHitRegion;

Replace this comment with:

"The mDispatchToContentHitRegion for a layer contains all areas for which the main-thread must be consulted before responding to events. This region will be a subregion of mHitRegion."

@@ +168,5 @@
>    nsIntRegion mDispatchToContentHitRegion;
>  
> +  // The touchAction regions will always include the true region of concern and
> +  // also this region may or may not contain regions outside true region,
> +  // these regions will be taken care by the DispatchToContentRegion.

"The following regions represent the touch-action areas of this layer. All of these regions are approximations to the true region, but any variance between the approximation and the true region is guaranteed to be included in the mDispatchToContentHitRegion."

::: layout/base/FrameLayerBuilder.cpp
@@ +439,5 @@
> +   * The touch-action region when panning is disabled. The touch-action
> +   * regions may have maybeHitRegion. If dispatchToContentHit region  takes
> +   * priority the maybeHitRegion would be taken care of during hit-testing.
> +   */
> +  nsRegion mNoActionRegion;

"The region for this PaintedLayer that is sensitive to events but disallows panning and zooming. This is an approximation and any deviation from the true region will be part of the mDispatchToContentHitRegion."

@@ +443,5 @@
> +  nsRegion mNoActionRegion;
> +  /**
> +   * The touch-action horizontal pan region.
> +   */
> +  nsRegion mHorizontalPanRegion;

"The region for this PaintedLayer that is sensitive to events and allows horizontal panning but not zooming. This is an approximation and any deviation from the true region will be part of the mDispatchToContentHitRegion."

@@ +447,5 @@
> +  nsRegion mHorizontalPanRegion;
> +  /**
> +   * The touch-action vertical pan region.
> +   */
> +  nsRegion mVerticalPanRegion;

"The region for this PaintedLayer that is sensitive to events and allows vertical panning but not zooming. This is an approximation and any deviation from the true region will be part of the mDispatchToContentHitRegion."

@@ +2364,5 @@
> +        data->mNoActionRegion.GetBounds(),
> +        containingPaintedLayerData->mReferenceFrame);
> +      nsRegion* dest = isPrecise ? &containingPaintedLayerData->mNoActionRegion
> +                                 : &containingPaintedLayerData->mDispatchToContentHitRegion;
> +      dest->Or(*dest, rect);

There's still a bunch of duplicated code here that can be factored out.

::: layout/base/nsDisplayList.cpp
@@ +3136,5 @@
> +  if (touchAction & NS_STYLE_TOUCH_ACTION_NONE) {
> +    mNoActionRegion.Or(mNoActionRegion, borderBox);
> +  } else {
> +    if ((touchAction & NS_STYLE_TOUCH_ACTION_PAN_X) && !(touchAction & NS_STYLE_TOUCH_ACTION_PAN_Y)) {
> +		  mHorizontalPanRegion.Or(mHorizontalPanRegion, borderBox);

This isn't quite right. See comment 44, you want the conditions to check for pan_x and pan_y independently. Also please fix the indentation here.
Attached patch addtouch.patch (obsolete) — Splinter Review
Hi, sorry for a late reply.
I hope everything is as expected now, please review the code.
Thanks :)
Attachment #8564331 - Attachment is obsolete: true
Attachment #8572584 - Flags: review?(bugmail.mozilla)
Attachment #8572584 - Flags: feedback?
Comment on attachment 8572584 [details] [diff] [review]
addtouch.patch

Review of attachment 8572584 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/stlport/stlport/signal.h
@@ +1,1 @@
> + /*

Seems like a spurious space character got added to this file. Please remove.

::: gfx/layers/LayersTypes.h
@@ +159,5 @@
>  
>  struct EventRegions {
> +
> +  // The hit region for a layer contains all areas on the layer that are
> +  // sensitive to events. This region is an over-approximation and may 

remove trailing whitespace

@@ +171,5 @@
>  
> +  // The following regions represent the touch-action areas of this layer.
> +  // All of these regions are approximations to the true region, but any
> +  // variance between the approximation and the true region is guaranteed
> +  // to be included in the mDispatchToContentHitRegion."

Please remove the quote mark at the end of this line

@@ +175,5 @@
> +  // to be included in the mDispatchToContentHitRegion."
> +  nsIntRegion mNoActionRegion;
> +  nsIntRegion mHorizontalPanRegion;
> +  nsIntRegion mVerticalPanRegion;
> +  

remove trailing whitespace

::: layout/base/FrameLayerBuilder.cpp
@@ +415,5 @@
>    /**
> +   * The region for this PaintedLayer that is sensitive to events
> +   * but disallows panning and zooming. This is an approximation
> +   * and any deviation from the true region will be part of the 
> +   * mDispatchToContentHitRegion.   

Remove trailing whitespace on these two lines

@@ +2303,5 @@
> +        mContainerReferenceFrame, 
> +        containingPaintedLayerData->mReferenceFrame,
> +        &containingPaintedLayerData->mHitRegion,
> +        &containingPaintedLayerData->mDispatchToContentHitRegion,
> +        data->mNoActionRegion.GetBounds(),

This change is incorrect.. it seems to deal with mNoActionRegion which is clearly wrong. What I would suggest is to further refactor this code so that it looks like this:

TransformToAncestorAndCombineRegions(
  data->mHitRegion.GetBounds(),
  mContainerReferenceFrame,
  containingPaintedLayerData->mReferenceFrame,
  &containingPaintedLayerData->mHitRegion,
  &containingPaintedLayerData->mMaybeHitRegion);
TransformToAncestorAndCombineRegions(
  data->mNoActionRegion.GetBounds(),
  mContainerReferenceFrame,
  containingPaintedLayerData->mReferenceFrame,
  &containingPaintedLayerData->mNoActionRegion,
  &containingPaintedLayerData->mDispatchToContentRegion);
TransformToAncestorAndCombineRegions(
  data->mHorizontalPanRegion.GetBounds(),
  mContainerReferenceFrame,
  containingPaintedLayerData->mReferenceFrame,
  &containingPaintedLayerData->mHorizontalPanRegion,
  &containingPaintedLayerData->mDispatchToContentRegion);
TransformToAncestorAndCombineRegions(
  data->mVerticalPanRegion.GetBounds(),
  mContainerReferenceFrame,
  containingPaintedLayerData->mReferenceFrame,
  &containingPaintedLayerData->mVerticalPanRegion,
  &containingPaintedLayerData->mDispatchToContentRegion);

and then implement the function so that it does both the IsEmpty check, followed by computing the Matrix4x4 and checking isPrecise, and all that other stuff. Does that make sense?

::: layout/base/nsDisplayList.cpp
@@ +3220,5 @@
> +
> +  // Touch action region
> +	
> +	uint32_t touchAction = nsLayoutUtils::GetTouchActionFromFrame(aFrame);
> +	if (touchAction & NS_STYLE_TOUCH_ACTION_NONE) {

The code here is fine, but please fix the indentation and remove trailing whitespace.

::: layout/base/nsDisplayList.h
@@ +2722,5 @@
>    nsRegion mMaybeHitRegion;
>    // These are points that need to be dispatched to the content thread for
>    // resolution. Always contained in the union of mHitRegion and mMaybeHitRegion.
>    nsRegion mDispatchToContentHitRegion;
> +  // These are points that lie in the touch action region when panning is disabled.

"These are points where panning is disabled, as determined by the touch-action property. Always contained in the union of mHitRegion and mMaybeHitRegion."

Update the other two accordingly as well.

::: layout/base/nsLayoutUtils.cpp
@@ +7931,5 @@
> +{
> +  // If aFrame is null then return default value
> +  if (!aFrame) {
> +		return NS_STYLE_TOUCH_ACTION_AUTO;
> +	}

Please fix indentation in this method.

@@ +7954,5 @@
> +
> +/* static */  nsRegion*
> +nsLayoutUtils::IsFramePrecise(nsIFrame* aFrame, const nsIFrame* aAncestor, nsRegion* aRegion, nsRegion* aDispatchToContentRegion, const nsRect aBounds, nsRect* aRect )
> +{
> +  *aRect = TransformFrameRectToAncestor(

See the comments I made in FrameLayerBuilder; replace this function with the TransformToAncestorAndCombineRegions function that I described there.

::: layout/base/nsLayoutUtils.h
@@ +2420,5 @@
> +	/**
> +	 * Helper method to get touch action behaviour from the frame
> +	 */
> +	static uint32_t
> +	GetTouchActionFromFrame(nsIFrame* aFrame);

Please fix the indentation here. There should be two-space indents for this.

@@ +2426,5 @@
> +	/**
> +	 * Helper method to get precision of the reference frame
> +	 */
> +	static  nsRegion*
> +	

Remove this blank line with whitespace
Attachment #8572584 - Flags: review?(bugmail.mozilla)
Attachment #8572584 - Flags: feedback?
Attachment #8572584 - Flags: feedback+
Attached patch addtouch.patch (obsolete) — Splinter Review
Hi kats,
I'm sorry for such a late reply. Got busy with college exams.
I have implemented the changes you asked. Please have a look, and if this bug requires to be applied on the latest code base, let me know. I will apply it.
Attachment #8572584 - Attachment is obsolete: true
Attachment #8581168 - Flags: feedback?(bugmail.mozilla)
Hi Kushan, the code in FrameLayerBuilder.cpp is still not quite what I asked for in comment 50. It might help to see the comments in context by clicking on the "review" link for attachment 8572584 [details] [diff] [review]. Sometimes the code snippets that splinter uses for the bug comments don't clearly show what I'm referring to. This latest patch still has things like "if (!data->mHorizontalPanRegion.GetBounds().IsEmpty())" in FrameLayerBuilder which I would like moved into nsLayoutUtils::TransformToAncestorAndCombineRegions. The indentation in nsDisplayLayerEventRegions::AddFrame is also not correct.
Attachment #8581168 - Flags: feedback?(bugmail.mozilla)
Kats, 
how am I going to handle the rect variable? Which is now committed according to the changes you suggest.
(In reply to Kushan Joshi [:kepta] from comment #53)
> how am I going to handle the rect variable?

The rect variable should also go inside the TransformToAncestorAndCombineRegions function.

> Which is now committed according
> to the changes you suggest.

I'm not sure what you mean by this. Specifically I don't know what you mean by "commit" in this context.
Hi kats,
I am really sorry, Can you please explain in detail  how to implement TransformToAncestorAndCombineRegions. What will be its arguments and how to implement it?
This is what I had in mind (I didn't compile it to make sure it works though):

/* static */ void
nsLayoutUtils::TransformToAncestorAndCombineRegions(
    const nsRect& aBounds,
    nsIFrame* aFrame,
    const nsIFrame* aAncestorFrame,
    nsRegion* aPreciseTargetDest,
    nsRegion* aImpreciseTargetDest)
{
  if (aBounds.IsEmpty()) {
    return;
  }
  Matrix4x4 matrix = GetTransformToAncestor(aFrame, aAncestorFrame);
  Matrix matrix2D;
  bool isPrecise = matrix.Is2D(&matrix2D)
      && !matrix2D.HasNonAxisAlignedTransform();
  nsRect transformed = TransformFrameRectToAncestor(
      aFrame, aBounds, aAncestorFrame);
  nsRegion* dest = isPrecise ? aPreciseTargetDest : aImpreciseTargetDest;
  dest->OrWith(transformed);
}

and then you can call it like so:

TransformToAncestorAndCombineRegions(
    data->mHitRegion.GetBounds(),
    mContainerReferenceFrame,
    containingPaintedLayerData->mReferenceFrame,
    &containingPaintedLayerData->mHitRegion,
    &containingPaintedLayerData->mMaybeHitRegion);
TransformToAncestorAndCombineRegions(
    data->mNoActionRegion.GetBounds(),
    mContainerReferenceFrame,
    containingPaintedLayerData->mReferenceFrame,
    &containingPaintedLayerData->mNoActionRegion,
    &containingPaintedLayerData->mDispatchToContentRegion);
TransformToAncestorAndCombineRegions(...mVerticalPan...);
TransformToAncestorAndCombineRegions(...mHorizontalPan...);
Attached patch addtouch.patch (obsolete) — Splinter Review
Hi,
Please review the code.

Thanks
Attachment #8581168 - Attachment is obsolete: true
Attachment #8581842 - Flags: review?(bugmail.mozilla)
Comment on attachment 8581842 [details] [diff] [review]
addtouch.patch

Review of attachment 8581842 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +2307,3 @@
>      }
> +    if (!data->mNoActionRegion.GetBounds().IsEmpty()) {
> +	  nsLayoutUtils::TransformToAncestorAndCombineRegions(

You still have IsEmpty() checks here which you don't need, as they have been moved inside the TransformToAncestorAndCombineRegions function.

::: layout/base/nsDisplayList.cpp
@@ +3225,5 @@
> +    mNoActionRegion.Or(mNoActionRegion, borderBox);
> +  } else {
> +	if ((touchAction & NS_STYLE_TOUCH_ACTION_PAN_X)) {
> +	  mHorizontalPanRegion.Or(mHorizontalPanRegion, borderBox);
> +	}

The indentation in this function is still not correct.
Attachment #8581842 - Flags: review?(bugmail.mozilla) → review-
Attached patch addtouch.patchSplinter Review
Please check now.
sorry for the errors.
Attachment #8581842 - Attachment is obsolete: true
Attachment #8581857 - Flags: review?(bugmail.mozilla)
Comment on attachment 8581857 [details] [diff] [review]
addtouch.patch

Review of attachment 8581857 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good, thanks. There's still a few tab characters which I replaced with spaces locally to fix up the indenting, and I pushed this patch to try to verify it passes tests. Once it comes back green we can land it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=979fd9467cbc
Attachment #8581857 - Flags: review?(bugmail.mozilla) → review+
I am really sorry for the tab and spacing problem, my text editor messes it up. Any suggestions for future?
Thanks a lot though, I really got to learn a lot from you.
(In reply to Kushan Joshi [:kepta] from comment #61)
> I am really sorry for the tab and spacing problem, my text editor messes it
> up. Any suggestions for future?

What editor do you use? Most editors can be configured to match our style guide for whitespace behaviour.
I use mostly Textmate, but sometimes use sublimetext as well.
(In reply to Kushan Joshi [:kepta] from comment #63)
> I use mostly Textmate, but sometimes use sublimetext as well.

Hey Kushan, I don't know about Textmate, but in sublime text you can change the "Settings - User" quite easily to, e.g.
{
	"draw_white_space": "all", // this makes it easy to spot a mix of spaces and tabs
	"translate_tabs_to_spaces": true, // changes your tabs to spaces
	"tab_size": 4, // or 2 or what ever the required size is
	"trim_trailing_white_space_on_save": true
	// etc.
}
Yeah in general most editors have a feature to use spaces instead of tabs, and doing that will avoid these problems. It's a really common issue though :)

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80be62413c65
https://hg.mozilla.org/mozilla-central/rev/80be62413c65
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Thanks for getting this done, Kushan!
You need to log in before you can comment on or make changes to this bug.