The inspector highlight rectangles are wrong for the contents of transformed iframes. They are probably doing a convertRect:toView:, rather than using the correct Widget methods.
Created attachment 96532 [details] [TEST] Test with Different Transforms (rotate and perspective)
Created attachment 96533 [details] [TEST] Test with Nested iframes
Created attachment 96535 [details] [PATCH] First Attempt I did not add a layout test. I don't think there are tests for node highlighting. I did a quick search in LayoutTests/inspector and saw tests for syntax highlighting related things. This worked as expected on my tests cases and the original test page <http://2011.beercamp.com/>. But this is by no means an area where I am an expert, and I'm guessing that there is already a way to do what it is I want to do but I could not find. So I'm going to put up a patch that people can actually see what I intend.
Wow, I used "accurate" way too much in my ChangeLog, I could probably remove that altogether. I expect to rewrite the ChangeLog anyways.
Created attachment 96537 [details] [PATCH] First Attempt
Comment on attachment 96537 [details] [PATCH] First Attempt Does this patch have anything to do with the bug?
Sorry, I attached the wrong patch the second time around and did not notice. That was one that already landed. You can view the first (obsoleted patch) but tomorrow I will attach the correct patch which has a more readable ChangeLog.
Created attachment 96755 [details] [PATCH] First Attempt (Better ChangeLog) Attaching the correct patch.
Comment on attachment 96755 [details] [PATCH] First Attempt (Better ChangeLog) View in context: https://bugs.webkit.org/attachment.cgi?id=96755&action=review > Source/WebCore/rendering/RenderObject.cpp:1868 > +void RenderObject::mapContainerToWindow(bool fixed, bool useTransforms, TransformState& transformState) const I'm not sure that I like having RenderObject having any knowledge of window-level things. > Source/WebCore/rendering/RenderObject.cpp:1876 > + // Account for the border of the frame renderer if we are mapping content inside the container. > + transformState.move(o->borderLeft(), o->borderTop()); > + static_cast<RenderObject*>(o)->mapLocalToContainer(0, fixed, useTransforms, transformState); I think this should be done with some combination of FrameView::convertFromRenderer(), convertToContainingView(), and convertToWindow(). I think you code here is ignoring padding, and might not work with scrolled content.
Comment on attachment 96755 [details] [PATCH] First Attempt (Better ChangeLog) View in context: https://bugs.webkit.org/attachment.cgi?id=96755&action=review >> Source/WebCore/rendering/RenderObject.cpp:1876 >> + static_cast<RenderObject*>(o)->mapLocalToContainer(0, fixed, useTransforms, transformState); > > I think this should be done with some combination of FrameView::convertFromRenderer(), convertToContainingView(), and convertToWindow(). I think you code here is ignoring padding, and might not work with scrolled content. I think you’re right.
(In reply to comment #9) > > Source/WebCore/rendering/RenderObject.cpp:1876 > > + // Account for the border of the frame renderer if we are mapping content inside the container. > > + transformState.move(o->borderLeft(), o->borderTop()); > > + static_cast<RenderObject*>(o)->mapLocalToContainer(0, fixed, useTransforms, transformState); > > I think this should be done with some combination of FrameView::convertFromRenderer(), > convertToContainingView(), and convertToWindow(). I think you code here is ignoring > padding, and might not work with scrolled content. The problem I was having with those was that many of those use IntRects. The desired result for the Inspector code was a FloatQuad. IntRects for instance cannot be used for rotated content. I think I can switch to the functions you mention above, if there a way that I can then apply all the transforms that would affect that resulting IntRect. Do you know offhand if there is an existing way to do that?
(In reply to comment #11) > (In reply to comment #9) > > > Source/WebCore/rendering/RenderObject.cpp:1876 > > > + // Account for the border of the frame renderer if we are mapping content inside the container. > > > + transformState.move(o->borderLeft(), o->borderTop()); > > > + static_cast<RenderObject*>(o)->mapLocalToContainer(0, fixed, useTransforms, transformState); > > > > I think this should be done with some combination of FrameView::convertFromRenderer(), > > convertToContainingView(), and convertToWindow(). I think you code here is ignoring > > padding, and might not work with scrolled content. > > The problem I was having with those was that many of those use IntRects. The desired result > for the Inspector code was a FloatQuad. IntRects for instance cannot be used for rotated content. > I think I can switch to the functions you mention above, if there a way that I can then apply all > the transforms that would affect that resulting IntRect. Do you know offhand if there is an > existing way to do that? I think the best way to fix this would be add FloatQuad variants of all the Widget rect mapping functions, or simply replace the IntRect ones. We could probably convert the IntPoint ones to FloatPoints.
> I think the best way to fix this would be add FloatQuad variants of all the > Widget rect mapping functions, or simply replace the IntRect ones. We > could probably convert the IntPoint ones to FloatPoints. Okay, thats what I was considering doing, but I wanted to make sure that I wasn't duplicating existing functionality that I just wasn't seeing. Thanks.
Comment on attachment 96755 [details] [PATCH] First Attempt (Better ChangeLog) Clearing review flag based on Comment #12 and Comment #13.
Trying again. This looks a little cleaner this time. 1. The URL the bug was reported against. http://2011.beercamp.com/ A lot of the content on this page is transformed and position:fixed. 2. The test cases attached. (Nested iframes). The 2nd nested iframe is slightly scrollable. So I can scroll that (causing a repaint bug) and test the highlight rect is correct for scrolled and transformed content in an iframe. > I think this should be done with some combination of FrameView::convertFromRenderer(), > convertToContainingView(), and convertToWindow(). I think you code here is ignoring > padding, and might not work with scrolled content. This is the approach I'm taking. I was ignoring padding, and scroll content was broken. These patches address that, but I encountered an issue with scroll content and fixed position content. This may be an existing bug in the IntRect version of convertToContainingView? I'm still getting familiar with the code, and I don't have a way to test this right now.
Created attachment 98576 [details] [PATCH] Part 1: Fix Transforms in Nested iframes test This worked for all the elements in the iframes test. Including correctly offset for the inner iframe's scroll position. (Despite a painting issue).
Created attachment 98577 [details] [PATCH] Part 2: Fix Fixed Position Content Without regressing the nested iframes case, this makes highlight rects work correctly on the beercamp page, where the majority of content is transformed iframes containing fixed position content.
Attachment 98576 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/ScrollView.h:31: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/Widget.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 98576 [details] [PATCH] Part 1: Fix Transforms in Nested iframes test View in context: https://bugs.webkit.org/attachment.cgi?id=98576&action=review >> Source/WebCore/platform/Widget.h:31 >> +#include "FloatQuad.h" > > Alphabetical sorting problem. [build/include_order] [4] Whoops, I'll fix that with any other review comments.
Comment on attachment 98577 [details] [PATCH] Part 2: Fix Fixed Position Content View in context: https://bugs.webkit.org/attachment.cgi?id=98577&action=review > Source/WebCore/rendering/RenderBlock.cpp:2747 > + mapLocalToContainer(repaintContainer, false, false, transformState, 0); You should give wasFixed a default value of 0 to avoid having to do this. > Source/WebCore/rendering/RenderBox.h:433 > + virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, bool fixed, bool useTransforms, TransformState&, bool* wasFixed) const; use wasFixed = 0 here. > Source/WebCore/rendering/RenderInline.h:133 > + virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, bool fixed, bool useTransforms, TransformState&, bool* wasFixed) const; And here. > Source/WebCore/rendering/RenderObject.h:774 > + virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, bool useTransforms, bool fixed, TransformState&, bool* wasFixed) const; And here. > Source/WebCore/rendering/RenderView.h:167 > + virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, bool useTransforms, bool fixed, TransformState&, bool* wasFixed) const; You get the drill.
(In reply to comment #20) > (From update of attachment 98577 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98577&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:2747 > > + mapLocalToContainer(repaintContainer, false, false, transformState, 0); > > You should give wasFixed a default value of 0 to avoid having to do this. Sounds good. I made the appropriate changes. Thanks for the review!
Landed in r89753 + r89754: <http://trac.webkit.org/changeset/89753> <http://trac.webkit.org/changeset/89754>
Fix warning. Use UNUSED_PARAM on the correct param ;) <http://trac.webkit.org/changeset/89756>
Reopening. These were rolled out in: <http://trac.webkit.org/changeset/90734>
Attachment 98577 [details] was posted by a committer and has review+, assigning to Joseph Pecoraro for commit.
Comment on attachment 98577 [details] [PATCH] Part 2: Fix Fixed Position Content Removed the r=simon from patches that landed long ago, were then rolled out, and then the bug was reopened. This bug require re-investigation.
<rdar://problem/16087048>