1359317 - (intersection-observer) Initial intersectionRect should not include css transform

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Tobias Schneider [:tobytailor]]

intersectionRect should be calculated without css transform to avoid it being applied twice in nsLayoutUtils::TransformFrameRectToAncestor.

Comment 1

[Tobias Schneider [:tobytailor]]

Attachment: Don't include CSS transform in initial intersectionRect

Comment 2

[Jet Villegas (inactive)]

This needs a test.

Comment 3

[Tobias Schneider [:tobytailor]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1359318#c8.

Comment 4

[Tobias Schneider [:tobytailor]]

Attachment: Don't include CSS transform in initial intersectionRect

Don't block enabling web platform tests.

Comment 5

[Daniel Holbert [:dholbert]]

Am I correct in understanding that attachment 8853597 [details] (from bug 1332922) is a testcase for this bug?  (Asking because it's helpful for me to see an example of something that's broken when reviewing a patch.  It helps me understand what's broken & how the patch might help.)

Comment 6

[Tobias Schneider [:tobytailor]]

Yes, others are https://bugzilla.mozilla.org/attachment.cgi?id=8868379&action=diff#a/testing/web-platform/meta/intersection-observer/edge-inclusive-intersection.html.ini_sec1 and https://bugzilla.mozilla.org/attachment.cgi?id=8868379&action=diff#a/testing/web-platform/meta/intersection-observer/unclipped-root.html.ini_sec1.

Comment 7

[Tobias Schneider [:tobytailor]]

Attachment: Don't include CSS transform in initial intersectionRect

Rebased patch.

Comment 8

[Tobias Schneider [:tobytailor]]

Attachment: Don't include CSS transform in initial intersectionRect

Uses GetContentRectRelativeToSelf to make sure the initial intersection rect doesn't include any transformation.

Comment 9

[Tobias Schneider [:tobytailor]]

Problem here was that GetVisualOverflowRect applied the CSS transformation, which we have to avoid since it will taken into account later in the algorithm. Also, see https://bugzilla.mozilla.org/show_bug.cgi?id=1363650#c12.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8878705 [details] [diff] [review]
Don't include CSS transform in initial intersectionRect

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

Are there tests about what should happen with padding / border / box-shadow?

Comment 11

[Tobias Schneider [:tobytailor]]

No, they simply test if positioning via CSS translate() is taking into account.

Comment 12

[Markus Stange [:mstange]]

Then please create a testcase and compare its behavior in Chrome and Firefox to see whether padding and border should be treated as part of the frame or not, so that you know whether to call GetContentRectRelativeToSelf or GetRectRelativeToSelf.

Comment 13

[Tobias Schneider [:tobytailor]]

Attachment: Don't include CSS transform in initial intersectionRect

Confirm mustang's concern to use GetRectRelativeToSelf instead. Added test to prove.

Comment 14

[Tobias Schneider [:tobytailor]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=705c23204c424205cc95a387794697ceb2f8e4e4

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8878930 [details] [diff] [review]
Don't include CSS transform in initial intersectionRect

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

Thanks for adding the test.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/197b644d2d4b
(intersection-observer) Use targetFrame->GetRectRelativeToSelf() as the initial intersection rect. r=mstange

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/197b644d2d4b

Comment 19

[Tobias Schneider [:tobytailor]]

Comment on attachment 8878930 [details] [diff] [review]
Don't include CSS transform in initial intersectionRect

[Feature/Bug causing the regression]: 1321865
[User impact if declined]: Incompatibility with Chrome.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes, this fix has been in Nightly for ~3 weeks.
[Is the change risky?]: No

Comment 20

[Liz Henry (:lizzard)]

Comment on attachment 8878930 [details] [diff] [review]
Don't include CSS transform in initial intersectionRect

Needed for the Intersection Observer API, let's uplift for beta 9.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/058ee78afe6f