[Webkit-unassigned] [Bug 90611] contenteditable justify commands applied to next paragraph as well

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 1 11:23:43 PDT 2013


https://bugs.webkit.org/show_bug.cgi?id=90611


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #213098|review?                     |review-
               Flag|                            |




--- Comment #20 from Ryosuke Niwa <rniwa at webkit.org>  2013-10-01 11:22:39 PST ---
(From update of attachment 213098)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.


More information about the webkit-unassigned mailing list