1406102 - IntersectionObserverEntry intersectionRatio value is sometimes negative

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[balbuf]

Attachment: Screen Shot 2017-10-05 at 11.52.29 AM.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

In console:
var observer = new IntersectionObserver(console.log);
observer.observe(document.body);


Actual results:

An IntersectionObserverEntry object is logged to the console and contains a negative value for intersectionRatio (see attached screenshot).


Expected results:

Per the spec, the intersectionRatio value is supposed to be:

> If the boundingClientRect has non-zero area, this will be the ratio of intersectionRect area to boundingClientRect area. Otherwise, this will be 1 if the isIntersecting is true, and 0 if not.

https://www.w3.org/TR/intersection-observer/#dom-intersectionobserverentry-intersectionratio

Considering this is a ratio between two areas, which by definition are always positive or zero, a negative value for this property is illogical. There is nothing in the spec that leads me to believe that this value should ever be less than 0.

This seems to only occur when the target element is larger than the root element (in my example, the body which is larger than the viewport).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It would probably help to have a testcase on which this can be reproduced.  Simplest way to do that is to give the URL of the page where this happens.

Comment 2

[balbuf]

It appears to happen on any page where the target element is larger than the root (though I've only tried with the viewport being the root) using the "Steps to Reproduce" above. As an example, it happens on this current bug report page (assuming the viewport is small enough that the body is larger). By comparison, a shorter page, such as the homepage for this site (https://bugzilla.mozilla.org/) does not have this issue when the viewport is large enough to fit the entire body.

Comment 3

[Tobias Schneider [:tobytailor]]

Was able to reproduce. Will have a look into this. Thanks for reporting.

Comment 4

[Tobias Schneider [:tobytailor]]

Attachment: Convert dimensions to doubles before calculating areas

Comment 5

[Tobias Schneider [:tobytailor]]

Comment on attachment 8916121 [details] [diff] [review]
Convert dimensions to doubles before calculating areas

Since you're around :).

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8916121 [details] [diff] [review]
Convert dimensions to doubles before calculating areas

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

::: dom/base/DOMIntersectionObserver.cpp
@@ +438,5 @@
>  
> +    double targetArea =
> +      (double) targetRect.Width() * (double) targetRect.Height();
> +    double intersectionArea = !intersectionRect ? 0 :
> +      (double) intersectionRect->Width() * (double) intersectionRect->Height();

Is there a reason these values are double?

They're the result of multiplying two integers, so it seems like they should be integers (int64_t to avoid the possibility of overflow).

So I think this section wants s/double/int64_t/ -- and then below, we should cast them to double for the intersectionRatio calculation.

(This will probably produce the same results, but it makes more sense I think -- we'll be keeping integer operations in integer space, and only converting to floating-point once we're actually coming up with a possibly-fractional ratio.)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8916121 [details] [diff] [review]
Convert dimensions to doubles before calculating areas

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

r- on this version, because (per comment 6) I think we should be upgrading this to an int64_t multiply operation, rather than a double multiply operation -- and we should only convert to double once we have to divide.

(I'll grant swift r+ once that and the test-wording-nit are addressed, though!)

::: dom/base/test/test_bug1406102.html
@@ +23,5 @@
> +<pre id="test">
> +<script type="application/javascript">
> +
> +  let observer = new IntersectionObserver(function (changes) {
> +    ok(changes[0].intersectionRatio > 0, 'intersectionRatio greater than zero');

s/greater/should be greater/

Otherwise, when this message is displayed (i.e. when the test fails), it's unclear whether this wording ("intersectionRatio greater than zero") is describing the problematic condition vs. describing the expected condition.

Comment 8

[Tobias Schneider [:tobytailor]]

Attachment: Calculate areas using int64_t

Addresses review comments.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8916716 [details] [diff] [review]
Calculate areas using int64_t

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

Looks good -- thanks!

Comment 10

[Tobias Schneider [:tobytailor]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=799d681c8b1e0a12a06aa0ae9a69b6a50bbcf14b&selectedJob=135774264

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e71258981e43
[intersection-observer] Calculate areas using int64_t. r=dholbert

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e71258981e43