Bug 114960 - Wrong text position when you click backspace on the line below the image
Summary: Wrong text position when you click backspace on the line below the image
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-22 08:30 PDT by Krzysztof Wolanski
Modified: 2014-03-21 09:52 PDT (History)
12 users (show)

See Also:


Attachments
HTML page, shows unexpected behavior (246 bytes, text/html)
2013-04-23 01:19 PDT, Krzysztof Wolanski
no flags Details
HTML page, shows unexpected behavior (202 bytes, text/html)
2013-04-24 02:30 PDT, Krzysztof Wolanski
no flags Details
Bug quick fix patch to be discussed. (1.43 KB, patch)
2013-04-26 06:19 PDT, Lukasz Gajowy
no flags Details | Formatted Diff | Diff
Invalid insertion position computation - to discuss (1.99 KB, patch)
2013-05-07 07:29 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
Layout test testing the issue. (2.58 KB, patch)
2013-05-09 05:44 PDT, Lukasz Gajowy
rniwa: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Fix placement position when inserting as the last child of a node (2.45 KB, patch)
2013-05-09 06:04 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
Fix placement position when inserting as the last child of a node (2.45 KB, patch)
2013-05-09 07:03 PDT, Wojciech Bielawski
rniwa: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (495.26 KB, application/zip)
2013-05-09 08:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (578.60 KB, application/zip)
2013-05-09 08:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (561.32 KB, application/zip)
2013-05-09 08:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (497.88 KB, application/zip)
2013-05-09 12:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (776.70 KB, application/zip)
2013-05-09 12:55 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (517.40 KB, application/zip)
2013-05-13 16:06 PDT, Build Bot
no flags Details
Patch (4.55 KB, patch)
2014-03-05 05:12 PST, Sudarshan C P
no flags Details | Formatted Diff | Diff
Patch (4.88 KB, patch)
2014-03-12 02:06 PDT, Sudarshan C P
no flags Details | Formatted Diff | Diff
Patch (5.04 KB, patch)
2014-03-13 23:29 PDT, Sudarshan C P
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (493.24 KB, application/zip)
2014-03-14 02:25 PDT, Build Bot
no flags Details
Patch (5.04 KB, patch)
2014-03-14 02:51 PDT, Sudarshan C P
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (516.29 KB, application/zip)
2014-03-14 03:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (547.36 KB, application/zip)
2014-03-14 04:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (598.60 KB, application/zip)
2014-03-14 04:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (548.72 KB, application/zip)
2014-03-14 04:55 PDT, Build Bot
no flags Details
Patch (4.96 KB, patch)
2014-03-16 21:06 PDT, Sudarshan C P
darin: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (558.53 KB, application/zip)
2014-03-16 21:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (543.14 KB, application/zip)
2014-03-16 22:26 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (550.01 KB, application/zip)
2014-03-16 23:39 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Wolanski 2013-04-22 08:30:03 PDT
Steps to Reproduce:
  1)Open the website that contains:
    <DIV><SPAN style="color:#FF0000"><IMG src="black.jpg"></SPAN><br>
    <SPAN style="color:#FF0000">text</SPAN>
    </DIV>
  2) Press backspace in front of "text".

Actual Results: 
  The "text" goes in front of the IMG:
    <DIV><SPAN style=\94color:#FF0000">text</SPAN>
    <SPAN style=\94color:#FF0000"><IMG src=\94xxx\94/></SPAN>
    </DIV>

Expected Results:
  The "text" goes behind the image:
    <DIV><SPAN style=\94color:#FF0000"><IMG src=\94xxx\94/></SPAN>
    <SPAN style=\94color:#FF0000">text</ SPAN >
    </DIV>

Build Date & Platform:
  Build 2013-04-22 on Ubuntu 12.04 LTS
Comment 1 Krzysztof Wolanski 2013-04-23 01:19:27 PDT
Created attachment 199169 [details]
HTML page, shows unexpected behavior
Comment 2 Alexey Proskuryakov 2013-04-23 12:59:43 PDT
I cannot reproduce this with r148978.
Comment 3 Krzysztof Wolanski 2013-04-24 02:30:52 PDT
Created attachment 199407 [details]
 HTML page, shows unexpected behavior
Comment 4 Krzysztof Wolanski 2013-04-24 02:36:52 PDT
In description there is a mistake, should be:
Steps to Reproduce:
  1)Open the website that contains:
    <DIV><SPAN style="color:#FF0000"><IMG src="black.jpg"></SPAN></DIV>
    <DIV><SPAN style="color:#FF0000">text</SPAN>
    </DIV>
  2) Press backspace in front of "text".

Sorry for misleading.
Comment 5 Lukasz Gajowy 2013-04-24 04:11:41 PDT
Changing m_upstreamStart to m_upstreamEnd in:

https://trac.webkit.org/browser/trunk/Source/WebCore/editing/DeleteSelectionCommand.cpp#L607

fixes this issue. What do you think of this solution?
Comment 6 Ryosuke Niwa 2013-04-24 10:49:34 PDT
(In reply to comment #5)
> Changing m_upstreamStart to m_upstreamEnd in:
> 
> https://trac.webkit.org/browser/trunk/Source/WebCore/editing/DeleteSelectionCommand.cpp#L607
> 
> fixes this issue. What do you think of this solution?

That doesn't sound like a good solution without a more detailed description as to why that's the right fix.
Comment 7 Lukasz Gajowy 2013-04-25 08:10:13 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Changing m_upstreamStart to m_upstreamEnd in:
> > 
> > https://trac.webkit.org/browser/trunk/Source/WebCore/editing/DeleteSelectionCommand.cpp#L607
> > 
> > fixes this issue. What do you think of this solution?
> 
> That doesn't sound like a good solution without a more detailed description as to why that's the right fix.

As i was debugging the code I assumed that m_upstreamEnd is the Position of the end of an image (according to the test page included). I came up to a conclusion, that the merge destination should be set to the m_upstreamEnd, because when we hit backspace, we want to have the text at the end of the image, not at the start of it. I am not sure that I understand 
this code well. Please correct me if I am wrong.

Another thesis is that the problem stems from wrong behaviour of mergeParagraphs() method, when in m_upstreamStart field (it is a Position class object):
 - m_offset = 0
 - m_anchorNode = 2 (AnchorType = PositionIsAfterAnchor)

What do you think of that?
Comment 8 Lukasz Gajowy 2013-04-26 06:19:37 PDT
Created attachment 199823 [details]
Bug quick fix patch to be discussed.

Please refer to this solution and comment it.
Thank you in advance.
Comment 9 Wojciech Bielawski 2013-05-07 07:26:27 PDT
I see suspicious behaviour in ReplaceSelectionCommand::doApply() 
When insertion position points after last child of a container node then new insertion position is computed (lines 1046-1054) incorrectly. 
I prepared a patch, but it breaks 2 other layout tests. Any suggestions how the computation should looks like?
Comment 10 Wojciech Bielawski 2013-05-07 07:29:24 PDT
Created attachment 200902 [details]
Invalid insertion position computation - to discuss
Comment 11 Lukasz Gajowy 2013-05-09 05:44:27 PDT
Created attachment 201183 [details]
Layout test testing the issue.
Comment 12 Wojciech Bielawski 2013-05-09 06:04:27 PDT
Created attachment 201184 [details]
Fix placement position when inserting as the last child of a node
Comment 13 WebKit Commit Bot 2013-05-09 06:06:59 PDT
Attachment 201184 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/editing/ReplaceSelectionCommand.cpp']" exit_code: 1
Source/WebCore/editing/ReplaceSelectionCommand.cpp:1052:  Declaration has space between type name and * in Node *insertAfterNode  [whitespace/declaration] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Wojciech Bielawski 2013-05-09 07:03:27 PDT
Created attachment 201189 [details]
Fix placement position when inserting as the last child of a node

Style correction
Comment 15 Build Bot 2013-05-09 08:11:22 PDT
Comment on attachment 201189 [details]
Fix placement position when inserting as the last child of a node

Attachment 201189 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/404029

New failing tests:
editing/pasteboard/display-block-on-spans.html
Comment 16 Build Bot 2013-05-09 08:11:24 PDT
Created attachment 201235 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.2
Comment 17 Build Bot 2013-05-09 08:12:25 PDT
Comment on attachment 201183 [details]
Layout test testing the issue.

Attachment 201183 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/326086

New failing tests:
editing/deleting/merge-image-and-text.html
Comment 18 Build Bot 2013-05-09 08:12:29 PDT
Created attachment 201236 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 19 Build Bot 2013-05-09 08:40:32 PDT
Comment on attachment 201189 [details]
Fix placement position when inserting as the last child of a node

Attachment 201189 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/432012

New failing tests:
editing/pasteboard/display-block-on-spans.html
Comment 20 Build Bot 2013-05-09 08:40:35 PDT
Created attachment 201242 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.2
Comment 21 Build Bot 2013-05-09 12:21:01 PDT
Comment on attachment 201183 [details]
Layout test testing the issue.

Attachment 201183 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/349163

New failing tests:
editing/deleting/merge-image-and-text.html
Comment 22 Build Bot 2013-05-09 12:21:04 PDT
Created attachment 201263 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.2
Comment 23 Build Bot 2013-05-09 12:55:46 PDT
Comment on attachment 201183 [details]
Layout test testing the issue.

Attachment 201183 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/349167

New failing tests:
editing/deleting/merge-image-and-text.html
Comment 24 Build Bot 2013-05-09 12:55:49 PDT
Created attachment 201266 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.2
Comment 25 Wojciech Bielawski 2013-05-10 03:28:54 PDT
I have no option to test it with MAC platform. Could someone take a look does the patch is acceptable?
Comment 26 Build Bot 2013-05-13 16:06:40 PDT
Comment on attachment 201189 [details]
Fix placement position when inserting as the last child of a node

Attachment 201189 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/265962

New failing tests:
editing/pasteboard/display-block-on-spans.html
Comment 27 Build Bot 2013-05-13 16:06:45 PDT
Created attachment 201642 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 28 Ryosuke Niwa 2013-05-16 08:46:19 PDT
Comment on attachment 201189 [details]
Fix placement position when inserting as the last child of a node

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

> Source/WebCore/ChangeLog:8
> +        Layout test added in separate patch: https://bug-114960-attachments.webkit.org/attachment.cgi?id=201183

Please combine that patch into this.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1054
> +                    if (!insertAfterNode)
> +                        insertAfterNode = nodeToSplitTo.get();

This doesn't look right. We shouldn't be using the position after the nodeToSplit ever.
We probably need to get the position before the nodeToSplitTo in this case.
Comment 29 Ryosuke Niwa 2013-05-16 08:49:48 PDT
Comment on attachment 201183 [details]
Layout test testing the issue.

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

We don't add a test by itself. This patch needs to be merged with the actual fix.

> LayoutTests/editing/deleting/merge-image-and-text.html:13
> +Markup.description('Testcase for bug https://webkit.org/b/114960: Wrong text position when you click backspace on the line below the image.\n'+
> +'The test passes if "text1" appears on the right side of the image.');

Since everyone can look at trac or svn blame to figure out for which bug this test was added, it's not that interesting to repeat the bug URL and title.
Instead you should describe what this test is doing; the specific condition under which deletion occurs.

> LayoutTests/editing/deleting/merge-image-and-text.html:16
> +var testedHTML = document.getElementById('editableContent');

We should name this variable like container or editableContent to be more descriptive.
Comment 30 KyungTae Kim 2013-10-18 00:18:32 PDT
Is there any progress on this?
Comment 31 Sudarshan C P 2014-03-05 05:12:53 PST
Created attachment 225874 [details]
Patch
Comment 32 Ryosuke Niwa 2014-03-10 21:37:08 PDT
Comment on attachment 225874 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        While finding new position for inserting element after deleting/pasting,
> +        included image element also for find the proper position to merge the split node.

This description should be appear before "Test:" line but after "Reviewed by" line.
But this doesn't really describe what was causing the bug and how we're fixing it.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:119
>      // If we're already on a break, it's probably a placeholder and we shouldn't change our position.
> -    if (editingIgnoresContent(pos.deprecatedNode()))
> +    if (editingIgnoresContent(pos.deprecatedNode()) && !pos.deprecatedNode()->hasTagName(imgTag))

The problem here is that editingIgnoresContent returns true on more than just a placeholder.
We should be checking whether the deprecatedNode is br or not instead.
What's weird is that lineBreakExistsAtPosition below should already be checking this condition.

r- because special-casing img can't be the right fix. e.g. it'll fail for other elements such as object and hr that are ignored by editing.
Comment 33 Sudarshan C P 2014-03-11 02:42:46 PDT
(In reply to comment #32)
> (From update of attachment 225874 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225874&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        While finding new position for inserting element after deleting/pasting,
> > +        included image element also for find the proper position to merge the split node.
> 
> This description should be appear before "Test:" line but after "Reviewed by" line.
> But this doesn't really describe what was causing the bug and how we're fixing it.
> 
i will modify the above comments.

> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:119
> >      // If we're already on a break, it's probably a placeholder and we shouldn't change our position.
> > -    if (editingIgnoresContent(pos.deprecatedNode()))
> > +    if (editingIgnoresContent(pos.deprecatedNode()) && !pos.deprecatedNode()->hasTagName(imgTag))
> 
> The problem here is that editingIgnoresContent returns true on more than just a placeholder.
> We should be checking whether the deprecatedNode is br or not instead.
> What's weird is that lineBreakExistsAtPosition below should already be checking this condition.
> 
> r- because special-casing img can't be the right fix. e.g. it'll fail for other elements such as object and hr that are ignored by editing.

root cause for this issue is nodeToSplitToAvoidPastingIntolineNodeWithStyle() is trying to find the highestEnclosingNodeType(), so elementIsStyleOrHTMLEquivalent() matchedAttributes<=element->attributeount() condition satisfies and wrong position gives in case of image element to split and merge with new node, hence added the image condition to over come this error.

Thanks for your comments i will try to find the new set of patch as per comments and update.
Comment 34 Sudarshan C P 2014-03-12 02:06:29 PDT
Created attachment 226484 [details]
Patch
Comment 35 Darin Adler 2014-03-13 11:00:18 PDT
Comment on attachment 226484 [details]
Patch

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

This patch is OK, but I’d like to see a better test. We shouldn’t land with a test that we know will fail on some platforms.

> LayoutTests/editing/deleting/merge-image-and-text-expected.txt:11
> +            RenderText {#text} at (4,0) size 31x17

This is non-portable, so this test is going to fail on some platforms. The text might not be 31 pixels wide on every platform due to font differences.

> LayoutTests/editing/deleting/merge-image-and-text.html:1
> +<!DOCTYPE html>

It’s much better for editing tests to dump markup rather than a render tree. Look at a recently added editing tests such as editing/execCommand/indent-img-twice.html to see how to do this. It makes a much useful test for the WebKit project. And deals with the portability issue I mentioned above.
Comment 36 Darin Adler 2014-03-13 11:07:11 PDT
Comment on attachment 226484 [details]
Patch

I’d like to see test cases covering more. Ryosuke pointed out that <object> and <br> are other cases that we might have broken if we did this wrong. We should use his insight to make test cases. We want test cases that would have shown the problems with earlier incorrect versions of this patch.
Comment 37 Sudarshan C P 2014-03-13 23:29:51 PDT
Created attachment 226654 [details]
Patch
Comment 38 Build Bot 2014-03-14 02:25:34 PDT
Comment on attachment 226654 [details]
Patch

Attachment 226654 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4576228228988928

New failing tests:
editing/deleting/merge-image-and-text.html
Comment 39 Build Bot 2014-03-14 02:25:39 PDT
Created attachment 226669 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 40 Sudarshan C P 2014-03-14 02:51:03 PDT
Created attachment 226672 [details]
Patch
Comment 41 Build Bot 2014-03-14 03:52:54 PDT
Comment on attachment 226672 [details]
Patch

Attachment 226672 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5830178827665408

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html
Comment 42 Build Bot 2014-03-14 03:52:59 PDT
Created attachment 226677 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 43 Build Bot 2014-03-14 04:15:40 PDT
Comment on attachment 226672 [details]
Patch

Attachment 226672 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5187704870404096

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html
Comment 44 Build Bot 2014-03-14 04:15:47 PDT
Created attachment 226679 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 45 Build Bot 2014-03-14 04:41:54 PDT
Comment on attachment 226672 [details]
Patch

Attachment 226672 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4745941714206720

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html
Comment 46 Build Bot 2014-03-14 04:41:59 PDT
Created attachment 226684 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 47 Build Bot 2014-03-14 04:55:15 PDT
Comment on attachment 226672 [details]
Patch

Attachment 226672 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6374369974550528

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html
Comment 48 Build Bot 2014-03-14 04:55:21 PDT
Created attachment 226686 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 49 Ryosuke Niwa 2014-03-14 17:50:18 PDT
Comment on attachment 226672 [details]
Patch

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

> LayoutTests/editing/deleting/merge-image-and-text.html:6
> +<div><span><img src="some_img" style="color:#FF0000"></span></div>

Can't we use ../abe.png instead?  Why do we need to have style attribute?  Is that required?

> LayoutTests/editing/deleting/merge-image-and-text.html:7
> +<div><span id="textSpan" style="color:#FF0000">text</span></div>

Ditto about style attribute.

> LayoutTests/editing/deleting/merge-image-and-text.html:11
> +Markup.description('Wrong text position when you click backspace on the line below the image.');

This description isn't descriptive enough.  We should explain what kind of behavior this test is testing.

> LayoutTests/editing/deleting/merge-image-and-text.html:15
> +var selection = window.getSelection();

We don't need this local variable.

> LayoutTests/editing/deleting/merge-image-and-text.html:16
> +Markup.dump(editElement, 'Before Delete');

I don't see a value in dumping the content without selection since that's basically identical to the content with caret in it.

> LayoutTests/editing/deleting/merge-image-and-text.html:18
> +selection.collapse(text, text.childNodes.length -1);

Just call getSelecrtion() here.
Nit: text.childNodes.length - 1.

> LayoutTests/editing/deleting/merge-image-and-text.html:19
> +Markup.dump(text, 'Place the cursor at');

It's useless to dump the content of text because we don't know where "text" is located at in the original content.
We should dump the whole editElement here and label this one as 'Before deletion' instead.

> LayoutTests/editing/deleting/merge-image-and-text.html:22
> +Markup.dump(editElement, 'After Delete');

'After deletion'.
Comment 50 Sudarshan C P 2014-03-16 21:06:59 PDT
Created attachment 226874 [details]
Patch
Comment 51 Build Bot 2014-03-16 21:34:56 PDT
Comment on attachment 226874 [details]
Patch

Attachment 226874 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6516534465789952

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html
Comment 52 Build Bot 2014-03-16 21:35:01 PDT
Created attachment 226877 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 53 Build Bot 2014-03-16 22:26:28 PDT
Comment on attachment 226874 [details]
Patch

Attachment 226874 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6281862553010176

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html
Comment 54 Build Bot 2014-03-16 22:26:34 PDT
Created attachment 226881 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 55 Build Bot 2014-03-16 23:39:13 PDT
Comment on attachment 226874 [details]
Patch

Attachment 226874 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4855721615163392

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/paste-and-sanitize.html
editing/pasteboard/paste-after-inline-style-element.html
editing/pasteboard/merge-end-1.html
editing/pasteboard/paste-with-redundant-style.html
editing/pasteboard/paste-text-with-style-3.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/merge-end-2.html
Comment 56 Build Bot 2014-03-16 23:39:19 PDT
Created attachment 226888 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 57 Darin Adler 2014-03-21 09:52:29 PDT
Comment on attachment 226874 [details]
Patch

The EWS bot shows 8 regression tests failing.

  editing/pasteboard/5065605.html [ Failure ]
  editing/pasteboard/merge-end-1.html [ Failure ]
  editing/pasteboard/merge-end-2.html [ Failure ]
  editing/pasteboard/paste-after-inline-style-element.html [ Failure ]
  editing/pasteboard/paste-and-sanitize.html [ Failure ]
  editing/pasteboard/paste-text-011.html [ Failure ]
  editing/pasteboard/paste-text-with-style-3.html [ Failure ]
  editing/pasteboard/paste-with-redundant-style.html [ Failure ]

The patch needs to include new results for those tests if the “failures” are actually progressions, or somehow resolve the failures.
Comment 58 Darin Adler 2014-03-21 09:52:55 PDT
The new test in the patch itself looks really good!