Bug 53627 – Web Inspector: highlight rect is wrong for contents of transformed iframes

• Status: REOPENED
• Component: Web Inspector

[Simon Fraser (smfr)]

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.

[Joseph Pecoraro]

Created attachment 96532 [details]
[TEST] Test with Different Transforms (rotate and perspective)

[Joseph Pecoraro]

Created attachment 96533 [details]
[TEST] Test with Nested iframes

[Joseph Pecoraro]

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]

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]

Created attachment 96537 [details]
[PATCH] First Attempt

[Simon Fraser (smfr)]

Comment on attachment 96537 [details]
[PATCH] First Attempt

Does this patch have anything to do with the bug?

[Joseph Pecoraro]

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]

Created attachment 96755 [details]
[PATCH] First Attempt (Better ChangeLog)

Attaching the correct patch.

[Simon Fraser (smfr)]

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 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]

(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)]

(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]

> 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 on attachment 96755 [details]
[PATCH] First Attempt (Better ChangeLog)

Clearing review flag based on Comment #12 and Comment #13.

[Joseph Pecoraro]

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]

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]

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]

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 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 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]

(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]

Landed in r89753 + r89754:
<http://trac.webkit.org/changeset/89753>
<http://trac.webkit.org/changeset/89754>

[Joseph Pecoraro]

Fix warning. Use UNUSED_PARAM on the correct param ;)
<http://trac.webkit.org/changeset/89756>

[Joseph Pecoraro]

Reopening. These were rolled out in: <http://trac.webkit.org/changeset/90734>

[Eric Seidel (no email)]

Attachment 98577 [details] was posted by a committer and has review+, assigning to Joseph Pecoraro for commit.

[Joseph Pecoraro]

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]

<rdar://problem/16087048>