Closed
Bug 1147279
Opened 9 years ago
Closed 9 years ago
The event region seems wrong if element becomes image layer.
Categories
(Core :: Layout, defect)
Tracking
()
People
(Reporter: mtseng, Assigned: mtseng)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(5 files, 4 obsolete files)
4.29 KB,
text/html
|
Details | |
31.72 KB,
image/png
|
Details | |
4.09 KB,
text/html
|
Details | |
3.45 KB,
patch
|
roc
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
3.01 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
Upload correct image.
Attachment #8582948 -
Attachment is obsolete: true
Comment 3•9 years ago
|
||
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)
Updated•9 years ago
|
Whiteboard: [gfx-noted]
Updated•9 years ago
|
Component: Panning and Zooming → Layout
Comment 4•9 years ago
|
||
I'm guessing that the transform that ConfigureLayer puts on the image layer isn't getting factored in anywhere.
Assignee | ||
Comment 5•9 years ago
|
||
I'm not sure this is right solution or not.
Attachment #8583679 -
Flags: review?(tnikkel)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 years ago
|
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-
Assignee | ||
Comment 8•9 years ago
|
||
Addressed to reviewer's comment.
Attachment #8583679 -
Attachment is obsolete: true
Attachment #8591451 -
Flags: review?(roc)
Updated•9 years ago
|
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-
Assignee | ||
Comment 10•9 years ago
|
||
Invert scale as roc mentioned.
Attachment #8591451 -
Attachment is obsolete: true
Attachment #8592085 -
Flags: review?(roc)
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•9 years ago
|
||
(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)
Assignee | ||
Comment 13•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d85a7922118
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)
Comment 16•9 years ago
|
||
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)
Comment 17•9 years ago
|
||
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.
Assignee | ||
Comment 18•9 years ago
|
||
Using Matrix::Invert().
Attachment #8592085 -
Attachment is obsolete: true
Attachment #8592627 -
Flags: review?(roc)
Assignee | ||
Comment 19•9 years ago
|
||
try https://treeherder.mozilla.org/#/jobs?repo=try&revision=795fc63d736e
Attachment #8592627 -
Flags: review?(roc) → review+
Updated•9 years ago
|
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 20•9 years ago
|
||
Something wrong with Gij 24 on b2g desktop. Retry with latest m-c. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4dc056a506c
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c508fdc8ead0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 23•9 years ago
|
||
Please request b2g37 approval on this patch when you get a chance.
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → fixed
status-firefox38:
--- → wontfix
status-firefox39:
--- → wontfix
Flags: needinfo?(mtseng)
Assignee | ||
Comment 24•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8592627 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Assignee | ||
Comment 26•9 years ago
|
||
Assignee | ||
Comment 27•9 years ago
|
||
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.
Description
•