Bug 85385 - Caret rendered at incorrect location inside empty table cell
Summary: Caret rendered at incorrect location inside empty table cell
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL: http://jsfiddle.net/wceLA/
Keywords: InRadar
: 166685 (view as bug list)
Depends on: 86217
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-02 09:50 PDT by Shezan Baig
Modified: 2018-04-19 00:13 PDT (History)
20 users (show)

See Also:


Attachments
Patch (6.14 KB, patch)
2012-05-21 12:45 PDT, Shezan Baig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (548.65 KB, application/zip)
2012-05-21 13:29 PDT, WebKit Review Bot
no flags Details
Patch (with gtk rebaseline) (33.24 KB, patch)
2012-05-23 14:50 PDT, Shezan Baig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (586.90 KB, application/zip)
2012-05-23 16:05 PDT, WebKit Review Bot
no flags Details
Patch (with gtk+chromium rebaseline) (150.09 KB, patch)
2012-06-06 09:48 PDT, Shezan Baig
no flags Details | Formatted Diff | Diff
Patch (with change from comment 13) (150.06 KB, patch)
2012-06-06 13:31 PDT, Shezan Baig
no flags Details | Formatted Diff | Diff
Patch (with rebaseline fix) (150.06 KB, patch)
2012-06-07 09:18 PDT, Shezan Baig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (759.55 KB, application/zip)
2012-06-07 19:19 PDT, WebKit Review Bot
no flags Details
Patch (with rebaseline fix) (145.35 KB, patch)
2012-06-08 09:17 PDT, Shezan Baig
no flags Details | Formatted Diff | Diff
Patch (108.53 KB, patch)
2017-01-04 08:07 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.11 MB, application/zip)
2017-01-04 09:07 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.15 MB, application/zip)
2017-01-04 09:11 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.77 MB, application/zip)
2017-01-04 09:16 PST, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (8.51 MB, application/zip)
2017-01-04 09:23 PST, Build Bot
no flags Details
Patch (310.46 KB, patch)
2017-01-04 09:30 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (7.52 MB, application/zip)
2017-01-04 11:00 PST, Build Bot
no flags Details
Patch (318.54 KB, patch)
2017-01-04 22:54 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (318.54 KB, patch)
2017-01-05 03:09 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (362.95 KB, patch)
2018-04-18 01:19 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.87 MB, application/zip)
2018-04-18 02:59 PDT, EWS Watchlist
no flags Details
Patch (456.22 KB, patch)
2018-04-18 03:14 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shezan Baig 2012-05-02 09:50:22 PDT
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.
Comment 1 Alexey Proskuryakov 2012-05-03 13:59:37 PDT
See also: bug 20129.
Comment 2 Shezan Baig 2012-05-03 14:01:03 PDT
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.
Comment 3 Ryosuke Niwa 2012-05-03 14:17:25 PDT
I think caret rendering belongs to the editing component still.
Comment 4 Shezan Baig 2012-05-04 14:02:21 PDT
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?
Comment 5 Ryosuke Niwa 2012-05-05 10:30:52 PDT
(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.
Comment 6 Shezan Baig 2012-05-21 12:45:38 PDT
Created attachment 143078 [details]
Patch
Comment 7 WebKit Review Bot 2012-05-21 13:29:23 PDT
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
Comment 8 WebKit Review Bot 2012-05-21 13:29:27 PDT
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
Comment 9 Shezan Baig 2012-05-23 14:50:37 PDT
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.
Comment 10 WebKit Review Bot 2012-05-23 16:05:10 PDT
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
Comment 11 WebKit Review Bot 2012-05-23 16:05:20 PDT
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
Comment 12 Shezan Baig 2012-06-06 09:48:34 PDT
Created attachment 146049 [details]
Patch (with gtk+chromium rebaseline)
Comment 13 Ryosuke Niwa 2012-06-06 11:56:57 PDT
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.
Comment 14 Shezan Baig 2012-06-06 12:35:39 PDT
(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 15 Shezan Baig 2012-06-06 13:31:00 PDT
Created attachment 146099 [details]
Patch (with change from comment 13)
Comment 16 WebKit Review Bot 2012-06-07 01:28:03 PDT
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
Comment 17 Shezan Baig 2012-06-07 09:18:43 PDT
Created attachment 146302 [details]
Patch (with rebaseline fix)

select-all-004-expected.png was overwritten simultaneously in r119630
Comment 18 WebKit Review Bot 2012-06-07 19:19:41 PDT
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
Comment 19 WebKit Review Bot 2012-06-07 19:19:45 PDT
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
Comment 20 Shezan Baig 2012-06-08 09:17:34 PDT
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 21 Ryosuke Niwa 2012-06-19 00:44:13 PDT
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.
Comment 22 Shezan Baig 2012-07-13 12:07:00 PDT
(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?
Comment 23 Ryosuke Niwa 2012-07-13 12:11:02 PDT
(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 24 Ryosuke Niwa 2012-12-03 13:55:25 PST
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.
Comment 25 Manuel Rego Casasnovas 2017-01-04 07:54:22 PST
*** Bug 166685 has been marked as a duplicate of this bug. ***
Comment 26 Manuel Rego Casasnovas 2017-01-04 07:58:34 PST
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.
Comment 27 Manuel Rego Casasnovas 2017-01-04 08:07:41 PST
Created attachment 298013 [details]
Patch
Comment 28 Build Bot 2017-01-04 09:07:00 PST
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
Comment 29 Build Bot 2017-01-04 09:07:04 PST
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
Comment 30 Build Bot 2017-01-04 09:11:41 PST
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
Comment 31 Build Bot 2017-01-04 09:11:45 PST
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
Comment 32 Build Bot 2017-01-04 09:15:58 PST
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
Comment 33 Build Bot 2017-01-04 09:16:03 PST
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
Comment 34 Build Bot 2017-01-04 09:23:22 PST
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
Comment 35 Build Bot 2017-01-04 09:23:26 PST
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
Comment 36 Manuel Rego Casasnovas 2017-01-04 09:30:35 PST
Created attachment 298023 [details]
Patch
Comment 37 Build Bot 2017-01-04 11:00:20 PST
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
Comment 38 Build Bot 2017-01-04 11:00:26 PST
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
Comment 39 Manuel Rego Casasnovas 2017-01-04 22:54:01 PST
Created attachment 298069 [details]
Patch
Comment 40 Manuel Rego Casasnovas 2017-01-05 00:28:12 PST
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 41 Darin Adler 2017-01-05 00:56:32 PST
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 42 Darin Adler 2017-01-05 01:01:01 PST
Comment on attachment 298069 [details]
Patch

Oops, this wasn’t for me to review. Ryosuke needs to review it.
Comment 43 Manuel Rego Casasnovas 2017-01-05 03:09:58 PST
Created attachment 298085 [details]
Patch
Comment 44 Manuel Rego Casasnovas 2017-01-05 03:13:52 PST
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.
Comment 45 Manuel Rego Casasnovas 2017-01-27 02:21:21 PST
rniwa is not sure if this is the right solution and suggested to ask antti & zalan.
Please could you take a look? Thanks!
Comment 46 Michael Catanzaro 2018-04-09 17:40:40 PDT
(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
Comment 47 zalan 2018-04-09 18:22:30 PDT
(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+/-.
Comment 48 Manuel Rego Casasnovas 2018-04-10 00:03:18 PDT
(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
Comment 49 Michael Catanzaro 2018-04-10 18:40:29 PDT
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.
Comment 50 Manuel Rego Casasnovas 2018-04-18 01:19:38 PDT
Created attachment 338200 [details]
Patch
Comment 51 Manuel Rego Casasnovas 2018-04-18 01:28:10 PDT
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.
Comment 52 EWS Watchlist 2018-04-18 02:59:06 PDT
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
Comment 53 EWS Watchlist 2018-04-18 02:59:17 PDT
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
Comment 54 Manuel Rego Casasnovas 2018-04-18 03:14:21 PDT
Created attachment 338206 [details]
Patch
Comment 55 WebKit Commit Bot 2018-04-19 00:10:56 PDT
Comment on attachment 338206 [details]
Patch

Clearing flags on attachment: 338206

Committed r230797: <https://trac.webkit.org/changeset/230797>
Comment 56 WebKit Commit Bot 2018-04-19 00:10:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 57 Radar WebKit Bug Importer 2018-04-19 00:13:38 PDT
<rdar://problem/39556189>