Closed Bug 1363650 Opened 3 years ago Closed 2 years ago

(intersection-observer) Use content area as the intersection rectangle for custom root with overflow clip

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: tschneider, Assigned: tschneider)

References

Details

Attachments

(1 file, 7 obsolete files)

If a root element is specified, use its RootScrollRect instead of ContentRect.
Blocks: 1358666
Summary: (intersection-observer) Use ScrollPortRect if root element is given → (intersection-observer) Use content area for custom root with overflow clip
Summary: (intersection-observer) Use content area for custom root with overflow clip → (intersection-observer) Use content area as the intersection rectangle for custom root with overflow clip
Attached patch 1363650.patch (obsolete) — Splinter Review
Use the root elements content area as the intersection rect
Attachment #8866231 - Attachment is obsolete: true
Re-enables web platform test.
Attachment #8867399 - Attachment is obsolete: true
Attachment #8868630 - Flags: review?(dholbert)
No longer blocks: 1358666
Depends on: 1358666
Missed some tests.
Attachment #8868630 - Attachment is obsolete: true
Attachment #8868630 - Flags: review?(dholbert)
Attachment #8868643 - Flags: review?(dholbert)
Assignee: nobody → tschneider
(per IRC discussion last week, I think you're going to have mstange review this, correct?)
Flags: needinfo?(tschneider)
Correct! Focused on Bug 1335644 getting landed. I need to properly rebase this patch on top of it.
Flags: needinfo?(tschneider)
Attachment #8868643 - Flags: review?(dholbert)
Rebased patch
Attachment #8868643 - Attachment is obsolete: true
Attachment #8874955 - Flags: review?(mstange)
Please say a few words to explain what the patch is doing. Why is it adding the root rect's position to the scroll port rect? What coordinate spaces are these rectangles in?
I couldn't locate any existing helper function to calculate the scroll port rect at absolute position within the intersection root. My solution is basically a port of what we use in our web platform tests:

function contentBounds(root) {
  var left = root.offsetLeft + root.clientLeft;
  var right = left + root.clientWidth;
  var top = root.offsetTop + root.clientTop;
  var bottom = top + root.clientHeight;
  return [left, right, top, bottom];
}
Markus, any thoughts/suggestions on this?
Flags: needinfo?(mstange)
Comment on attachment 8874955 [details] [diff] [review]
Use root element's content area for intersection

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

How about this instead:

nsIFrame* containingBlock =
  nsLayoutUtils::GetContainingBlockForClientRect(rootFrame);
nsRect rootRectRelativeToRootFrame;
if (rootFrame->IsScrollFrame()) {
  // rootRectRelativeToRootFrame should be the content rect of rootFrame, not including the scrollbars.
  nsIScrollableFrame* scrollFrame = do_QueryFrame(rootFrame);
  rootRectRelativeToRootFrame = scrollFrame->GetScrollPortRect();
} else {
  // rootRectRelativeToRootFrame should be the content rect of rootFrame.
  rootRectRelativeToRootFrame = rootRect->GetContentRectRelativeToSelf();
}
rootRect =
  nsLayoutUtils::TransformFrameRectToAncestor(rootFrame,
                                              rootRectRelativeToRootFrame,
                                              containingBlock);

Does that also make tests pass?

nsLayoutUtils::GetAllInFlowRectsUnion is usually used when you need the bounding rect of an element that is split into multiple boxes. That can happen if you have an inline element that spans across a line break, or if you have an element that gets flowed into multiple columns of a multi-column layout.
Are there any tests that check what happens if "root" is such an element? If not, I think you should only handle the case where the element has just one frame.

Your current patch also doesn't account for transforms in the case where rootFrame is a scrollable frame.
Attachment #8874955 - Flags: review?(mstange)
Yeah, that works great too after fixing rootRect->GetContentRectRelativeToSelf() -> rootFrame->GetContentRectRelativeToSelf() ;).
Using mstange's code.
Attachment #8874955 - Attachment is obsolete: true
Flags: needinfo?(mstange)
Attachment #8877407 - Flags: review?(mstange)
Attachment #8877407 - Flags: review?(mstange) → review?(matt.woodrow)
Matt, do you have some time at hand giving this a quick review?
Flags: needinfo?(matt.woodrow)
Comment on attachment 8877407 [details] [diff] [review]
Use root element's content area for intersection

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

::: dom/base/DOMIntersectionObserver.cpp
@@ +272,5 @@
>      root = mRoot;
>      rootFrame = root->GetPrimaryFrame();
>      if (rootFrame) {
> +      nsIFrame* containingBlock =
> +        nsLayoutUtils::GetContainingBlockForClientRect(rootFrame);

Could move this down to be with TransformFrameRectToAncestor, since the result is only used there.
Attachment #8877407 - Flags: review?(matt.woodrow) → review+
Flags: needinfo?(matt.woodrow)
Thanks for the review Matt! Addresses your comment.
Attachment #8877407 - Attachment is obsolete: true
Pushed by tschneider@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb0a65afa6d
(intersection-observer) Use content area as the intersection rectangle for custom root with overflow clip. r=mattwoodrow
Blocks: 1374883
Three steps forward, two steps back. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dbadec2bba3b for making those three tests pass, but making intersection-observer/bounding-box.html and intersection-observer/unclipped-root fail, https://treeherder.mozilla.org/logviewer.html#?job_id=108702788&repo=mozilla-inbound
Unfortunate race condition. Bug 1359317 landed between https://bugzilla.mozilla.org/show_bug.cgi?id=1363650#c15 and https://bugzilla.mozilla.org/show_bug.cgi?id=1363650#c19. Will change the tests and re-land.
Make patch work after Bug 1359317 landed first.
Attachment #8879748 - Attachment is obsolete: true
Pushed by tschneider@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5424426a7189
(intersection-observer) Use content area as the intersection rectangle for custom root with overflow clip. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/5424426a7189
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8880052 [details] [diff] [review]
Use root element's content area for intersection. r=mattwoodrow

[Feature/Bug causing the regression]: 1321865
[User impact if declined]: Incompatibility with Chrome.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes, this fix has been in Nightly for ~3 weeks.
[List of other uplifts needed for the feature/fix]: 1359317
[Is the change risky?]: No
Attachment #8880052 - Flags: approval-mozilla-beta?
Comment on attachment 8880052 [details] [diff] [review]
Use root element's content area for intersection. r=mattwoodrow

Fix for Intersection Observer API which we plan to ship in 55. Let's try this for beta 9.
Attachment #8880052 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.