Spec quote: "Map intersectionRect to the coordinate space of the viewport of the Document containing the target.".
(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.
Round transformed rect to nearest pixels.
Attachment #8861501 - Attachment is obsolete: true
Avoid crash on Android.
Attachment #8862916 - Attachment is obsolete: true
Needs a test. Please assume that any IntersectionObserver changes *require* tests.
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.
Re-enable web platform test.
Attachment #8863193 - Attachment is obsolete: true
Missed some tests.
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?
(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.
2 years ago
Assignee: nobody → tschneider
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.
> 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.
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.
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
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?
Don't round values, instead depend on bug 1369870 to have it happen in nsLayoutUtils::TransformRect.
Attachment #8873997 - Flags: review?(mstange) → review+
Removed unused variable.
Attachment #8873997 - Attachment is obsolete: true
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/279a08ffe577 (intersection-observer) Map intersectionRect to the coordinate space of the target document. r=mstange
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.