Bug 53627 - Web Inspector: highlight rect is wrong for contents of transformed iframes
Summary: Web Inspector: highlight rect is wrong for contents of transformed iframes
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL: http://2011.beercamp.com/
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-02-02 13:59 PST by Simon Fraser (smfr)
Modified: 2016-12-13 15:35 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 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.
Comment 1 Joseph Pecoraro 2011-06-08 20:17:02 PDT
Created attachment 96532 [details]
[TEST] Test with Different Transforms (rotate and perspective)
Comment 2 Joseph Pecoraro 2011-06-08 20:17:23 PDT
Created attachment 96533 [details]
[TEST] Test with Nested iframes
Comment 3 Joseph Pecoraro 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.
Comment 4 Joseph Pecoraro 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.
Comment 5 Joseph Pecoraro 2011-06-08 20:31:20 PDT
Created attachment 96537 [details]
[PATCH] First Attempt
Comment 6 Simon Fraser (smfr) 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?
Comment 7 Joseph Pecoraro 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.
Comment 8 Joseph Pecoraro 2011-06-10 10:19:11 PDT
Created attachment 96755 [details]
[PATCH] First Attempt (Better ChangeLog)

Attaching the correct patch.
Comment 9 Simon Fraser (smfr) 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.
Comment 10 Darin Adler 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.
Comment 11 Joseph Pecoraro 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?
Comment 12 Simon Fraser (smfr) 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.
Comment 13 Joseph Pecoraro 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.
Comment 14 David Kilzer (:ddkilzer) 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.
Comment 15 Joseph Pecoraro 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.
Comment 16 Joseph Pecoraro 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).
Comment 17 Joseph Pecoraro 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.
Comment 18 WebKit Review Bot 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.
Comment 19 Joseph Pecoraro 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.
Comment 20 Simon Fraser (smfr) 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.
Comment 21 Joseph Pecoraro 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!
Comment 23 Joseph Pecoraro 2011-06-25 16:33:18 PDT
Fix warning. Use UNUSED_PARAM on the correct param ;)
<http://trac.webkit.org/changeset/89756>
Comment 24 Joseph Pecoraro 2011-07-11 18:03:30 PDT
Reopening. These were rolled out in: <http://trac.webkit.org/changeset/90734>
Comment 25 Eric Seidel (no email) 2011-12-21 14:34:09 PST
Attachment 98577 [details] was posted by a committer and has review+, assigning to Joseph Pecoraro for commit.
Comment 26 Joseph Pecoraro 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.
Comment 27 Radar WebKit Bug Importer 2014-02-17 11:35:47 PST
<rdar://problem/16087048>