Closed Bug 1359317 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: tschneider, Assigned: tschneider)

References

Details

Attachments

(1 file, 4 obsolete files)

intersectionRect should be calculated without css transform to avoid it being applied twice in nsLayoutUtils::TransformFrameRectToAncestor.
Blocks: 1332922
Blocks: 1358666
This needs a test.
Don't block enabling web platform tests.
Attachment #8861499 - Attachment is obsolete: true
Attachment #8868637 - Flags: review?(dholbert)
No longer blocks: 1358666
Depends on: 1358666
Assignee: nobody → tschneider
Status: NEW → ASSIGNED
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.)
Flags: needinfo?(tschneider)
Rebased patch.
Attachment #8868637 - Attachment is obsolete: true
Attachment #8868637 - Flags: review?(dholbert)
Uses GetContentRectRelativeToSelf to make sure the initial intersection rect doesn't include any transformation.
Attachment #8878235 - Attachment is obsolete: true
Attachment #8878705 - Flags: review?(mstange)
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 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?
No, they simply test if positioning via CSS translate() is taking into account.
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.
Confirm mustang's concern to use GetRectRelativeToSelf instead. Added test to prove.
Attachment #8878705 - Attachment is obsolete: true
Attachment #8878705 - Flags: review?(mstange)
Attachment #8878930 - Flags: review?(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.
Attachment #8878930 - Flags: review?(mstange) → review+
Pushed by tschneider@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/197b644d2d4b
(intersection-observer) Use targetFrame->GetRectRelativeToSelf() as the initial intersection rect. r=mstange
https://hg.mozilla.org/mozilla-central/rev/197b644d2d4b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Duplicate of this bug: 1377581
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
Attachment #8878930 - Flags: approval-mozilla-beta?
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.
Attachment #8878930 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1332922
You need to log in before you can comment on or make changes to this bug.