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.
I would expose the caret rectangle in a document-relative fashion, not “local”.
(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.
(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?
(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.
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.
Comment on attachment 142076 [details] Patch Attachment 142076 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12701609
Created attachment 142246 [details] Patch (with exported symbols for windows)
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?
(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.
Created attachment 142297 [details] Patch (with change from comment 8)
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?
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.
(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
Created attachment 142711 [details] Patch (with change from comment 12)
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
Created attachment 142726 [details] Patch (with exported symbols for windows)
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.
Created attachment 142730 [details] Patch (removed unneccessary include statement)
Comment on attachment 142730 [details] Patch (removed unneccessary include statement) Clearing flags on attachment: 142730 Committed r117610: <http://trac.webkit.org/changeset/117610>
All reviewed patches have been landed. Closing bug.