[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