Bug 85385 – Caret rendered at incorrect location inside empty table cell

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

[Shezan Baig]

See the test URL.  In WebKit-based browsers, the caret is rendered at the wrong y-coordinate when inside one of the empty cells.

The cause of the bug is that RenderBlock::localCaretRect uses 'paddingTop()' when computing the y-coordinate, which includes the intrinsic padding of the table-cell.  The fix for this is to use 'computedCSSPaddingTop()' instead.

I will upload a patch shortly, however I'm not sure how one would test this.

[Alexey Proskuryakov]

See also: bug 20129.

[Shezan Baig]

After further research, I found that this is not an issue with RenderBlock::localCaretRect -- the intrinsic padding needs to be included when computing the y-coordinate.

However, I found that the intrinsic padding itself is calculated wrongly.  Try the following test cases and put the cursor inside one of the empty cells:

http://jsfiddle.net/wceLA/1/  (cursor appears at bottom of cell)
http://jsfiddle.net/wceLA/2/  (cursor appears between the middle and the bottom)

In both cases, the cursor should appear in the middle of the cell.  In fact, once you type something into the cell, it jumps to the middle.

I believe the logic that computes the intrinsic padding in RenderTableSection::layoutRows() needs to be modified to account for empty cells as a special case.  I want to look into it a little deeper to verify everything, then I'll upload a patch with a bunch of test cases.

This change will most likely break the DRT dumps for any tests that have empty cells.

[Ryosuke Niwa]

I think caret rendering belongs to the editing component still.

[Shezan Baig]

All the tests so far are fixed with a simple change to RenderBlock::hasLineIfEmpty() to make it return true if this is an editable table-cell.  This also fixes:

http://jsfiddle.net/wceLA/3/

In existing webkit browsers, the table in the line above is rendered as a flat line, but when RenderBlock::hasLineIfEmpty() returns true for editable table-cells, we can see the table cells and click inside it.

I wonder if we should just return true for all editable renderers that have visible borders?

[Ryosuke Niwa]

(In reply to comment #4)
> All the tests so far are fixed with a simple change to RenderBlock::hasLineIfEmpty() to make it return true if this is an editable table-cell.

Yes, that makes sense because table cells act as a non-splittable element. By the way, the statement:
node()->rootEditableElement() == node()
is very inefficient because rootEditableElement() will walk up the tree to find the root editable element. A better check will be:

node()->rendererIsEditable() && (!node()->parentNode() || !node()->parentNode()->rendererIsEditable() || node()->hasTagName(bodyTag))

We should probably add a helper function for this.

[Shezan Baig]

Created attachment 143078 [details]
Patch

[WebKit Review Bot]

Comment on attachment 143078 [details]
Patch

Attachment 143078 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12734729

New failing tests:
editing/deleting/5483370.html
editing/deleting/5433862-1.html
editing/selection/select-all-004.html
editing/deleting/5206311-2.html
editing/selection/move-by-line-001.html
editing/deleting/5206311-1.html
editing/deleting/5433862-2.html
editing/deleting/5126166.html

[WebKit Review Bot]

Created attachment 143087 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Shezan Baig]

Created attachment 143657 [details]
Patch (with gtk rebaseline)

Rebaseline is needed because the coordinates for some tests have changed as a result of empty lines allocated for the table cell. I have included rebaseline for gtk, I need help to rebaseline for other platforms.

[WebKit Review Bot]

Comment on attachment 143657 [details]
Patch (with gtk rebaseline)

Attachment 143657 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12779296

New failing tests:
editing/deleting/5483370.html
editing/selection/select-all-004.html
editing/selection/move-by-line-001.html
editing/deleting/5206311-1.html
editing/deleting/5433862-2.html
editing/deleting/5126166.html

[WebKit Review Bot]

Created attachment 143668 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Shezan Baig]

Created attachment 146049 [details]
Patch (with gtk+chromium rebaseline)

[Ryosuke Niwa]

Comment on attachment 146049 [details]
Patch (with gtk+chromium rebaseline)

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

> Source/WebCore/rendering/RenderTableCell.cpp:191
> +    if (node() && node()->rendererIsEditable())
> +        return true;
> +    return RenderBlock::hasLineIfEmpty();

I would have done: (node() && node()->rendererIsEditable()) || RenderBlock::hasLineIfEmpty();

> LayoutTests/editing/selection/caret-in-empty-table-cell.html:62
> +    verifyCaretRectForCell("emptycell1", 12, 245, 1, 20);
> +    verifyCaretRectForCell("emptycell2", 12, 275, 1, 20);
> +    verifyCaretRectForCell("emptycell3", 12, 315, 1, 20);
> +    verifyCaretRectForCell("emptycell4", 12, 375, 1, 20);

Hopefully we wouldn't have off-by-one difference on some platforms.

[Shezan Baig]

(In reply to comment #13)
> (From update of attachment 146049 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146049&action=review
> 
> > Source/WebCore/rendering/RenderTableCell.cpp:191
> > +    if (node() && node()->rendererIsEditable())
> > +        return true;
> > +    return RenderBlock::hasLineIfEmpty();
> 
> I would have done: (node() && node()->rendererIsEditable()) || RenderBlock::hasLineIfEmpty();

Nice -- I will make this change and upload a new patch - thanks!

> 
> > LayoutTests/editing/selection/caret-in-empty-table-cell.html:62
> > +    verifyCaretRectForCell("emptycell1", 12, 245, 1, 20);
> > +    verifyCaretRectForCell("emptycell2", 12, 275, 1, 20);
> > +    verifyCaretRectForCell("emptycell3", 12, 315, 1, 20);
> > +    verifyCaretRectForCell("emptycell4", 12, 375, 1, 20);
> 
> Hopefully we wouldn't have off-by-one difference on some platforms.

I hope so too.  I believe it should be fine, this is very similar to the test for bug 86390, which seems to be fine on all platforms.

[Shezan Baig]

Created attachment 146099 [details]
Patch (with change from comment 13)

[WebKit Review Bot]

Comment on attachment 146099 [details]
Patch (with change from comment 13)

Rejecting attachment 146099 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ching file LayoutTests/editing/selection/caret-in-empty-table-cell-expected.txt
patching file LayoutTests/editing/selection/caret-in-empty-table-cell.html
Original content of LayoutTests/platform/chromium-linux/editing/selection/select-all-004-expected.png mismatches at /mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply line 279.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Ryosuke Ni..." exit_code: 9 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12914140

[Shezan Baig]

Created attachment 146302 [details]
Patch (with rebaseline fix)

select-all-004-expected.png was overwritten simultaneously in r119630

[WebKit Review Bot]

Comment on attachment 146302 [details]
Patch (with rebaseline fix)

Attachment 146302 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12922347

New failing tests:
editing/selection/select-all-004.html

[WebKit Review Bot]

Created attachment 146453 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Shezan Baig]

Created attachment 146584 [details]
Patch (with rebaseline fix)

remove chromium's image rebaseline for select-all-004.html (it fails locally for me even without this patch)

[Ryosuke Niwa]

Comment on attachment 146584 [details]
Patch (with rebaseline fix)

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

> Source/WebCore/rendering/RenderTableCell.cpp:190
> +bool RenderTableCell::hasLineIfEmpty() const
> +{
> +    return (node() && node()->rendererIsEditable()) || RenderBlock::hasLineIfEmpty();
> +}

After doing some search, I've started to think that this isn't the right fix.

> LayoutTests/editing/deleting/5206311-2-expected.txt:-30
> -|         <br>

We need these br for empty td's to have the right line height.

[Shezan Baig]

(In reply to comment #21)
> (From update of attachment 146584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146584&action=review
> 
> > Source/WebCore/rendering/RenderTableCell.cpp:190
> > +bool RenderTableCell::hasLineIfEmpty() const
> > +{
> > +    return (node() && node()->rendererIsEditable()) || RenderBlock::hasLineIfEmpty();
> > +}
> 
> After doing some search, I've started to think that this isn't the right fix.
> 
> > LayoutTests/editing/deleting/5206311-2-expected.txt:-30
> > -|         <br>
> 
> We need these br for empty td's to have the right line height.


Hmm, but wouldn't returning true for 'hasLineIfEmpty' cause the empty td's to have the right line height?

[Ryosuke Niwa]

(In reply to comment #22)
> Hmm, but wouldn't returning true for 'hasLineIfEmpty' cause the empty td's to have the right line height?

In WebKit, yes, but no on other browsers.

[Ryosuke Niwa]

Comment on attachment 146584 [details]
Patch (with rebaseline fix)

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

>>> Source/WebCore/rendering/RenderTableCell.cpp:190
>>> +}
>> 
>> After doing some search, I've started to think that this isn't the right fix.
> 
> Hmm, but wouldn't returning true for 'hasLineIfEmpty' cause the empty td's to have the right line height?

The problem is that "right line height" is superfluous here since it doesn’t really have a height on other browsers.

>> LayoutTests/editing/deleting/5206311-2-expected.txt:-30
>> -|         <br>
> 
> We need these br for empty td's to have the right line height.

To clarify this, we need to produce markup that could be rendered by other browsers properly.

[Manuel Rego Casasnovas]

*** Bug 166685 has been marked as a duplicate of this bug. ***

[Manuel Rego Casasnovas]

BTW, this has been fixed recently in Blink with a similar patch to the one discarded here:
https://codereview.chromium.org/2601923003

I'm going to upload a rebased version so we can discuss about it again.

[Manuel Rego Casasnovas]

Created attachment 298013 [details]
Patch

[Build Bot]

Comment on attachment 298013 [details]
Patch

Attachment 298013 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2829232

New failing tests:
editing/deleting/5483370.html
editing/selection/select-all-004.html
editing/selection/move-by-line-001.html
editing/deleting/5206311-1.html
editing/deleting/5433862-2.html
editing/deleting/5126166.html

[Build Bot]

Created attachment 298016 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298013 [details]
Patch

Attachment 298013 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2829237

New failing tests:
editing/deleting/5483370.html
editing/selection/select-all-004.html
editing/selection/move-by-line-001.html
editing/deleting/5206311-1.html
editing/deleting/5433862-2.html
editing/deleting/5126166.html

[Build Bot]

Created attachment 298017 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298013 [details]
Patch

Attachment 298013 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2829236

New failing tests:
editing/deleting/5483370.html
editing/selection/select-all-004.html
editing/selection/move-by-line-001.html
editing/deleting/5206311-1.html
editing/deleting/5433862-2.html
editing/deleting/5126166.html

[Build Bot]

Created attachment 298018 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298013 [details]
Patch

Attachment 298013 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2829248

New failing tests:
editing/deleting/5483370.html
editing/deleting/5206311-1.html
editing/deleting/5433862-2.html
editing/deleting/5126166.html

[Build Bot]

Created attachment 298020 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Manuel Rego Casasnovas]

Created attachment 298023 [details]
Patch

[Build Bot]

Comment on attachment 298023 [details]
Patch

Attachment 298023 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2829624

New failing tests:
editing/deleting/5483370.html
editing/deleting/5206311-1.html
editing/deleting/5433862-2.html
editing/deleting/5126166.html

[Build Bot]

Created attachment 298030 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Manuel Rego Casasnovas]

Created attachment 298069 [details]
Patch

[Manuel Rego Casasnovas]

After getting the new baselines for Mac and iOS, EWSs are now happy.

@rniwa could you take a look to the patch and tell me if the concerns are still valid and we should look for a different approach? Thanks.

[Darin Adler]

Comment on attachment 298069 [details]
Patch

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

> Source/WebCore/rendering/RenderTableCell.h:136
> +    bool hasLineIfEmpty() const override;

I would have this override be private and final, since nobody needs to call it using a RenderTableCell&, they only want to call it polymorphically and this is a final class so our coding style says we should say final rather than override.

[Darin Adler]

Comment on attachment 298069 [details]
Patch

Oops, this wasn’t for me to review. Ryosuke needs to review it.

[Manuel Rego Casasnovas]

Created attachment 298085 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 298069 [details]
Patch

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

Yeah it's probably better wait for Ryosuke feedback.

>> Source/WebCore/rendering/RenderTableCell.h:136
>> +    bool hasLineIfEmpty() const override;
> 
> I would have this override be private and final, since nobody needs to call it using a RenderTableCell&, they only want to call it polymorphically and this is a final class so our coding style says we should say final rather than override.

Make sense. Thanks for the suggestion.

[Manuel Rego Casasnovas]

rniwa is not sure if this is the right solution and suggested to ask antti & zalan.
Please could you take a look? Thanks!

[Michael Catanzaro]

(In reply to Manuel Rego Casasnovas from comment #45)
> rniwa is not sure if this is the right solution and suggested to ask antti &
> zalan.
> Please could you take a look? Thanks!

Maybe if we say his name three times fast, Zalan will appear?

Zalanzalanzalan

[zalan]

(In reply to Michael Catanzaro from comment #46)
> (In reply to Manuel Rego Casasnovas from comment #45)
> > rniwa is not sure if this is the right solution and suggested to ask antti &
> > zalan.
> > Please could you take a look? Thanks!
> 
> Maybe if we say his name three times fast, Zalan will appear?
> 
> Zalanzalanzalan
lol, what can I do for you, Sir? :)
will talk to rniwa tomorrow and r+/-.

[Manuel Rego Casasnovas]

(In reply to zalan from comment #47)
> (In reply to Michael Catanzaro from comment #46)
> > (In reply to Manuel Rego Casasnovas from comment #45)
> > > rniwa is not sure if this is the right solution and suggested to ask antti &
> > > zalan.
> > > Please could you take a look? Thanks!
> > 
> > Maybe if we say his name three times fast, Zalan will appear?
> > 
> > Zalanzalanzalan
> lol, what can I do for you, Sir? :)
> will talk to rniwa tomorrow and r+/-.

Wow I had forgotten about this issue, BTW the fix landed more than 1 year ago in Blink: https://bugs.webkit.org/show_bug.cgi?id=85385

[Michael Catanzaro]

We need to do a better job of making sure our patches get reviewed so we don't miss useful stuff like this. It only works if we check the "My Requests" link at the top of Bugzilla every once in a while and ping reviewers on old patches.

[Manuel Rego Casasnovas]

Created attachment 338200 [details]
Patch

[Manuel Rego Casasnovas]

I have uploaded a rebased version to verify EWSs before landing.

(In reply to Michael Catanzaro from comment #49)
> We need to do a better job of making sure our patches get reviewed so we
> don't miss useful stuff like this. It only works if we check the "My
> Requests" link at the top of Bugzilla every once in a while and ping
> reviewers on old patches.

Yeah, I could have re-pinged again here or in IRC but I forgot about this one.
TBH, I was pinging Zalan on IRC way back in time, but he was really busy at that time, and I ended up forgetting about this.
On top of that I didn't know the "Zalanzalanzalan" trick, it worked really nice. :-)

Note that the patch is essentially the same than in 2012, so it's good it's finally landing.

[EWS Watchlist]

Comment on attachment 338200 [details]
Patch

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

New failing tests:
editing/deleting/5483370.html
editing/deleting/5206311-1.html
editing/deleting/5433862-2.html
editing/deleting/5126166.html

[EWS Watchlist]

Created attachment 338205 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Manuel Rego Casasnovas]

Created attachment 338206 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 338206 [details]
Patch

Clearing flags on attachment: 338206

Committed r230797: <https://trac.webkit.org/changeset/230797>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39556189>