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.
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.
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?
(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.
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
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.
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
(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.
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
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
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)
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.
(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?
(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.
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.
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.
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
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
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
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
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
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.
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.
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.
(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
(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+/-.
(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
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.
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.
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
2012-05-21 12:45 PDT, Shezan Baig
2012-05-21 13:29 PDT, WebKit Review Bot
2012-05-23 14:50 PDT, Shezan Baig
2012-05-23 16:05 PDT, WebKit Review Bot
2012-06-06 09:48 PDT, Shezan Baig
2012-06-06 13:31 PDT, Shezan Baig
2012-06-07 09:18 PDT, Shezan Baig
2012-06-07 19:19 PDT, WebKit Review Bot
2012-06-08 09:17 PDT, Shezan Baig
2017-01-04 08:07 PST, Manuel Rego Casasnovas
2017-01-04 09:07 PST, Build Bot
2017-01-04 09:11 PST, Build Bot
2017-01-04 09:16 PST, Build Bot
2017-01-04 09:23 PST, Build Bot
2017-01-04 09:30 PST, Manuel Rego Casasnovas
2017-01-04 11:00 PST, Build Bot
2017-01-04 22:54 PST, Manuel Rego Casasnovas
2017-01-05 03:09 PST, Manuel Rego Casasnovas
2018-04-18 01:19 PDT, Manuel Rego Casasnovas
2018-04-18 02:59 PDT, EWS Watchlist
2018-04-18 03:14 PDT, Manuel Rego Casasnovas