Bug 65722 – Rename absoluteQuadsForRange and InlineTextBox::selectionRect to local*

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

RenderText::absoluteQuadsForRange() has:

            FloatRect rect = absoluteQuadForTextBox(box, start, end, useSelectionHeight);
            if (!rect.isZero())
                quads.append(localToAbsoluteQuad(rect));

This is converting local to absolute twice.

[Simon Fraser (smfr)]

Regression from http://trac.webkit.org/changeset/91763

[Emil A Eklund]

Actually I think it's just an unfortunate (and incorrect) name. absoluteQuadForTextBox doesn't convert to absolute, I'll make sure this is the case and rename it to localQuadForTextBox.

[Emil A Eklund]

Created attachment 103101 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 103101 [details]
Patch

Not sure this is right. quadForTextBox() calls selectionRect(), which uses selectionRectForRepaint(). The 'for repaint' indicates that it's already been converted to the coordinate system of the repaintContainer (or the root).

If there aren't repaint tests for this code, you should make some.

[Emil A Eklund]

The change 91763 didn't change change any logic apart from replacing a check for (!r.isEmpty()) with (r.height()) to match absoluteRectsForRange. Just as before it calls selectionRect and then adjust the rect and converts it to the absolute coordinate space using localToAbsoluteQuad.

As far as I can tell quadForTextBox calls InlineTextBox::selectionRect which in turn returns the selection rect in the local coordinate space.

There are a couple of tests that cover this code already and they helped me catch a a mistake in the first version of my patch.

If you still do not trust this to be correct I'd gladly revert change 91763.

[Simon Fraser (smfr)]

Ah, InlineTextBox::selectionRect() is in local coords, RenderObject::selectionRect() is in "repaint" coords. The former should be renamed ("localSelectionRect()"?)

[Emil A Eklund]

(In reply to comment #6)
> Ah, InlineTextBox::selectionRect() is in local coords, RenderObject::selectionRect() is in "repaint" coords. The former should be renamed ("localSelectionRect()"?)

Makes sense, I'll make the change.
Thanks!

[Emil A Eklund]

Created attachment 103128 [details]
Patch

[Emil A Eklund]

Thanks Simon!

[WebKit Review Bot]

Comment on attachment 103128 [details]
Patch

Clearing flags on attachment: 103128

Committed r92625: <http://trac.webkit.org/changeset/92625>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

RenderText::absoluteRectsForRange still has:

            // FIXME: This code is wrong. It's converting local to absolute twice. http://webkit.org/b/65722

[Tim Horton]

The confusing comment remains and I have no idea if it's true or not anymore (but it seems not).

[Tim Horton]

It's still there :(

[Tim Horton]

No longer: https://trac.webkit.org/changeset/219753/webkit