Bug 20766 – Unable to resize deeply nested frames

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Bo Link]

We're working on the cross-browser compatibility for a web-based application, and in doing so, we've seemed to run across a small problem with nested framesets in Safari/WebKit based browsers (happens in Chrome as well).

The application uses nested framesets, and we can't seem to resize the frames that are >2 levels deep. The example link I've attached shows a frames based page that demonstrates our problems. The left frame and the top two right frames are resizable, but the rest of the frames on the right hand side of the page won't resize.

I tried to search for a similarly reported bug, but I wasn't able to find anything that directly correlated to the problem we're having. Please move this to the appropriate location if I did duplicate an earlier report.

Thanks in advance for any help you can provide.

-Bo Link

[Alexey Proskuryakov]

Confirmed with r35843.

[Adam Treat]

Created attachment 26597 [details]
Fixes coord transformation for deeply nested frames

There was a missing coord transformation for deeply nested frames.  This patch fixes the problem.

[Darin Adler]

Comment on attachment 26597 [details]
Fixes coord transformation for deeply nested frames

Looks great!

> +IntPoint RenderFrameSet::localPos(const IntPoint& point) const

I know that both xPos and yPos abbreviate the word "position" as "pos", but I'd prefer to not add more uses of such abbreviations.

I also think that this converts a point from local, object-relative coordinates to document coordinates. I don't think localPos is a good name for that. Maybe Hyatt has a suggestion for a better name. We have functions named localToAbsolute in RenderObject; maybe localToDocument? Or maybe you can use localToAbsolute instead of writing a new function?

> +    IntPoint pos = IntPoint(point.x() + xPos(), point.y() + yPos());
> +    RenderObject* o = parent();
> +    if (!o || !o->isFrameSet())
> +        return pos;
> +
> +    return static_cast<RenderFrameSet*>(o)->localPos(pos);

I'd prefer a looping algorithm to a recursive one. With recursive algorithms, large data structures can become crashing bugs that might even have security implications. When something is, like this, so easy to write as a loop, I'd prefer to do so.

Bug fixes need regression tests. Please make a test demonstrating the bug and the fact that it's fixed.

[Adam Treat]

(In reply to comment #3)
> (From update of attachment 26597 [details] [review])
> Looks great!
> 
> > +IntPoint RenderFrameSet::localPos(const IntPoint& point) const
> 
> I know that both xPos and yPos abbreviate the word "position" as "pos", but I'd
> prefer to not add more uses of such abbreviations.

Sure!

> I also think that this converts a point from local, object-relative coordinates
> to document coordinates. I don't think localPos is a good name for that. Maybe
> Hyatt has a suggestion for a better name. We have functions named
> localToAbsolute in RenderObject; maybe localToDocument? Or maybe you can use
> localToAbsolute instead of writing a new function?

I looked extensively for such a function and did find RenderObject::localToAbsolute, but was put off by the fact that it took a FloatPoint as this seemed like it was intended for some other usage.  Also, I wonder if localToAbsolute is correct since we're only interested in the offset of parent RenderFrameSet's?  I'll test localToAbsolute though just to make sure.

> > +    IntPoint pos = IntPoint(point.x() + xPos(), point.y() + yPos());
> > +    RenderObject* o = parent();
> > +    if (!o || !o->isFrameSet())
> > +        return pos;
> > +
> > +    return static_cast<RenderFrameSet*>(o)->localPos(pos);
> 
> I'd prefer a looping algorithm to a recursive one. With recursive algorithms,
> large data structures can become crashing bugs that might even have security
> implications. When something is, like this, so easy to write as a loop, I'd
> prefer to do so.

Ok, no problem.

> Bug fixes need regression tests. Please make a test demonstrating the bug and
> the fact that it's fixed.

I'm not sure how I'd make such a regression test as this requires user interaction.  Specifically, it would require a series of programmatic mouse events.  Do the layout tests currently have this ability, because if so, I haven't seen it.

[Adam Treat]

Darin, one more point... RenderObject::localToAbsolute has the same type of recursive algorithm.  Should this be changed?

[Darin Adler]

(In reply to comment #5)
> Darin, one more point... RenderObject::localToAbsolute has the same type of
> recursive algorithm.  Should this be changed?

Yes. I'm not sure it's urgent because we have a lot of them. I don't want to unnecessarily add new ones.

[Darin Adler]

(In reply to comment #4)
> I'm not sure how I'd make such a regression test as this requires user
> interaction.  Specifically, it would require a series of programmatic mouse
> events.  Do the layout tests currently have this ability, because if so, I
> haven't seen it.

Yes, we have many layout tests that test various types of dragging such as dragging-to-disabled-file-input.html.

You say "a series of events", but I think that for this case you might just need a mouse down in one place and then a mouse up in another. Given that this code is done based on DOM events I think it may even be possible without resorting to the "eventSender" object at all, but it's definitely possible with eventSender.

[Darin Adler]

(In reply to comment #4)
> I looked extensively for such a function and did find
> RenderObject::localToAbsolute, but was put off by the fact that it took a
> FloatPoint as this seemed like it was intended for some other usage.  Also, I
> wonder if localToAbsolute is correct since we're only interested in the offset
> of parent RenderFrameSet's?  I'll test localToAbsolute though just to make
> sure.

People to ask would be Dave Hyatt, Simon Fraser, and Dan Bernstein.

I don't know the answers here, and I didn't really think deeply about this. But it doesn't really seem to me like what you want here is something specific to frames. Saying you're only interested in the offsets of parent RenderFrameSet is slightly surprising to me.

[Adam Treat]

Thinking about it a bit more, I believe you are right.  Looking at correct offset no matter the parent type.  With frameset's nested in other elements the other elements would become just as important.

I'll look into using localToAbsolute and also a proper regression test for this.

Thanks!

[Adam Treat]

Hah, that was easy.  RenderObject::localToAbsolute is the correct function here.  Moreover, the reason it is using FloatPoint is that it has a 'useTransforms' default arg of false.  For use cases with the transforms, of course we'd want a FloatPoint over an IntPoint.

Making a regression test now.

[Adam Treat]

Created attachment 26607 [details]
New version which uses localToAbsolute and adds layout test

I've added a layout test as well as switched to using RenderObject::localToAbsolute

Cheers,
Adam

[Darin Adler]

Comment on attachment 26607 [details]
New version which uses localToAbsolute and adds layout test

> +            function init() {
> +                if (window.layoutTestController) {
> +                    //Move the last horizontal resizer ten pixels south...
> +                    eventSender.mouseMoveTo(400, 393);
> +                    eventSender.mouseDown();
> +                    eventSender.mouseMoveTo(400, 403);
> +                    eventSender.mouseUp();
> +                }
> +            }

I'd like this test better if it used dumpAsText instead of a render tree dump for its results.

Fix looks great!

I'm going to say r=me on this as is, but please work on an even-better test if you're willing to.

[Adam Treat]

Created attachment 26617 [details]
Updated to include a much better layout test

I've gone ahead and created a much better layout test that uses dumpAsText.  It also tests every frame resizer in the test as well as provides a way to use it outside of DumpRenderTree.

Cheers,
Adam

[Darin Adler]

Comment on attachment 26617 [details]
Updated to include a much better layout test

r=me

[Adam Treat]

Fixed with r39812.