RenderObject::absoluteBoundingBoxRect() should ignore rects outside the bounds of the document

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

* SUMMARY
The RenderObject::absoluteBoundingBoxRect() method should ignore rects outside the bounds of the document (to the left or to the top) when creating its united rect.  This will clean up some Web Inspector highlighting such as the "Sign In" button/link on bankofamerica.com as well as the test case attached.

* STEPS TO REPRODUCE
1. Open Safari/WebKit.
2. Open URL or test case for this bug.
3. Right click on "Sign In" button/link (on either page), and select "Inspect Element".
4. Note location of red highlight box.

* EXPECTED RESULTS
The red highlight box should be drawn around the "Sign In" text.

* ACTUAL RESULTS
The red highlight box is shifted to the left of the button/link.

* REGRESSION
Unknown--no way to easily test shipping Safari without the Web Inspector.

[David Kilzer (:ddkilzer)]

Created attachment 14166 [details]
Test case

[David Kilzer (:ddkilzer)]

Created attachment 14167 [details]
Patch v1 (feedback only!)

Looking for feedback on this patch.

- Am I getting the correct document bounds using windowClipRect(false)?
- Why do I have to "flip" the Y coordinate to make this work?

[Dave Hyatt]

windowClipRect is in window coordinates, not in your doc's coords.  YOu don't want to use it.

[David Kilzer (:ddkilzer)]

Comment on attachment 14167 [details]
Patch v1 (feedback only!)

I think I want to use the ScrollView's bounds instead.

[David Kilzer (:ddkilzer)]

Created attachment 14187 [details]
Patch v2

Use frameGeometry() instead of windowClipRect(false) to get rect for the whole page.

Still no test.  I think I'll need to extend the ObjC plug-in so it may call [DOMNode boundingBox] to test the resulting rect.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> Created an attachment (id=14187) [edit]
> Patch v2
> 
> Use frameGeometry() instead of windowClipRect(false) to get rect for the whole
> page.

Note that all layout tests passed in r21100 with this change.

[Dave Hyatt]

Comment on attachment 14187 [details]
Patch v2

Frame geometry is the rectangle of the widget in its parent's coordinate space.  This widget is just the rectangle of the scroll view itself.  This is still not what you want.

You just want contentsWidth/Height on scrollview.  The document's bounds are always (0, 0, contentsWidth(), contentsHeight()).

[David Kilzer (:ddkilzer)]

Created attachment 14219 [details]
Patch v3

Use document()->view()->contentsWidth() and document()->view()->contentsHeight().

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> Created an attachment (id=14219) [edit]
> Patch v3
> 
> Use document()->view()->contentsWidth() and
> document()->view()->contentsHeight().

Again, I should mention that all layout tests pass!

[Dave Hyatt]

Comment on attachment 14219 [details]
Patch v3

So you have really opened up a can of worms with this patch.

I don't think this is the right spot to deal with this problem looking into this more.  There are several other callers of absoluteRects, and they are all wrong too.

absoluteRects shouldn't even be handing back these unreachable rects in the first place, so patching in absoluteBoundingBoxRect is dealing with the problem too late.

WebCoreAXObject.mm calls absoluteRects for example (and so suffers from the same bug), and HTMLAnchorElement.cpp does as well.

I think you basically need to patch absoluteRects to intersect the rects with the document bounds.  There is no logical reason to ever be handing these completely unreachable rects back.

[David Kilzer (:ddkilzer)]

Oops!  Didn't mean to close this.

[David Kilzer (:ddkilzer)]

Created attachment 14310 [details]
Patch v4

Addressing issues in Comment #10:
- Updated all absoluteRects() methods to clip rects added to document bounds.
- Added Document::bounds() method to return a rect with coordinates (0,0) and the height/width set to the contents.
- Added RenderObject::addRectClippedToBounds() method to avoid code duplication.

Questions:
- Is Document the correct place to put the bounds() method?  I thought about putting it on the FrameView instead, but wanted to avoid calling document()->view()->bounds() instead of just document()->bounds().
- Is bounds() a good name for Document::bounds()?  Others:  boundsRect(), contentsRect(), contents()?
- Is addRectClippedToBounds() a good name?  Should it be more generic?  Is there other code that could be using this method?
- Does the way WebCoreAXObject.mm uses absoluteRects() cause this patch to prevent users with special needs from accessing elements on the page, or does it only deal with the visible (rendered) part of the page and we're only clipping non-visible content anyway?

[Dave Hyatt]

No, Document is not an appropriate place for a rect method like that.  I would just fix ScrollView to have a method that gives you its contents rect as (0, 0, contentsWidth, contentsHeight).

[David Kilzer (:ddkilzer)]

Comment on attachment 14310 [details]
Patch v4

Need to move Document::bounds() to ScrollView.

[David Kilzer (:ddkilzer)]

BTW, running layout tests for r21214 with Patch v4 produces only one change in pixel tests (the gradient differs because it doesn't start offscreen anymore):

svg/custom/js-late-gradient-and-object-creation.svg

All of the the non-pixel tests pass with Patch v4 applied.

[Simon Fraser (smfr)]

Testcase looks fine with current builds.

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> Testcase looks fine with current builds.

Closing as fixed (configuration change).