Closed Bug 1406102 Opened 2 years ago Closed 2 years ago

IntersectionObserverEntry intersectionRatio value is sometimes negative

Categories

(Core :: Layout, defect, P3)

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: balbuf, Assigned: tschneider)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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).
Component: Untriaged → Layout
Product: Firefox → Core
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.
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.
Was able to reproduce. Will have a look into this. Thanks for reporting.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → tschneider
Priority: -- → P3
Blocks: 1381574
Comment on attachment 8916121 [details] [diff] [review]
Convert dimensions to doubles before calculating areas

Since you're around :).
Attachment #8916121 - Flags: review?(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 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.
Attachment #8916121 - Flags: review?(dholbert) → review-
Addresses review comments.
Attachment #8916121 - Attachment is obsolete: true
Attachment #8916716 - Flags: review?(dholbert)
Comment on attachment 8916716 [details] [diff] [review]
Calculate areas using int64_t

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

Looks good -- thanks!
Attachment #8916716 - Flags: review?(dholbert) → review+
Pushed by tschneider@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e71258981e43
[intersection-observer] Calculate areas using int64_t. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/e71258981e43
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.