Bug 86390 – Expose FrameSelection::absoluteCaretBounds() via window.internals

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Shezan Baig]

A number of bugs (bug 85385, bug 85793, among others) involve caret positions for empty cells.  Right now, the only way to test fixes for these bugs is by pixel tests.  If we expose FrameSelection::localCaretRect to the Internals object, then we can use a simple dumpAsText test.

[Darin Adler]

I would expose the caret rectangle in a document-relative fashion, not “local”.

[Shezan Baig]

(In reply to comment #1)
> I would expose the caret rectangle in a document-relative fashion, not “local”.

My only concern with this is that it will make testing harder because the document-relative position of our test divs may be different depending on the platform (for example if we have some explanatory text at the top of the document), which means our test values are going to be platform-dependent.

[Ryosuke Niwa]

(In reply to comment #2)
> (In reply to comment #1)
> > I would expose the caret rectangle in a document-relative fashion, not “local”.
> 
> My only concern with this is that it will make testing harder because the document-relative position of our test divs may be different depending on the platform (for example if we have some explanatory text at the top of the document), which means our test values are going to be platform-dependent.

But you can compute the diff between element's offsetTop/offsetLeft, right?

[Shezan Baig]

(In reply to comment #3)
> But you can compute the diff between element's offsetTop/offsetLeft, right?

Yeah, we could do that.  I've updated the bug title for document-relative caret rect, thanks.

[Shezan Baig]

Created attachment 142076 [details]
Patch

Initial patch to pass to the build bots, I suspect there will be some missing exported symbols from other platforms that I'm not building on.

[Build Bot]

Comment on attachment 142076 [details]
Patch

Attachment 142076 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12701609

[Shezan Baig]

Created attachment 142246 [details]
Patch (with exported symbols for windows)

[Ryosuke Niwa]

Comment on attachment 142246 [details]
Patch (with exported symbols for windows)

View in context: https://bugs.webkit.org/attachment.cgi?id=142246&action=review

> Source/WebCore/testing/Internals.cpp:409
> +        || !document->frame()->selection()->caretRenderer()
> +        || !document->frame()->selection()->caretRenderer()->isBoxModelObject()) {
> +        ec = INVALID_ACCESS_ERR;
> +        return ClientRect::create();

It's kind of odd to throw when there are no renderers.

> LayoutTests/editing/selection/internal-caret-rect.html:41
> +    getSelection().collapse(textNode, 0);  // before a
> +    verifyCaretRect(8, 140, 1, 20);
> +    getSelection().collapse(textNode, 1);  // after a
> +    verifyCaretRect(28, 140, 1, 20);
> +    getSelection().collapse(textNode, 2);  // after b
> +    verifyCaretRect(48, 140, 1, 20);
> +    getSelection().collapse(textNode, 3);  // after c
> +    verifyCaretRect(68, 140, 1, 20);
> +    getSelection().collapse(textNode, 4);  // after d
> +    verifyCaretRect(88, 140, 1, 20);

Are you sure these values work on all platforms?

[Shezan Baig]

(In reply to comment #8)
> (From update of attachment 142246 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142246&action=review
> 
> > Source/WebCore/testing/Internals.cpp:409
> > +        || !document->frame()->selection()->caretRenderer()
> > +        || !document->frame()->selection()->caretRenderer()->isBoxModelObject()) {
> > +        ec = INVALID_ACCESS_ERR;
> > +        return ClientRect::create();
> 
> It's kind of odd to throw when there are no renderers.


Agreed -- will fix this in a new patch to just return an empty rect.


> 
> > LayoutTests/editing/selection/internal-caret-rect.html:41
> > +    getSelection().collapse(textNode, 0);  // before a
> > +    verifyCaretRect(8, 140, 1, 20);
> > +    getSelection().collapse(textNode, 1);  // after a
> > +    verifyCaretRect(28, 140, 1, 20);
> > +    getSelection().collapse(textNode, 2);  // after b
> > +    verifyCaretRect(48, 140, 1, 20);
> > +    getSelection().collapse(textNode, 3);  // after c
> > +    verifyCaretRect(68, 140, 1, 20);
> > +    getSelection().collapse(textNode, 4);  // after d
> > +    verifyCaretRect(88, 140, 1, 20);
> 
> Are you sure these values work on all platforms?


It works on chromium-win and gtk-linux.  I don't have a mac to test this on, but I'm hoping the Ahem font at 20px (applied to the body element) will guarantee these numbers on all platforms.  Unfortunately the mac ews doesn't run the layout tests, so I'm not sure how to verify this.

[Shezan Baig]

Created attachment 142297 [details]
Patch (with change from comment 8)

[Ryosuke Niwa]

Comment on attachment 142297 [details]
Patch (with change from comment 8)

View in context: https://bugs.webkit.org/attachment.cgi?id=142297&action=review

> Source/WebCore/testing/Internals.cpp:421
> +    while (renderer->offsetParent()) {
> +        caretRect.move(renderer->offsetLeft(), renderer->offsetTop());
> +        renderer = renderer->offsetParent();
> +    }

Can't we just use localToAbsolute or its friends here?

[Simon Fraser (smfr)]

Comment on attachment 142297 [details]
Patch (with change from comment 8)

View in context: https://bugs.webkit.org/attachment.cgi?id=142297&action=review

>> Source/WebCore/testing/Internals.cpp:421
>> +    }
> 
> Can't we just use localToAbsolute or its friends here?

Yes we totally should. We also have FrameSelection::absoluteCaretBounds() which does this already.

[Shezan Baig]

(In reply to comment #12)
> (From update of attachment 142297 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142297&action=review
> 
> >> Source/WebCore/testing/Internals.cpp:421
> >> +    }
> > 
> > Can't we just use localToAbsolute or its friends here?
> 
> Yes we totally should. We also have FrameSelection::absoluteCaretBounds() which does this already.

Sweet!  I'll use this.  Will also rename the method to window.internals.absoluteCaretBounds

[Shezan Baig]

Created attachment 142711 [details]
Patch (with change from comment 12)

[Build Bot]

Comment on attachment 142711 [details]
Patch (with change from comment 12)

Attachment 142711 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12722834

[Shezan Baig]

Created attachment 142726 [details]
Patch (with exported symbols for windows)

[Shezan Baig]

Comment on attachment 142726 [details]
Patch (with exported symbols for windows)

View in context: https://bugs.webkit.org/attachment.cgi?id=142726&action=review

> Source/WebCore/testing/Internals.cpp:57
> +#include "RenderBoxModelObject.h"

I just noticed this, sorry I didn't see it earlier when I was cleaning up the previous implementation.  I'll upload a new patch since this include isn't necessary.

[Shezan Baig]

Created attachment 142730 [details]
Patch (removed unneccessary include statement)

[WebKit Review Bot]

Comment on attachment 142730 [details]
Patch (removed unneccessary include statement)

Clearing flags on attachment: 142730

Committed r117610: <http://trac.webkit.org/changeset/117610>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.