WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
REOPENED
53627
Web Inspector: highlight rect is wrong for contents of transformed iframes
https://bugs.webkit.org/show_bug.cgi?id=53627
Summary
Web Inspector: highlight rect is wrong for contents of transformed iframes
Simon Fraser (smfr)
Reported
2011-02-02 13:59:21 PST
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.
Attachments
[TEST] Test with Different Transforms (rotate and perspective)
(1.11 KB, text/html)
2011-06-08 20:17 PDT
,
Joseph Pecoraro
no flags
Details
[TEST] Test with Nested iframes
(658 bytes, text/html)
2011-06-08 20:17 PDT
,
Joseph Pecoraro
no flags
Details
[PATCH] First Attempt
(8.38 KB, patch)
2011-06-08 20:20 PDT
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
[PATCH] First Attempt
(2.42 KB, patch)
2011-06-08 20:31 PDT
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
[PATCH] First Attempt (Better ChangeLog)
(8.20 KB, patch)
2011-06-10 10:19 PDT
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
[PATCH] Part 1: Fix Transforms in Nested iframes test
(10.17 KB, patch)
2011-06-24 21:57 PDT
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
[PATCH] Part 2: Fix Fixed Position Content
(27.59 KB, patch)
2011-06-24 21:59 PDT
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1
2011-06-08 20:17:02 PDT
Created
attachment 96532
[details]
[TEST] Test with Different Transforms (rotate and perspective)
Joseph Pecoraro
Comment 2
2011-06-08 20:17:23 PDT
Created
attachment 96533
[details]
[TEST] Test with Nested iframes
Joseph Pecoraro
Comment 3
2011-06-08 20:20:54 PDT
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.
Joseph Pecoraro
Comment 4
2011-06-08 20:23:16 PDT
Wow, I used "accurate" way too much in my ChangeLog, I could probably remove that altogether. I expect to rewrite the ChangeLog anyways.
Joseph Pecoraro
Comment 5
2011-06-08 20:31:20 PDT
Created
attachment 96537
[details]
[PATCH] First Attempt
Simon Fraser (smfr)
Comment 6
2011-06-08 23:10:37 PDT
Comment on
attachment 96537
[details]
[PATCH] First Attempt Does this patch have anything to do with the bug?
Joseph Pecoraro
Comment 7
2011-06-09 23:05:41 PDT
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.
Joseph Pecoraro
Comment 8
2011-06-10 10:19:11 PDT
Created
attachment 96755
[details]
[PATCH] First Attempt (Better ChangeLog) Attaching the correct patch.
Simon Fraser (smfr)
Comment 9
2011-06-10 10:39:33 PDT
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.
Darin Adler
Comment 10
2011-06-10 11:16:49 PDT
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.
Joseph Pecoraro
Comment 11
2011-06-10 11:30:45 PDT
(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?
Simon Fraser (smfr)
Comment 12
2011-06-10 11:32:22 PDT
(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.
Joseph Pecoraro
Comment 13
2011-06-10 11:38:18 PDT
> 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.
David Kilzer (:ddkilzer)
Comment 14
2011-06-15 10:50:34 PDT
Comment on
attachment 96755
[details]
[PATCH] First Attempt (Better ChangeLog) Clearing review flag based on
Comment #12
and
Comment #13
.
Joseph Pecoraro
Comment 15
2011-06-24 21:56:12 PDT
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.
Joseph Pecoraro
Comment 16
2011-06-24 21:57:50 PDT
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).
Joseph Pecoraro
Comment 17
2011-06-24 21:59:46 PDT
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.
WebKit Review Bot
Comment 18
2011-06-24 22:00:19 PDT
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.
Joseph Pecoraro
Comment 19
2011-06-24 22:01:32 PDT
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.
Simon Fraser (smfr)
Comment 20
2011-06-25 09:20:39 PDT
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.
Joseph Pecoraro
Comment 21
2011-06-25 15:23:29 PDT
(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!
Joseph Pecoraro
Comment 22
2011-06-25 16:02:31 PDT
Landed in
r89753
+
r89754
: <
http://trac.webkit.org/changeset/89753
> <
http://trac.webkit.org/changeset/89754
>
Joseph Pecoraro
Comment 23
2011-06-25 16:33:18 PDT
Fix warning. Use UNUSED_PARAM on the correct param ;) <
http://trac.webkit.org/changeset/89756
>
Joseph Pecoraro
Comment 24
2011-07-11 18:03:30 PDT
Reopening. These were rolled out in: <
http://trac.webkit.org/changeset/90734
>
Eric Seidel (no email)
Comment 25
2011-12-21 14:34:09 PST
Attachment 98577
[details]
was posted by a committer and has review+, assigning to Joseph Pecoraro for commit.
Joseph Pecoraro
Comment 26
2011-12-21 15:01:03 PST
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.
Radar WebKit Bug Importer
Comment 27
2014-02-17 11:35:47 PST
<
rdar://problem/16087048
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug