| Summary: | contenteditable justify commands applied to next paragraph as well | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sijmen Mulder <sjmulder> | ||||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, rniwa, santosh.ma | ||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
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.
Created attachment 212925 [details]
Work in progress
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 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
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 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
Created attachment 212956 [details]
work in progress
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 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
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 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
Created attachment 213000 [details]
WIP
Created attachment 213010 [details]
Patch
The patch is completed. please review it 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? 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? Created attachment 213095 [details]
Patch
> > >> 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. Created attachment 213098 [details]
Patch
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. Created attachment 213111 [details]
Patch
Created attachment 213150 [details]
Patch for Landing
Comment on attachment 213150 [details] Patch for Landing Clearing flags on attachment: 213150 Committed r156764: <http://trac.webkit.org/changeset/156764> All reviewed patches have been landed. Closing bug. |
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.