RESOLVED FIXED 13478
RenderObject::absoluteBoundingBoxRect() should ignore rects outside the bounds of the document
https://bugs.webkit.org/show_bug.cgi?id=13478
Summary RenderObject::absoluteBoundingBoxRect() should ignore rects outside the bound...
David Kilzer (:ddkilzer)
Reported 2007-04-24 22:01:54 PDT
* 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.
Attachments
Test case (91 bytes, text/html)
2007-04-24 22:02 PDT, David Kilzer (:ddkilzer)
no flags
Patch v1 (feedback only!) (1.29 KB, patch)
2007-04-24 22:04 PDT, David Kilzer (:ddkilzer)
ddkilzer: review-
Patch v2 (1.28 KB, patch)
2007-04-25 13:48 PDT, David Kilzer (:ddkilzer)
hyatt: review-
Patch v3 (1.32 KB, patch)
2007-04-26 22:44 PDT, David Kilzer (:ddkilzer)
hyatt: review-
Patch v4 (14.53 KB, patch)
2007-05-02 10:06 PDT, David Kilzer (:ddkilzer)
ddkilzer: review-
David Kilzer (:ddkilzer)
Comment 1 2007-04-24 22:02:26 PDT
Created attachment 14166 [details] Test case
David Kilzer (:ddkilzer)
Comment 2 2007-04-24 22:04:01 PDT
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
Comment 3 2007-04-24 22:31:42 PDT
windowClipRect is in window coordinates, not in your doc's coords. YOu don't want to use it.
David Kilzer (:ddkilzer)
Comment 4 2007-04-24 23:39:13 PDT
Comment on attachment 14167 [details] Patch v1 (feedback only!) I think I want to use the ScrollView's bounds instead.
David Kilzer (:ddkilzer)
Comment 5 2007-04-25 13:48:06 PDT
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)
Comment 6 2007-04-25 17:11:03 PDT
(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 7 2007-04-25 17:38:19 PDT
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)
Comment 8 2007-04-26 22:44:33 PDT
Created attachment 14219 [details] Patch v3 Use document()->view()->contentsWidth() and document()->view()->contentsHeight().
David Kilzer (:ddkilzer)
Comment 9 2007-04-26 23:02:37 PDT
(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 10 2007-04-26 23:26:16 PDT
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)
Comment 11 2007-04-27 00:06:52 PDT
Oops! Didn't mean to close this.
David Kilzer (:ddkilzer)
Comment 12 2007-05-02 10:06:37 PDT
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
Comment 13 2007-05-02 11:46:16 PDT
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 14 2007-05-02 16:58:20 PDT
Comment on attachment 14310 [details] Patch v4 Need to move Document::bounds() to ScrollView.
David Kilzer (:ddkilzer)
Comment 15 2007-05-02 19:39:44 PDT
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)
Comment 16 2009-02-06 20:08:58 PST
Testcase looks fine with current builds.
David Kilzer (:ddkilzer)
Comment 17 2009-02-06 22:57:13 PST
(In reply to comment #16) > Testcase looks fine with current builds. Closing as fixed (configuration change).
Note You need to log in before you can comment on or make changes to this bug.