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.
Unknown--no way to easily test shipping Safari without the Web Inspector.
Created attachment 14166 [details]
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?
windowClipRect is in window coordinates, not in your doc's coords. YOu don't want to use it.
Comment on attachment 14167 [details]
Patch v1 (feedback only!)
I think I want to use the ScrollView's bounds instead.
Created attachment 14187 [details]
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.
(In reply to comment #5)
> Created an attachment (id=14187) 
> Patch v2
> Use frameGeometry() instead of windowClipRect(false) to get rect for the whole
Note that all layout tests passed in r21100 with this change.
Comment on attachment 14187 [details]
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()).
Created attachment 14219 [details]
Use document()->view()->contentsWidth() and document()->view()->contentsHeight().
(In reply to comment #8)
> Created an attachment (id=14219) 
> Patch v3
> Use document()->view()->contentsWidth() and
Again, I should mention that all layout tests pass!
Comment on attachment 14219 [details]
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.
Oops! Didn't mean to close this.
Created attachment 14310 [details]
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.
- 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?
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).
Comment on attachment 14310 [details]
Need to move Document::bounds() to ScrollView.
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):
All of the the non-pixel tests pass with Patch v4 applied.
Testcase looks fine with current builds.
(In reply to comment #16)
> Testcase looks fine with current builds.
Closing as fixed (configuration change).