Bug 90611 – contenteditable justify commands applied to next paragraph as well

• Component: New Bugs
• Status: RESOLVED FIXED

[Sijmen Mulder]

Created attachment 150937 [details]
Sample case

When executing the justifyLeft, justifyRight or justifyFull command when a paragraph is selected in a contenteidtable element, the justification is applied to the paragraph after the selected paragraph as well.

Steps to reproduce:

1. Create a contenteditable element.
2. Have three paragraphs as its contents: <p>Paragraph one</p><p>Paragraph two</p><p>Paragraph three</p>
3. Select the first paragraph by triple-clicking on it or by dragging down from the start of the paragraph to the start of the next paragraph.
4. Call document.execCommand('justfiyRight', false, null)

Expected results:

* The first paragraph is justified to the right.

Actual results:

* The first and second paragraphs are both justified to the right, even though the second was not selected.

Note that if the paragraph is only selected up to and including the final character, and not the newline, the justification works correctly.

I’ve done some digging and this is what I’ve found: when a paragraph is selected in its entirely, the selection range (window.getSelection().range) extends until position 0 of the next paragraph. Perhaps that’s why the justification is applied to that paragraph as well, however, it’s not the user intention.

Find attached a sample page that demonstrates the issue.

[Sijmen Mulder]

Created attachment 151425 [details]
Hacky workaround

I’ve attached a sort of hacky workaround with a function that can be called to adjust the selection before performing the justify* commands.

[Santosh Mahto]

Created attachment 212925 [details]
Work in progress

[Build Bot]

Comment on attachment 212925 [details]
Work in progress

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

New failing tests:
editing/execCommand/justify.html
editing/style/create-block-for-style-013.html
editing/style/create-block-for-style-002.html
editing/execCommand/25320.html
editing/execCommand/5080333-1.html
editing/style/4230923.html
editing/execCommand/5062376.html
editing/style/create-block-for-style-003.html
editing/execCommand/25256.html
editing/deleting/delete-start-block.html
editing/execCommand/15381.html
editing/execCommand/5080333-2.html

[Build Bot]

Created attachment 212926 [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

[Build Bot]

Comment on attachment 212925 [details]
Work in progress

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

New failing tests:
editing/execCommand/justify.html
editing/style/create-block-for-style-013.html
editing/style/create-block-for-style-002.html
editing/execCommand/25320.html
editing/execCommand/5080333-1.html
editing/style/4230923.html
editing/execCommand/5062376.html
editing/style/create-block-for-style-003.html
editing/execCommand/25256.html
editing/deleting/delete-start-block.html
editing/execCommand/15381.html
editing/execCommand/5080333-2.html

[Build Bot]

Created attachment 212928 [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

[Santosh Mahto]

Created attachment 212956 [details]
work in progress

[Build Bot]

Comment on attachment 212956 [details]
work in progress

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

New failing tests:
editing/execCommand/align-in-span.html

[Build Bot]

Created attachment 212958 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 212956 [details]
work in progress

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

New failing tests:
editing/execCommand/align-in-span.html

[Build Bot]

Created attachment 212963 [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.5

[Santosh Mahto]

Created attachment 213000 [details]
WIP

[Santosh Mahto]

Created attachment 213010 [details]
Patch

[Santosh Mahto]

The patch is completed. please review it

[Darin Adler]

Comment on attachment 213010 [details]
Patch

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

> Source/WebCore/editing/ApplyStyleCommand.cpp:274
> +    VisiblePosition beyondEnd((visibleEnd.deepEquivalent().deprecatedEditingOffset() || visibleEnd == paragraphStart) ? endOfParagraph(visibleEnd).next() : visibleEnd);

Can you do this without using anything with “deprecated” in its name?

[Ryosuke Niwa]

Comment on attachment 213010 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        (WebCore::ApplyStyleCommand::applyBlockStyle):Ignore the justify
> +        command for paragraph if selection is not part of it.

Part of what?

>> Source/WebCore/editing/ApplyStyleCommand.cpp:274
>> +    VisiblePosition beyondEnd((visibleEnd.deepEquivalent().deprecatedEditingOffset() || visibleEnd == paragraphStart) ? endOfParagraph(visibleEnd).next() : visibleEnd);
> 
> Can you do this without using anything with “deprecated” in its name?

This change doesn't make much sense. What does the deprecated editing offset being non-zero to do with visibleEnd == paragraphStart?

> LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:4
> +<script type="text/javascript">

There is no need to specify type in HTML5.

> LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:12
> +    var elem1 = document.getElementById("p1");
> +    var elem2 = document.getElementById("p2");

Please don't use abbreviations like elem and p. Spell out words.

> LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:14
> +    var selection = window.getSelection();
> +    selection.setBaseAndExtent(elem1, 0, elem2, 0);

There is no need for the local variable here and "window.".

> LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:18
> +    document.getElementById("result").innerHTML=  "Final Result:<br>" +  "1st Paragraph:  " + elem1.getAttribute("style") +  "<br>" + str;

Why isn't this using dump-as-markup or js-test-pre.js?

> LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:30
> +<p id="result"> </p>

Why do we have a space here?

[Santosh Mahto]

Created attachment 213095 [details]
Patch

[Santosh Mahto]

> 
> >> Source/WebCore/editing/ApplyStyleCommand.cpp:274
> >> +    VisiblePosition beyondEnd((visibleEnd.deepEquivalent().deprecatedEditingOffset() || visibleEnd == paragraphStart) ? endOfParagraph(visibleEnd).next() : visibleEnd);
> > 
> > Can you do this without using anything with “deprecated” in its name?

I changed the code.

> 
> This change doesn't make much sense. What does the deprecated editing offset being non-zero to do with visibleEnd == paragraphStart?

I changed the code its now  visibleEnd != visibleStart.
 ApplyBlockElementCommand::doApply() : already  has same adjustment  for paragraph break. Same adjustment is done here.


I updated the testcase align-in-expected.txt as it was failing for this change.
I found on http://trac.webkit.org/changeset/50433  this test is for crash test.
and expected txt has not much significance.

[Santosh Mahto]

Created attachment 213098 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 213098 [details]
Patch

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

The code change looks much better but still r- because there are many stylistic issues with the patch,
and descriptions in change log entries need to be revised.

> Source/WebCore/ChangeLog:9
> +        end reaches upto next node with offset 0 just to include the

I don't know what you mean by "reaches up to next node with offset 0".

> Source/WebCore/ChangeLog:11
> +        paragraph element  who falls under selection causing next unselected

Typo: two spaces before "who".

> Source/WebCore/editing/ApplyStyleCommand.cpp:274
> +    // When a selection ends at the start of a paragraph and is range selection, style should not be applied to that paragraph

This comment repeats the code below. Please remove it.

> LayoutTests/ChangeLog:10
> +        TestCases verifying that range selection having end
> +        selection as paragraph and offset 0 should be ignored while
> +        treating as Justify candidate.

Why is "TestCases" camel case?  Also, this doesn't explain the end-user behavior.
e.g. Add a test for selecting one paragraph and the beginning of another paragraph to ensure that
only the first fully-selected paragraph should be justified when justifying the selection.

> LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:10
> +    <p id="p1">Paragraph one.</p>
> +    <p id="p2">Paragraph two.</p>

Please don't use abbreviations like p. Spell out paragraph.

> LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:17
> +    var firstPara= document.getElementById("p1");
> +    var secondPara = document.getElementById("p2");
> +    getSelection().setBaseAndExtent(firstPara, 0, secondPara, 0);

A much better way of accomplishing the same effect is to use getSelection().modify as in:
document.getElementById('container').focus();
getSelection().modify('Extend', 'Forward', 'Line');

> LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:20
> +    Markup.description('Only first Paragraph with id "p1" should have attribute style="text-align: center"');

We should also explain what we're testing here.
Otherwise, it won't be obvious as to why the output should be what's explained here.

[Santosh Mahto]

Created attachment 213111 [details]
Patch

[Santosh Mahto]

Created attachment 213150 [details]
Patch for Landing

[WebKit Commit Bot]

Comment on attachment 213150 [details]
Patch for Landing

Clearing flags on attachment: 213150

Committed r156764: <http://trac.webkit.org/changeset/156764>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.