(intersection-observer) Map intersectionRect to the coordinate space of the target document

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: tschneider, Assigned: tschneider)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

2 years ago
Spec quote: "Map intersectionRect to the coordinate space of the viewport of the Document containing the target.".
(Assignee)

Updated

2 years ago
Blocks: 1358666
(In reply to Tobias Schneider [:tobytailor] from comment #0)
> Spec quote: "Map intersectionRect to the coordinate space of the viewport of
> the Document containing the target.".

Please add a link to the spec you're quoting.
(Assignee)

Comment 3

2 years ago
Round transformed rect to nearest pixels.
Attachment #8861501 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
Avoid crash on Android.
Attachment #8862916 - Attachment is obsolete: true
Needs a test. Please assume that any IntersectionObserver changes *require* tests.
Flags: needinfo?(tschneider)
(Assignee)

Comment 8

2 years ago
Thanks. I'm aware of that. Please notice that this is a dependency of Bug 1358666 which tracks efforts to get the web-platform tests for the IntersectionObserver to pass in FF. Certainly I could split out individual tests of the wet suite to attach to this bug, but it would still need Bug 1358666 for shared harness code.
Flags: needinfo?(tschneider)
(Assignee)

Comment 9

2 years ago
Re-enable web platform test.
Attachment #8863193 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
No longer blocks: 1358666
Depends on: 1358666
(Assignee)

Updated

2 years ago
Attachment #8868633 - Flags: review?(dholbert)
(Assignee)

Comment 10

2 years ago
Missed some tests.
Attachment #8868633 - Attachment is obsolete: true
Attachment #8868633 - Flags: review?(dholbert)
Attachment #8868642 - Flags: review?(dholbert)
I have 2 questions re: these patches...

observer & target are NOT in the same doc:
intersectionRect is mapped to the coordinate space of the target document. This is the only case this fix is addressing. Does this need origin checks?

observer & target are in the same doc:
The TransformFrameRectToAncestor code was removed in this case, and it's not clear what intersectionRect should contain. Do we have a test for this case?
(Assignee)

Comment 12

2 years ago
(In reply to Jet Villegas (:jet) from comment #11)
> I have 2 questions re: these patches...
> 
> observer & target are NOT in the same doc:
> intersectionRect is mapped to the coordinate space of the target document.
> This is the only case this fix is addressing. Does this need origin checks?

We don't, this should happen independent of origin. We only need special cross-origin handling for rootBounds.

> observer & target are in the same doc:
> The TransformFrameRectToAncestor code was removed in this case, and it's not
> clear what intersectionRect should contain. Do we have a test for this case?

The old TransformFrameRectToAncestor was kind of redundant. In this case intersectionRect will be relative to root, cause intersectionRectRelativeToRoot is. And yes, the wpt suite will contain tests for this.
Assignee: nobody → tschneider
(Assignee)

Comment 13

2 years ago
Rebased patch.
Attachment #8868642 - Attachment is obsolete: true
Attachment #8868642 - Flags: review?(dholbert)
(Assignee)

Updated

2 years ago
Attachment #8873565 - Flags: review?(mstange)
Comment on attachment 8873565 [details] [diff] [review]
Map intersectionRect to the coordinate space of the target document

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

::: dom/base/DOMIntersectionObserver.cpp
@@ +416,5 @@
> +        nsPresContext* presContext = targetFrame->PresContext();
> +        nscoord appUnitsPerDevPixel = presContext->AppUnitsPerDevPixel();
> +        nsLayoutUtils::TransformRect(rootFrame,
> +          presContext->PresShell()->GetRootScrollFrame(), rect);
> +          gfx::IntRect roundedRect = rect.ToNearestPixels(appUnitsPerDevPixel);

Why is the rounding necessary? I'd rather not do it.
(Assignee)

Updated

2 years ago
Blocks: 1369363
(Assignee)

Comment 15

2 years ago
> Why is the rounding necessary? I'd rather not do it.

I experienced some precision issues (on sub-app-unit level) in the result of TransformRect even in case dealing with mostly identity matrices. Rounding here was the only way I found to solve it.
What numbers did you get? I'd really like to find a different solution.
(Assignee)

Comment 17

2 years ago
It's usually off by 1 app unit (in my latest test it was 479 vs 480).
Please give me a full example with all the coordinates and transforms that are involved in the calculation that goes bad.
(Assignee)

Comment 19

2 years ago
Actually not really an issue of matrix math than with typecasting. What happens in TransformRect with an input value of int x = 480 (app units) is the following:

> float devPixelsPerAppUnitFromFrame = 1.0f / aFromFrame->PresContext()->AppUnitsPerDevPixel();

devPixelsPerAppUnitFromFrame at this point is 0.0333333351

> x = x * devPixelsPerAppUnitFromFrame // should be 16.000000848 but looses precision due to float to integer cast so x ends up being 16

At a later point x then gets transformed back to app units via:

> x = x / devPixelsPerAppUnitFromFrame // again, should be 480 but instead is 479.99997456000136 casted to 479, loosing the mentioned 1 app unit
(Assignee)

Comment 20

2 years ago
I guess the rounding should rather happen in TransformRect when assigning x?
Yes, that sounds like it would fix this. TransformRectTo*Point* already uses NSToCoordRound in the same place. Can you try this out, and if it works, submit it as a patch in a separate bug?
(Assignee)

Updated

2 years ago
Depends on: 1369870
(Assignee)

Comment 22

2 years ago
Don't round values, instead depend on bug 1369870 to have it happen in nsLayoutUtils::TransformRect.
Attachment #8873565 - Attachment is obsolete: true
Attachment #8873565 - Flags: review?(mstange)
Attachment #8873997 - Flags: review?(mstange)
Attachment #8873997 - Flags: review?(mstange) → review+
(Assignee)

Comment 24

2 years ago
Removed unused variable.
Attachment #8873997 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 26

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/279a08ffe577
(intersection-observer) Map intersectionRect to the coordinate space of the target document. r=mstange
Keywords: checkin-needed

Comment 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/279a08ffe577
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1370968

Comment 28

8 months ago
I may incorrectly understand this but it does not seem to work correctly even for current MDN example.
Here is a version that prints out intersection rect and bounds rect
https://jsfiddle.net/8f9whxbk/11/

In Chrome 68.0.3440.106 it prints matching rects which is excpected
intersection: left: 10 top: 10 right: 508 bottom: 508
bounds: left: 10 top: 10 right: 508 bottom: 508

In Firefox 61.0.2 it prints
intersection: left: 1138.300048828125 top: 555.2999877929688 right: 1536.300048828125 bottom: 953.2999877929688
bounds: left: 8 top: 8 right: 406 bottom: 406

So they do not match at all

It seems that intersection rect is in parent iframe coordinates instead of its own document coordinates which does not seem right.

Is this a regression?
You need to log in before you can comment on or make changes to this bug.