The event region seems wrong if element becomes image layer.

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mtseng, Assigned: mtseng)

Tracking

unspecified
mozilla40
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox38 wontfix, firefox39 wontfix, firefox40 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(5 attachments, 4 obsolete attachments)

Posted file index.html
I found this bug when implementing bug 1021499. Please see bug 1021499 comment 24.

I write a simple test case for this.

The key css attribute is "background-color:green" on the #caretfake.

If we have that css attribute, #caretfake becomes painted layer and everything works great.

But if we don't have that css attribute, #caretfake becomes image layer and drag on #caretfake would trigger apz scrolling.
Flags: needinfo?(bugmail.mozilla)
Posted image index.png (obsolete) —
I did some experiment. I turn on APZCTM log and see what different between good case and bad one. As the attachment, I hit 2 point on screen, one is near top left corner of #caretfake, another one is near bottom right. Here are logs.

===================
This is good case. That is, add background-color to #caretfake. So #caretfake is painted layer.

========Tap on top-left corner=============
I/Gecko   (13478): APZCTM: Testing ParentLayer point (124,256) (Layer (124,256)) against node 0xad410a60
I/Gecko   (13478): mEventRegions.mHitRegion [0,0,480,45; 0,90,30,794; 450,90,480,794]
I/Gecko   (13478): APZCTM: Testing ParentLayer point (124,211) (Layer (124,211)) against node 0xad410c40
I/Gecko   (13478): mEventRegions.mHitRegion [75,75,267,363]
I/Gecko   (13478): APZCTM: Successfully matched APZC 0xaf021000 via node 0xad410c40 (hit result 2)
I/Gecko   (13478): Morris hitResult:2
I/Gecko   (13478): APZCTM: OverscrollHandoffChain[0] = 0xaf021000
I/Gecko   (13478): APZCTM: OverscrollHandoffChain[1] = 0xaf0f1000
I/Gecko   (13478): APZCTM: Re-using APZC 0xaf021000 as continuation of event block
=============Log end==================

============Tap on bottom right corner===============
I/Gecko   (13478): APZCTM: Testing ParentLayer point (237,386) (Layer (237,386)) against node 0xad410a60
I/Gecko   (13478): mEventRegions.mHitRegion [0,0,480,45; 0,90,30,794; 450,90,480,794]
I/Gecko   (13478): APZCTM: Testing ParentLayer point (237,341) (Layer (237,341)) against node 0xad410c40
I/Gecko   (13478): mEventRegions.mHitRegion [75,75,267,363]
I/Gecko   (13478): APZCTM: Successfully matched APZC 0xaf021000 via node 0xad410c40 (hit result 2)
I/Gecko   (13478): Morris hitResult:2
I/Gecko   (13478): APZCTM: OverscrollHandoffChain[0] = 0xaf021000
I/Gecko   (13478): APZCTM: OverscrollHandoffChain[1] = 0xaf0f1000
I/Gecko   (13478): APZCTM: Re-using APZC 0xaf021000 as continuation of event block
=============Log end=================

===========================
Following is bad case. No background-color attribute. Layer is image layer.

==============Tap on top left corner==============      <===this is bad one!!!
I/Gecko   (13478): APZCTM: Testing ParentLayer point (122,280) (Layer (122,280)) against node 0xad410a60
I/Gecko   (13478): mEventRegions.mHitRegion [0,0,480,45; 0,90,30,794; 450,90,480,794]
I/Gecko   (13478): APZCTM: Testing ParentLayer point (122,235) (Layer (31.3333,42.6667)) against node 0xad410c40
I/Gecko   (13478): mEventRegions.mHitRegion [75,75,267,363]
I/Gecko   (13478): APZCTM: Point 122.000000 235.000000 outside clip for node 0xad36a200
I/Gecko   (13478): APZCTM: Testing ParentLayer point (122,235) (Layer (110,223)) against node 0xad36a6b0
I/Gecko   (13478): mEventRegions.mHitRegion [0,0,450,6000]
I/Gecko   (13478): APZCTM: Successfully matched APZC 0xaffc3800 via node 0xad36a6b0 (hit result 1)
I/Gecko   (13478): Morris hitResult:1
I/Gecko   (13478): APZCTM: OverscrollHandoffChain[0] = 0xaffc3800
I/Gecko   (13478): APZCTM: OverscrollHandoffChain[1] = 0xaffbe000
I/Gecko   (13478): APZCTM: OverscrollHandoffChain[2] = 0xaf0f1000
I/Gecko   (13478): APZCTM: Re-using APZC 0xaffc3800 as continuation of event block
===========Log end=================

==============Tap on bottom-right corner================
I/Gecko   (13478): APZCTM: Testing ParentLayer point (229,368) (Layer (229,368)) against node 0xad410a60
I/Gecko   (13478): mEventRegions.mHitRegion [0,0,480,45; 0,90,30,794; 450,90,480,794]
I/Gecko   (13478): APZCTM: Testing ParentLayer point (229,323) (Layer (102.667,101.333)) against node 0xad410c40
I/Gecko   (13478): mEventRegions.mHitRegion [75,75,267,363]
I/Gecko   (13478): APZCTM: Successfully matched APZC 0xaffbe000 via node 0xad410c40 (hit result 2)
I/Gecko   (13478): Morris hitResult:2
I/Gecko   (13478): APZCTM: OverscrollHandoffChain[0] = 0xaffbe000
I/Gecko   (13478): APZCTM: OverscrollHandoffChain[1] = 0xaf0f1000
I/Gecko   (13478): APZCTM: Re-using APZC 0xaffbe000 as continuation of event block
I/Gecko   (13478): APZCTM: Re-using APZC 0xaffbe000 as continuation of event block
==============Log end==================


From the log, looks like hitTestPointForChildLayers is odd on the bad case. But I'm not sure why it is odd.
Posted image index.png
Upload correct image.
Attachment #8582948 - Attachment is obsolete: true
Hm, this does look like a bug in the event regions code. I don't think the problem is so much that the layer becomes an ImageLayer, but that the transform on the layer isn't accounted for properly in the event regions. This is what the layer looks like when I load the attached test page in OS X nightly with APZ enabled:

    ClientImageLayer (0x11936e400) [clip=(x=50, y=50, w=128, h=192)] [transform=[ 1 0; 0 1; 50 114; ]] [bounds=(x=50, y=114, w=128, h=128)] [visible=< (x=0, y=0, w=128, h=128); >] { hitregion=< (x=50, y=50, w=128, h=192); > dispatchtocontentregion=< (x=50, y=50, w=128, h=192); >} [opaqueContent] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1068.000000, h=514.000000)] [sr=(x=0.000000, y=0.000000, w=1068.000000, h=514.000000)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=1068.000000, h=514.000000)] [cdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000)] [color=rgba(255, 255, 255, 1)] [scrollId=41]

Note that the hitregion and dispatchtocontentregion have their top-left at (50,50) but because the layer is transformed I would expect them to have their top left at (0,-64) instead. In order words, they should be in the same coordinate space as the visible region but they are not. I think the visible region is getting transformed at [1] but the event regions (and layer bounds, for that matter) are not.

tn, any thoughts here?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=99153c410bad#3280
Flags: needinfo?(bugmail.mozilla) → needinfo?(tnikkel)
Component: Panning and Zooming → Layout
I'm guessing that the transform that ConfigureLayer puts on the image layer isn't getting factored in anywhere.
I'm not sure this is right solution or not.
Attachment #8583679 - Flags: review?(tnikkel)
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Attachment #8583679 - Flags: review?(tnikkel) → review?(roc)
Comment on attachment 8583679 [details] [diff] [review]
Get correct translation when layer optimize away.

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2482,5 @@
> +      translation = -GetTranslationForPaintedLayer(data->mLayer);
> +    } else {
> +      Point pt = layer->GetBaseTransform().As2D().GetTranslation();
> +      translation.x = -pt.x;
> +      translation.y = -pt.y;

The transform might not be just a translation, there can be a scale as well.

To simplify this, I suggest adding to EventRegions a method that applies a translation and a scale to each region. Call that method from each branch here, removing the 'translation' local variable. That method should call nsIntRegion::ScaleRoundOut, and nsRegion::ScaleRoundOut should add an early exit path when aXScale and aYScale are both 1.0f.
Attachment #8583679 - Flags: review?(roc) → review-

Comment 7

4 years ago
This blocks a 2.2 blocker, make it 2.2+
blocking-b2g: --- → 2.2+
Addressed to reviewer's comment.
Attachment #8583679 - Attachment is obsolete: true
Attachment #8591451 - Flags: review?(roc)
Blocks: 1106440
No longer blocks: 928833
Comment on attachment 8591451 [details] [diff] [review]
Get correct translation when layer optimize away v2.

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

::: layout/base/FrameLayerBuilder.cpp
@@ +3043,5 @@
>          ScaleRegionToOutsidePixels(data->mDispatchToContentHitRegion));
>      regions.mHitRegion.OrWith(maybeHitRegion);
>  
> +    Matrix mat = layer->GetBaseTransform().As2D();
> +    regions.ApplyTranslationAndScale(-mat._31, -mat._32, mat._11, mat._22);

I think we should be inverting the scale here, just like we invert the translation. In fact we probably should invert the whole 2D matrix...

We need a test for this. Should be pretty easy; create a document with a CSS-transformed image, where the image has been scaled by the nsImageFrame, e.g. <img width="..." height="..."> setting the width and height to twice the intrinsic size.
Attachment #8591451 - Flags: review?(roc) → review-
Invert scale as roc mentioned.
Attachment #8591451 - Attachment is obsolete: true
Attachment #8592085 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 8591451 [details] [diff] [review]
> Get correct translation when layer optimize away v2.
> 
> Review of attachment 8591451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +3043,5 @@
> >          ScaleRegionToOutsidePixels(data->mDispatchToContentHitRegion));
> >      regions.mHitRegion.OrWith(maybeHitRegion);
> >  
> > +    Matrix mat = layer->GetBaseTransform().As2D();
> > +    regions.ApplyTranslationAndScale(-mat._31, -mat._32, mat._11, mat._22);
> 
> I think we should be inverting the scale here, just like we invert the
> translation. In fact we probably should invert the whole 2D matrix...
> 
> We need a test for this. Should be pretty easy; create a document with a
> CSS-transformed image, where the image has been scaled by the nsImageFrame,
> e.g. <img width="..." height="..."> setting the width and height to twice
> the intrinsic size.

I was able to create a test case like comment 11 and test it locally. But I have no idea how to test it in our test framework. Looks like our test like mochitest or reftest cannot test event region. roc, do you have any suggestion? Thanks.
Flags: needinfo?(roc)
Comment on attachment 8592085 [details] [diff] [review]
Get correct translation when layer optimize away v3.

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

::: layout/base/FrameLayerBuilder.cpp
@@ +3043,5 @@
>          ScaleRegionToOutsidePixels(data->mDispatchToContentHitRegion));
>      regions.mHitRegion.OrWith(maybeHitRegion);
>  
> +    Matrix mat = layer->GetBaseTransform().As2D();
> +    regions.ApplyTranslationAndScale(-mat._31, -mat._32, 1.0f / mat._11, 1.0f / mat._22);

I'm afraid this isn't correct when there's both a translation and a scale. The scale needs to be factored into the translation part. You might just want to invert 'mat'.
Attachment #8592085 - Flags: review?(roc) → review-
(In reply to Morris Tseng [:mtseng] from comment #12)
> I was able to create a test case like comment 11 and test it locally. But I
> have no idea how to test it in our test framework. Looks like our test like
> mochitest or reftest cannot test event region. roc, do you have any
> suggestion? Thanks.

Hmm, I don't know. Kats might know.
Flags: needinfo?(roc) → needinfo?(bugmail.mozilla)
At this point we don't have much of a mechanism to test event regions properly. I discussed this with Markus just now and we agreed that it makes sense to write tests that synthesize native events to exericse the APZ code which uses the event regions produced by layout. I landed bug 1146349 this morning which should make it easier to write such tests, and I hope to get a bunch of these tests landed in Q2.
Flags: needinfo?(bugmail.mozilla)
For now what would help though is just keeping a simple test page that demonstrates the problem with STR, then we can convert it to a full automated test later.
Using Matrix::Invert().
Attachment #8592085 - Attachment is obsolete: true
Attachment #8592627 - Flags: review?(roc)
Flags: needinfo?(tnikkel)
Something wrong with Gij 24 on b2g desktop. Retry with latest m-c.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4dc056a506c
https://hg.mozilla.org/mozilla-central/rev/c508fdc8ead0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Please request b2g37 approval on this patch when you get a chance.
Flags: needinfo?(mtseng)
Comment on attachment 8592627 [details] [diff] [review]
Get correct translation when layer optimize away v4.

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 #): 
User impact if declined: APZ behavior would be wrong if there are image layers.
Testing completed: on master.
Risk to taking this patch (and alternatives if risky): Should be low risk.
String or UUID changes made by this patch: none.
Flags: needinfo?(mtseng)
Attachment #8592627 - Flags: approval-mozilla-b2g37?
Attachment #8592627 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Needs rebasing around bug 1101627.
Flags: needinfo?(mtseng)
Ryan, I have uploaded rebasing patch based on latest b2g37 branch.
Flags: needinfo?(mtseng)
You need to log in before you can comment on or make changes to this bug.