1665447 - IntersectionObserverEntry.intersectionRect for an ib split element has incorrect values

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[account]

Attachment: intersection-rect-ib-split.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:82.0) Gecko/20100101 Firefox/82.0

Steps to reproduce:

1. Render an inline element that contains a block element. Ensure the inline element is fully visible in the viewport
2. Create an IntersectionObserver and observe the inline element

Actual results:

When the IntersectionObserver fires, the IntersectionObserverEntry.intersectionRect has incorrect values. These values do not match up with the boundingClientRect values.

Expected results:

Since the observed element is fully visible (intersectionRatio === 1), I would expect that the boundingClientRect and intersectionRect properties have the same values (top/left/height/width). This is true in Safari (13.1.1), Chrome (85.0.4183.102), Chrome Canary (87.0.4265.0), and Edge (86.0.622.3). Failing in Firefox :( (82.0a1)

Comment 1

[account]

This is pretty similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1581876, but for different properties of the IntersectionObserverEntry (intersectionRect instead of intersectionRatio/isIntersecting). The attached file in the original comment is a modification of the test case added in 1581876 (https://hg.mozilla.org/mozilla-central/rev/8992f6cfc323)

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Yeah, good catch, I should've tested intersectionRect as well :(

The good thing is that the boundingClientRect matches at least :P

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1665447 - Fix BoxToRect when the "relative to" frame is not an ancestor. r=dholbert,mstange

Right now the BoxToRect callback assumes that when it gets a
RECTS_ACCOUNT_FOR_TRANSFORMS flag, the "relative to" is an ancestor.

This holds for all the callers of GetAllInFlowRectsUnion except for 1,
which was introduced in bug 1581876, because they pass
GetContainingBlockForClientRect (that is, the root frame) as the root.

But that caller passes target, so IB split continuations or such would
get two siblings as the from/to frames, messing up the bounds.

An alternative would be to not pass the RECTS_ACCOUNT_FOR_TRANSFORMS
flag for that call (as it's computing a rect relative to self, but I
don't think that'd be quite correct in the presence of fragmentation
with transformed containers (if that's possible at all? would need to
think harder...), and it seems like the API should just behave more
generally, or assert otherwise.

To that effect, this patch adds an assertion to TransformRectToAncestor
that would've caught this bug.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9178642 [details]
Bug 1665447 - Fix BoxToRect when the "relative to" frame is not an ancestor. r=dholbert,mstange

Beta/Release Uplift Approval Request

• User impact if declined: NOTE: The release request should probably be conditional on confirming that bug 1668113 is fixed by this.

The beta request probably still stands, this fixes a regression from bug 1581876.

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: If possible, test bug 1668113, otherwise no test needed, if the automated test passes.
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple fix for an assumption in a layout API that the caller in bug 1581876 broke.
• String changes made/needed: none

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92f24022a62c
Fix BoxToRect when the "relative to" frame is not an ancestor. r=dholbert,mstange

Comment 7

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25861 for changes under testing/web-platform/tests

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/92f24022a62c

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 9178642 [details]
Bug 1665447 - Fix BoxToRect when the "relative to" frame is not an ancestor. r=dholbert,mstange

approved for 82.0b6

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9178642 [details]
Bug 1665447 - Fix BoxToRect when the "relative to" frame is not an ancestor. r=dholbert,mstange

The duplicate bug seems like a pretty high risk for retention given the extensive amount of virtual schooling happening currently and Blackboard's place in the market. While we'd normally prefer more bake time than this has had, it seems worth taking the risk given the relative simplicity of the patch and inclusion of automated tests. Approved for 81.0.1.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/a83dabc977bd

Comment 14

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6042cd95cbe8

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Added to the Firefox 81.0.1 relnotes:

Fixed missing content on Blackboard course listings

Comment 16

[Timea Cernea [:tbabos][inactive]]

Hey Emilio, we tried to sign-up to BlackBoard but didn't manage to get courses to check this out. Looking at https://bugzilla.mozilla.org/show_bug.cgi?id=1668113#c13 seems like the fix worked but we can't confirm that on our side. If the automated tests passed and we already have confirmation in Bug 1668113, I guess we are safe to call this verified. Can you please share your opinion about this?

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Yeah, that seems fine. https://github.com/webcompat/web-bugs/issues/58869 is another thing that should be tested by this.

Comment 18

[Timea Cernea [:tbabos][inactive]]

Thank you! Removing qe+ flag as per Comment 16 and Comment 17.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

The reporter of bug 1668113 has confirmed that 81.0.1 fixed the Blackboard bug. I don't see where we verified comment 17, though.

Comment 20

[Timea Cernea [:tbabos][inactive]]

For the https://www.mediasetplay.mediaset.it/ site from Comment 17 we have the following results with testing on Windows 10, MacOS 10.15 and Ubuntu 18.04
Latest Nightly 83.0a1: verified-fixed
Latest Beta 82.0b5: not fixed
Latest Release 81.0.1: verified-fixed

Hey Ryan, should we submit a new bug in Bugzilla for Beta and investigate further there?

Comment 21

[Julien Cristau [:jcristau]]

per comment 9, this will be in 82.0b6 not 82.0b5.

Comment 22

[Timea Cernea [:tbabos][inactive]]

Thanks Julien, marking this verified on Nightly and Release and will verify on Beta when it is landed.

Comment 23

[Timea Cernea [:tbabos][inactive]]

For the https://www.mediasetplay.mediaset.it/ site from Comment 17

Verified-fixed on latest Firefox Beta 82.0b6 on Windows 10, macOS 10.15 and Ubuntu 18.04.