Bug 110979

Summary: Selection direction is not preserved when applying styles
Product: WebKit Reporter: Shezan Baig <shezbaig.wk>
Component: HTML EditingAssignee: Sukolsak Sakshuwong <sukolsak>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, dglazkov, jfernandez, mifenton, rniwa, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 114612    
Bug Blocks:    
Attachments:
Description Flags
Patch
rniwa: review+, webkit.review.bot: commit-queue-
Patch none

Description Shezan Baig 2013-02-27 07:12:41 PST
ApplyStyleCommand::updateStartEnd does not preserve the original selection's direction
Comment 1 Shezan Baig 2013-02-27 08:22:24 PST
Created attachment 190525 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-27 09:09:21 PST
Comment on attachment 190525 [details]
Patch

Attachment 190525 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16668112

New failing tests:
editing/style/block-style-005.html
Comment 3 Ryosuke Niwa 2013-02-27 10:28:09 PST
Comment on attachment 190525 [details]
Patch

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

> Source/WebCore/editing/ApplyStyleCommand.cpp:189
> +    Position base = wasBaseFirst ? newStart : newEnd;
> +    Position extent = wasBaseFirst ? newEnd : newStart;

I would have put this directly in the line below but okay.

> LayoutTests/editing/style/preserve-selection-direction-expected.txt:7
> +PASS sel.anchorOffset is 0
> +PASS sel.focusOffset is 5

By just looking at this output, it's not clear why these numbers should be correct.

> LayoutTests/editing/style/preserve-selection-direction.html:14
> +    var sel = window.getSelection();

Please don't use abbreviations like sel.

> LayoutTests/editing/style/preserve-selection-direction.html:16
> +    // test forward selection direction

I don't think this comment is useful.

> LayoutTests/editing/style/preserve-selection-direction.html:19
> +    sel.collapse(editable.firstChild, 3);
> +    sel.extend(editable.firstChild, 8);
> +    document.execCommand('foreColor', false, 'green');

I would have put all of this inside evalAndLog.

> LayoutTests/editing/style/preserve-selection-direction.html:23
> +    // reset and test backwards selection direction

I don't think this comment is useful.

> LayoutTests/editing/style/preserve-selection-direction.html:27
> +    editable.innerHTML = "This is some sample text";
> +    sel.collapse(editable.firstChild, 8);
> +    sel.extend(editable.firstChild, 3);
> +    document.execCommand('foreColor', false, 'green');

Ditto.
Comment 4 Ryosuke Niwa 2013-02-27 10:28:30 PST
Comment on attachment 190525 [details]
Patch

r=me with me that.
Comment 5 Shezan Baig 2013-02-27 10:54:44 PST
Thanks, I will incorporate your comments, and I'm also looking into the failing test: block-style-005.html
Comment 6 Build Bot 2013-02-27 11:07:38 PST
Comment on attachment 190525 [details]
Patch

Attachment 190525 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16791116

New failing tests:
editing/execCommand/remove-format-orphaned-list-item.html
editing/execCommand/4920488.html
editing/execCommand/4920742-1.html
editing/style/apply-style-atomic.html
fast/events/autoscroll-in-textarea.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
fast/frames/sandboxed-iframe-scripting.html
fast/history/visited-link-background-color.html
editing/style/apple-style-editable-mix.html
editing/execCommand/remove-format-background-color.html
editing/execCommand/4786404-1.html
editing/execCommand/5049671.html
editing/style/remove-styled-element-with-style-span.html
editing/execCommand/5573879.html
editing/inserting/space-after-removeformat.html
fast/events/event-sender-mouse-moved.html
editing/execCommand/4786404-2.html
editing/execCommand/remove-formatting.html
http/tests/cookies/third-party-cookie-relaxing.html
editing/style/style-text-node-without-editable-parent.html
Comment 7 Build Bot 2013-02-28 05:20:05 PST
Comment on attachment 190525 [details]
Patch

Attachment 190525 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16855052

New failing tests:
editing/execCommand/remove-format-orphaned-list-item.html
editing/execCommand/4920488.html
editing/execCommand/4920742-1.html
editing/style/apply-style-atomic.html
editing/style/apple-style-editable-mix.html
platform/mac/editing/input/bold-node.html
editing/execCommand/4786404-2.html
editing/execCommand/remove-format-background-color.html
editing/execCommand/4786404-1.html
editing/execCommand/5049671.html
editing/execCommand/5573879.html
editing/inserting/space-after-removeformat.html
editing/execCommand/remove-formatting.html
editing/style/remove-styled-element-with-style-span.html
editing/style/style-text-node-without-editable-parent.html
Comment 8 Sukolsak Sakshuwong 2013-04-13 16:18:55 PDT
Created attachment 197960 [details]
Patch
Comment 9 WebKit Commit Bot 2013-04-13 20:54:14 PDT
Comment on attachment 197960 [details]
Patch

Clearing flags on attachment: 197960

Committed r148378: <http://trac.webkit.org/changeset/148378>
Comment 10 Shezan Baig 2013-04-14 05:27:16 PDT
Thanks Sukolsak!
Comment 11 Radar WebKit Bug Importer 2017-12-15 01:35:26 PST
<rdar://problem/36070206>