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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Tobias Schneider [:tobytailor]]

Spec quote: "Map intersectionRect to the coordinate space of the viewport of the Document containing the target.".

Comment 1

[Tobias Schneider [:tobytailor]]

Attachment: Map intersectionRect to the coordinate space of the target document

Comment 2

[Jet Villegas (inactive)]

(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.

Comment 3

[Tobias Schneider [:tobytailor]]

Attachment: Map intersectionRect to the coordinate space of the target document

Round transformed rect to nearest pixels.

Comment 4

[Tobias Schneider [:tobytailor]]

See https://wicg.github.io/IntersectionObserver/#calculate-intersection-rect-algo, point 6.

Comment 5

[Tobias Schneider [:tobytailor]]

Attachment: Map intersectionRect to the coordinate space of the target document

Comment 6

[Tobias Schneider [:tobytailor]]

Attachment: Map intersectionRect to the coordinate space of the target document

Avoid crash on Android.

Comment 7

[Jet Villegas (inactive)]

Needs a test. Please assume that any IntersectionObserver changes *require* tests.

Comment 8

[Tobias Schneider [:tobytailor]]

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.

Comment 9

[Tobias Schneider [:tobytailor]]

Attachment: Map intersectionRect to the coordinate space of the target document

Re-enable web platform test.

Comment 10

[Tobias Schneider [:tobytailor]]

Attachment: Map intersectionRect to the coordinate space of the target document

Missed some tests.

Comment 11

[Jet Villegas (inactive)]

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?

Comment 12

[Tobias Schneider [:tobytailor]]

(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.

Comment 13

[Tobias Schneider [:tobytailor]]

Attachment: Map intersectionRect to the coordinate space of the target document

Rebased patch.

Comment 14

[Markus Stange [: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.

Comment 15

[Tobias Schneider [:tobytailor]]

> 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.

Comment 16

[Markus Stange [:mstange]]

What numbers did you get? I'd really like to find a different solution.

Comment 17

[Tobias Schneider [:tobytailor]]

It's usually off by 1 app unit (in my latest test it was 479 vs 480).

Comment 18

[Markus Stange [:mstange]]

Please give me a full example with all the coordinates and transforms that are involved in the calculation that goes bad.

Comment 19

[Tobias Schneider [:tobytailor]]

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

Comment 20

[Tobias Schneider [:tobytailor]]

I guess the rounding should rather happen in TransformRect when assigning x?

Comment 21

[Markus Stange [:mstange]]

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?

Comment 22

[Tobias Schneider [:tobytailor]]

Attachment: Map intersectionRect to the coordinate space of the target document

Don't round values, instead depend on bug 1369870 to have it happen in nsLayoutUtils::TransformRect.

Comment 23

[Tobias Schneider [:tobytailor]]

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

Comment 24

[Tobias Schneider [:tobytailor]]

Attachment: Map intersectionRect to the coordinate space of the target document. r=mstange

Removed unused variable.

Comment 25

[Tobias Schneider [:tobytailor]]

New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecebdc65b033c009265478163aec79eaf416f676

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/279a08ffe577
(intersection-observer) Map intersectionRect to the coordinate space of the target document. r=mstange

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/279a08ffe577

Comment 28

[wonderwhy.er]

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?