Bug 91912 - Adding "all" to -webkit-user-select
Summary: Adding "all" to -webkit-user-select
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, WebExposed
Depends on: 93562
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-20 17:26 PDT by Alice Cheng
Modified: 2012-11-01 15:59 PDT (History)
19 users (show)

See Also:


Attachments
Prototype (45.73 KB, patch)
2012-07-20 17:41 PDT, Alice Cheng
rniwa: review-
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
prototype (49.77 KB, patch)
2012-07-21 09:01 PDT, Alice Cheng
adele: review-
Details | Formatted Diff | Diff
part1 (6.35 KB, patch)
2012-08-07 15:45 PDT, Alice Cheng
rniwa: review-
rniwa: commit-queue-
Details | Formatted Diff | Diff
proposed patch for part2 (18.09 KB, patch)
2012-08-16 14:11 PDT, Alice Cheng
no flags Details | Formatted Diff | Diff
proposed patch for stylebot (18.09 KB, patch)
2012-08-16 14:16 PDT, Alice Cheng
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (421.07 KB, application/zip)
2012-08-16 15:38 PDT, WebKit Review Bot
no flags Details
proposed patch (20.35 KB, patch)
2012-08-17 12:14 PDT, Alice Cheng
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (313.53 KB, application/zip)
2012-08-17 13:28 PDT, WebKit Review Bot
no flags Details
proposed patch (18.05 KB, patch)
2012-08-20 10:06 PDT, Alice Cheng
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-06 (439.80 KB, application/zip)
2012-08-20 13:39 PDT, WebKit Review Bot
no flags Details
proposed patch for buildbot (18.10 KB, patch)
2012-08-20 14:13 PDT, Alice Cheng
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (325.24 KB, application/zip)
2012-08-20 14:32 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-07 (329.80 KB, application/zip)
2012-08-20 17:27 PDT, WebKit Review Bot
no flags Details
poposed patch (20.90 KB, patch)
2012-08-21 10:23 PDT, Alice Cheng
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-04 (391.15 KB, application/zip)
2012-08-21 13:00 PDT, WebKit Review Bot
no flags Details
proposed patch for buildbot (20.96 KB, patch)
2012-08-21 15:57 PDT, Alice Cheng
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (500.88 KB, application/zip)
2012-08-21 17:58 PDT, WebKit Review Bot
no flags Details
proposed patch for buildbot (21.16 KB, patch)
2012-08-22 10:33 PDT, Alice Cheng
no flags Details | Formatted Diff | Diff
proposed patch (21.36 KB, patch)
2012-08-24 15:52 PDT, Alice Cheng
no flags Details | Formatted Diff | Diff
Latest patch (21.61 KB, patch)
2012-10-10 13:21 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Test (134 bytes, text/html)
2012-10-10 15:22 PDT, Ryosuke Niwa
no flags Details
patch revised (19.60 KB, patch)
2012-10-15 16:22 PDT, Enrica Casucci
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch with compilation switch (23.82 KB, patch)
2012-10-25 15:28 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch4 (29.44 KB, patch)
2012-10-30 16:53 PDT, Enrica Casucci
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch5 (35.31 KB, patch)
2012-10-31 17:12 PDT, Enrica Casucci
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch 6 (35.40 KB, patch)
2012-10-31 17:33 PDT, Enrica Casucci
rniwa: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alice Cheng 2012-07-20 17:26:22 PDT
Extend webkit-user-select property to support unified draggable inline atomic selection for needs of Mail. Here, atomic selection means a visually all-or-nothing selection.

1) added code to correctly parse the new value

2) modified VisibleSelection, VisiblePosition, visible_units, FrameSelection for atomic selection

3) modified DragController, EventHandler to improve the dragging effect

4) added test to test above three modifications
Comment 1 Alice Cheng 2012-07-20 17:26:42 PDT
<rdar://problem/10161404>
Comment 2 Alice Cheng 2012-07-20 17:41:19 PDT
Created attachment 153631 [details]
Prototype

This is a prototype of atomic value for -webkit-user-select property

The patch also includes a test for the selection and dragging.

However, there is still a lot of improvements needed: a space text node padding (for cursor to move right before and after the atomic in-line area), ReplaceSelection special treatment for atomic divs, and so on.
Comment 3 WebKit Review Bot 2012-07-20 17:44:02 PDT
Attachment 153631 [details] did not pass style-queue:

Source/WebCore/css/CSSPrimitiveValueMappings.h:2351:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/editing/ReplaceSelectionCommand.cpp:1072:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/editing/ReplaceSelectionCommand.cpp:1073:  Missing spaces around ==  [whitespace/operators] [3]
Source/WebCore/editing/ReplaceSelectionCommand.cpp:1073:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/page/DragController.cpp:641:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/page/DragController.cpp:772:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/page/DragController.cpp:772:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/page/DragController.cpp:788:  Missing spaces around ||  [whitespace/operators] [3]
Source/WebCore/rendering/style/StyleRareInheritedData.h:89:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/editing/VisibleSelection.cpp:518:  Missing spaces around |  [whitespace/operators] [3]
Source/WebCore/editing/VisibleSelection.cpp:518:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/VisibleSelection.cpp:523:  Missing space before ( in while(  [whitespace/parens] [5]
Source/WebCore/editing/VisibleSelection.cpp:523:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/editing/VisibleSelection.cpp:527:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/VisibleSelection.cpp:527:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/editing/VisibleSelection.cpp:540:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/editing/VisibleSelection.cpp:549:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/editing/VisibleSelection.cpp:551:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/editing/VisibleSelection.cpp:555:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/editing/VisibleSelection.cpp:557:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/editing/VisibleSelection.cpp:561:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/editing/VisibleSelection.cpp:562:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/VisibleSelection.cpp:565:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/VisibleSelection.h:119:  The parameter name "selection" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/editing/VisibleSelection.h:127:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/editing/FrameSelection.cpp:1845:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/FrameSelection.cpp:1847:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/visible_units.cpp:984:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/editing/visible_units.cpp:987:  Missing spaces around ==  [whitespace/operators] [3]
Source/WebCore/editing/visible_units.cpp:989:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/editing/visible_units.cpp:991:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/visible_units.cpp:991:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/editing/visible_units.cpp:1058:  Missing spaces around ==  [whitespace/operators] [3]
Source/WebCore/editing/visible_units.cpp:1060:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/editing/visible_units.cpp:1062:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/visible_units.cpp:1062:  An else statement can be removed when the prior "if" concFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/editing/selection/icon.png', u..." exit_code: 1
ludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/editing/VisiblePosition.cpp:65:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/editing/VisiblePosition.cpp:68:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/VisiblePosition.cpp:76:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/VisiblePosition.cpp:84:  Missing spaces around &&  [whitespace/operators] [3]
Source/WebCore/editing/VisiblePosition.cpp:84:  Missing space before ( in while(  [whitespace/parens] [5]
Source/WebCore/editing/VisiblePosition.cpp:86:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/editing/VisiblePosition.cpp:87:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/VisiblePosition.cpp:95:  Missing space before ( in while(  [whitespace/parens] [5]
Source/WebCore/editing/VisiblePosition.cpp:97:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/editing/VisiblePosition.cpp:98:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/VisiblePosition.cpp:106:  Missing spaces around &&  [whitespace/operators] [3]
Source/WebCore/editing/VisiblePosition.cpp:106:  Missing space before ( in while(  [whitespace/parens] [5]
Source/WebCore/editing/VisiblePosition.cpp:108:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/editing/VisiblePosition.cpp:109:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/editing/VisiblePosition.cpp:117:  Missing spaces around &&  [whitespace/operators] [3]
Source/WebCore/editing/VisiblePosition.cpp:117:  Missing space before ( in while(  [whitespace/parens] [5]
Source/WebCore/editing/VisiblePosition.cpp:119:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/editing/VisiblePosition.cpp:120:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/page/EventHandler.cpp:1299:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/page/EventHandler.cpp:1300:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/page/EventHandler.cpp:1300:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 57 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Gyuyoung Kim 2012-07-20 17:47:36 PDT
Comment on attachment 153631 [details]
Prototype

Attachment 153631 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13303533
Comment 5 Early Warning System Bot 2012-07-20 17:47:51 PDT
Comment on attachment 153631 [details]
Prototype

Attachment 153631 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13309421
Comment 6 Early Warning System Bot 2012-07-20 18:03:44 PDT
Comment on attachment 153631 [details]
Prototype

Attachment 153631 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13307444
Comment 7 Gustavo Noronha (kov) 2012-07-20 18:03:55 PDT
Comment on attachment 153631 [details]
Prototype

Attachment 153631 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13316395
Comment 8 WebKit Review Bot 2012-07-20 18:06:37 PDT
Comment on attachment 153631 [details]
Prototype

Attachment 153631 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13314419
Comment 9 Ryosuke Niwa 2012-07-20 22:57:49 PDT
Comment on attachment 153631 [details]
Prototype

First off, this patch needs a change log. Second, you need to announce this feature on webkit-dev since this will be exposed to the Web. Third, what is the use case of this feature? Could you give me an example of how atomic can be used? Lastly, this sounds a like like control selection: https://bugs.webkit.org/show_bug.cgi?id=12250
Comment 10 Ryosuke Niwa 2012-07-20 23:00:11 PDT
Looking at https://developer.mozilla.org/en/CSS/-moz-user-select, isn't "all" what we want here?
Comment 11 Ryosuke Niwa 2012-07-20 23:08:07 PDT
e.g. try <html><head></head><body contenteditable="">a<div style="-moz-user-select: all;">hello, world</div>baa</body></html> on Firefox.
Comment 12 Alice Cheng 2012-07-21 09:01:29 PDT
Created attachment 153671 [details]
prototype

Sorry about missing the ChangeLog for the previous post.

Sorry if I did not explain well the atomic feature: Rather than just a all-or-nothing selection, this feature also focuses on: dragging of any sub element in the area will result in correct atomic selection, as well as correct copy and paste behaviors for dropping of the atomic element. A use case would be attachments for Mail

uploading a new patch with ChangeLog
Comment 13 WebKit Review Bot 2012-07-21 09:03:45 PDT
Attachment 153671 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1
Source/WebCore/css/CSSPrimitiveValueMappings.h:2351:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/editing/ReplaceSelectionCommand.cpp:1073:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/page/DragController.cpp:788:  Missing spaces around ||  [whitespace/operators] [3]
Source/WebCore/rendering/style/StyleRareInheritedData.h:89:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/editing/VisibleSelection.cpp:523:  Missing space before ( in while(  [whitespace/parens] [5]
Source/WebCore/editing/VisibleSelection.cpp:527:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/editing/VisibleSelection.h:119:  The parameter name "selection" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/editing/visible_units.cpp:984:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/editing/visible_units.cpp:989:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/editing/visible_units.cpp:991:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/editing/visible_units.cpp:1060:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/editing/visible_units.cpp:1062:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/editing/VisiblePosition.cpp:65:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/page/EventHandler.cpp:1299:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/page/EventHandler.cpp:1300:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/page/EventHandler.cpp:1300:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 16 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Early Warning System Bot 2012-07-21 09:14:54 PDT
Comment on attachment 153671 [details]
prototype

Attachment 153671 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13316547
Comment 15 WebKit Review Bot 2012-07-21 09:24:55 PDT
Comment on attachment 153671 [details]
prototype

Attachment 153671 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13312571
Comment 16 Early Warning System Bot 2012-07-21 09:25:20 PDT
Comment on attachment 153671 [details]
prototype

Attachment 153671 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13316548
Comment 17 Gyuyoung Kim 2012-07-21 09:29:37 PDT
Comment on attachment 153671 [details]
prototype

Attachment 153671 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13318448
Comment 18 Simon Fraser (smfr) 2012-07-21 14:03:17 PDT
This does look the same as the 'all' value. We could change it, but I somewhat prefer 'atomic'.
Comment 19 Ryosuke Niwa 2012-07-21 14:21:54 PDT
(In reply to comment #18)
> This does look the same as the 'all' value. We could change it, but I somewhat prefer 'atomic'.

I think we should match Gecko unless there is a compelling reason not to.
Comment 20 Adele Peterson 2012-07-23 10:40:16 PDT
Comment on attachment 153671 [details]
prototype

This is not ready for review.  First, a discussion needs to take place on webkit-dev.  Second, this patch needs to be broken down into smaller patches.
Comment 21 Ehsan Akhgari [:ehsan] 2012-07-23 10:56:35 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > This does look the same as the 'all' value. We could change it, but I somewhat prefer 'atomic'.
> 
> I think we should match Gecko unless there is a compelling reason not to.

Are we just talking about the naming here?  It's not immediately clear to me what the semantics of the implemented change is by just looking at the patch.
Comment 22 Alice Cheng 2012-08-07 15:40:42 PDT
Here is a summary of the www-style discussion on the new value: we agree to use value "all", since Mozilla will converge their behavior for the new value.
Comment 23 Alice Cheng 2012-08-07 15:45:17 PDT
Created attachment 157026 [details]
part1

Parse the new "all" value for -webkit-user-select
Comment 24 WebKit Review Bot 2012-08-07 15:48:27 PDT
Attachment 157026 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1
Source/WebCore/rendering/style/StyleRareInheritedData.h:92:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Ryosuke Niwa 2012-08-07 16:30:04 PDT
Comment on attachment 157026 [details]
part1

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

The code change looks right but we should probably put this on a separate bug.

> Source/WebCore/ChangeLog:3
> +        Part 1 of: Extend -webkit-user-select with a new value "all"

We don't normally attach multiple patches on a single bug. If you're intending to break the patch into small pieces,
then please file a bug that blocks this bug and post a patch there.

> Source/WebCore/ChangeLog:14
> +        (WebCore::isValidKeywordPropertyAndValue): add new value all

Nit: Please capitalize.

> Source/WebCore/rendering/style/StyleRareInheritedData.h:89
> -    unsigned userSelect : 1;  // EUserSelect
> +    unsigned userSelect : 2;  // EUserSelect

Please use single space after before //.

> LayoutTests/editing/selection/user-select-all-parsing-expected.txt:1
> +tests

Where did this text come from?

> LayoutTests/editing/selection/user-select-all-parsing.html:1
> +<html>

Missing DOCTYPE.

> LayoutTests/editing/selection/user-select-all-parsing.html:5
> +    <head>
> +        <style>
> +            .userSelectAll {-webkit-user-select: all;}
> +        </style>

We don't typically indent elements like this.

> LayoutTests/editing/selection/user-select-all-parsing.html:6
> +        <script src="../editing.js"></script>

We don't need editing.js ijn this test.

> LayoutTests/editing/selection/user-select-all-parsing.html:8
> +        <script src="resources/js-test-selection-shared.js"></script>

We don't this js file.

> LayoutTests/editing/selection/user-select-all-parsing.html:9
> +        <title> Test for Parsing new value all for -webkit-user-select </title>

We should be using description() to add a test description.

> LayoutTests/editing/selection/user-select-all-parsing.html:13
> +            this is text of -webkit-user-select all

Did you mean test?
Comment 26 Alice Cheng 2012-08-16 14:11:32 PDT
Created attachment 158896 [details]
proposed patch for part2
Comment 27 WebKit Review Bot 2012-08-16 14:13:23 PDT
Attachment 158896 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1
Source/WebCore/page/EventHandler.cpp:829:  Extra space before ) in if  [whitespace/parens] [5]
Source/WebCore/page/EventHandler.cpp:836:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Alice Cheng 2012-08-16 14:16:54 PDT
Created attachment 158900 [details]
proposed patch for stylebot
Comment 29 Ryosuke Niwa 2012-08-16 14:43:43 PDT
Comment on attachment 158896 [details]
proposed patch for part2

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

> Source/WebCore/dom/Position.cpp:874
> +    return Position::nodeIsUserSelectAll(node) && node->parentNode() && !Position::nodeIsUserSelectAll(node->parentNode());

No need to qualify nodeIsUserSelectAll.
Also, we need a test for this code.

> Source/WebCore/dom/Position.cpp:886
> +    while (parent && Position::nodeIsUserSelectAll(parent)) {

Ditto about Position:: and needing a test.

> Source/WebCore/page/EventHandler.cpp:442
>          updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection, WordGranularity);

Why don't we call this in updateSelectionForMouseDownDispatchingSelectStart instead of repeating it everywhere?

> LayoutTests/editing/selection/user-select-all-selection-expected.txt:1
> +Test -webkit-user-select alllluser select all area

Nit: alllluser.

> LayoutTests/editing/selection/user-select-all-selection-expected.txt:8
> +PASS Selection is [anchorNode: [object HTMLBodyElement](null) anchorOffset: 1 focusNode: [object HTMLBodyElement](null) focusOffset: 2 isCollapsed: false]

This output doesn't really tell us what we're testing and why it's passing.

> LayoutTests/editing/selection/user-select-all-selection.html:13
> +                execSetSelectionCommand(body.firstChild, 30, body.firstChild, 30 );
> +                execExtendSelectionForwardByCharacterCommand();

You can put this in evalAndLog so that people can see what you're doing in the expected result.

> LayoutTests/editing/selection/user-select-all-selection.html:58
> +                eventSender.mouseMoveTo(clickTarget.offsetLeft + 200, clickTarget.offsetTop + 10);
> +                eventSender.mouseUp();
> +                assertSelectionAt(body, 1, body.firstChild.nextSibling.nextSibling, 13);

This test depends on font metrics. You need to use Ahem font and specify the font size by pixel or otherwise it'll fail on non-Mac ports.
Alternatively, you can figure out the appropriate width dynamically.
Comment 30 Alice Cheng 2012-08-16 15:17:11 PDT
Comment on attachment 158896 [details]
proposed patch for part2

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

Thanks Ryosuke for your review, and I have my questions.

>> Source/WebCore/dom/Position.cpp:886
>> +    while (parent && Position::nodeIsUserSelectAll(parent)) {
> 
> Ditto about Position:: and needing a test.

I think it is tested in LayoutTests/editing/selection/user-select-all-selection.html, but I do need to modify that html to be more readable. Or is there an additional test needed?

>> Source/WebCore/page/EventHandler.cpp:442
>>          updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection, WordGranularity);
> 
> Why don't we call this in updateSelectionForMouseDownDispatchingSelectStart instead of repeating it everywhere?

Because the VisibleSelection in updateSelectionForMouseDownDispatchingSelectStart is passed in as a constant reference, and therefore I cannot change it there. Do you want me to remove the const and call it there?
Comment 31 Ryosuke Niwa 2012-08-16 15:19:01 PDT
(In reply to comment #30)
> (From update of attachment 158896 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158896&action=review
> 
> Thanks Ryosuke for your review, and I have my questions.
> 
> >> Source/WebCore/dom/Position.cpp:886
> >> +    while (parent && Position::nodeIsUserSelectAll(parent)) {
> > 
> > Ditto about Position:: and needing a test.
> 
> I think it is tested in LayoutTests/editing/selection/user-select-all-selection.html, but I do need to modify that html to be more readable. Or is there an additional test needed?

We don't have test cases where some of ancestors already have -webkit-user-select: all.

> >> Source/WebCore/page/EventHandler.cpp:442
> >>          updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection, WordGranularity);
> > 
> > Why don't we call this in updateSelectionForMouseDownDispatchingSelectStart instead of repeating it everywhere?
> 
> Because the VisibleSelection in updateSelectionForMouseDownDispatchingSelectStart is passed in as a constant reference, and therefore I cannot change it there. Do you want me to remove the const and call it there?

Why do you need to modify newSelection? You can just create a local copy and modify that instead.
Comment 32 Alice Cheng 2012-08-16 15:26:11 PDT
Comment on attachment 158896 [details]
proposed patch for part2

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

Thanks for your reply, Ryosuke, more questions:

>>>> Source/WebCore/dom/Position.cpp:886
>>>> +    while (parent && Position::nodeIsUserSelectAll(parent)) {
>>> 
>>> Ditto about Position:: and needing a test.
>> 
>> I think it is tested in LayoutTests/editing/selection/user-select-all-selection.html, but I do need to modify that html to be more readable. Or is there an additional test needed?
> 
> We don't have test cases where some of ancestors already have -webkit-user-select: all.

<span class="userSelectAll" id ="allArea"> is the ancestor of the text nodes, anchor node, and br node (user select all area<br><a href="#">user select all area</a>). Doest that count?
My mouse test (click) tests on clicking on text node of the span, which has ancestor span node.

>>>> Source/WebCore/page/EventHandler.cpp:442
>>>>          updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection, WordGranularity);
>>> 
>>> Why don't we call this in updateSelectionForMouseDownDispatchingSelectStart instead of repeating it everywhere?
>> 
>> Because the VisibleSelection in updateSelectionForMouseDownDispatchingSelectStart is passed in as a constant reference, and therefore I cannot change it there. Do you want me to remove the const and call it there?
> 
> Why do you need to modify newSelection? You can just create a local copy and modify that instead.

sounds good. will fix.
Comment 33 Ryosuke Niwa 2012-08-16 15:28:39 PDT
(In reply to comment #32)
> (From update of attachment 158896 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158896&action=review
> 
> Thanks for your reply, Ryosuke, more questions:
> 
> >>>> Source/WebCore/dom/Position.cpp:886
> >>>> +    while (parent && Position::nodeIsUserSelectAll(parent)) {
> >>> 
> >>> Ditto about Position:: and needing a test.
> >> 
> >> I think it is tested in LayoutTests/editing/selection/user-select-all-selection.html, but I do need to modify that html to be more readable. Or is there an additional test needed?
> > 
> > We don't have test cases where some of ancestors already have -webkit-user-select: all.
> 
> <span class="userSelectAll" id ="allArea"> is the ancestor of the text nodes, anchor node, and br node (user select all area<br><a href="#">user select all area</a>). Doest that count?

No, the case I'm thinking of is with
<span style="-webkit-user-select: all;">hello <span style="-webkit-user-select: all;">world</span> world</span>
and you click somewhere in "world".
Comment 34 Alice Cheng 2012-08-16 15:34:26 PDT
Comment on attachment 158896 [details]
proposed patch for part2

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

Thanks for the clarification! will add that to the test.

>>>>>> Source/WebCore/dom/Position.cpp:886
>>>>>> +    while (parent && Position::nodeIsUserSelectAll(parent)) {
>>>>> 
>>>>> Ditto about Position:: and needing a test.
>>>> 
>>>> I think it is tested in LayoutTests/editing/selection/user-select-all-selection.html, but I do need to modify that html to be more readable. Or is there an additional test needed?
>>> 
>>> We don't have test cases where some of ancestors already have -webkit-user-select: all.
>> 
>> <span class="userSelectAll" id ="allArea"> is the ancestor of the text nodes, anchor node, and br node (user select all area<br><a href="#">user select all area</a>). Doest that count?
>> My mouse test (click) tests on clicking on text node of the span, which has ancestor span node.
> 
> No, the case I'm thinking of is with
> <span style="-webkit-user-select: all;">hello <span style="-webkit-user-select: all;">world</span> world</span>
> and you click somewhere in "world".

I see, you want it more nested. Will add it into the test. Though inner span will inherit user-select-all from the outer span, so I do not think we need that additional style attribute in the inner span.
Comment 35 Ryosuke Niwa 2012-08-16 15:38:21 PDT
(In reply to comment #34)
> (From update of attachment 158896 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158896&action=review
>
> > No, the case I'm thinking of is with
> > <span style="-webkit-user-select: all;">hello <span style="-webkit-user-select: all;">world</span> world</span>
> > and you click somewhere in "world".
> 
> I see, you want it more nested. Will add it into the test. Though inner span will inherit user-select-all from the outer span, so I do not think we need that additional style attribute in the inner span.

Oh, I guess that's true. But we should definitely have a test for other opposite cases where different values of -webkit-user-select are specified in ancestors. e.g.
<span style="-webkit-user-select: all;"> 1 <span style="-webkit-user-select: none;">hello <span style="-webkit-user-select: all;">world</span> 2 </span>
Comment 36 WebKit Review Bot 2012-08-16 15:38:30 PDT
Comment on attachment 158900 [details]
proposed patch for stylebot

Attachment 158900 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13510771

New failing tests:
editing/selection/user-select-all-selection.html
Comment 37 WebKit Review Bot 2012-08-16 15:38:36 PDT
Created attachment 158923 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 38 Alice Cheng 2012-08-17 12:14:19 PDT
Created attachment 159176 [details]
proposed patch

Fixed Ryosuke's review comments

Added more info to test results, added a descendent test
Comment 39 WebKit Review Bot 2012-08-17 13:28:05 PDT
Comment on attachment 159176 [details]
proposed patch

Attachment 159176 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13517859

New failing tests:
http/tests/cache/post-redirect-get.php
editing/selection/user-select-all-selection.html
http/tests/cache/post-with-cached-subresources.php
Comment 40 WebKit Review Bot 2012-08-17 13:28:53 PDT
Created attachment 159190 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 41 Ryosuke Niwa 2012-08-17 14:34:22 PDT
Comment on attachment 159176 [details]
proposed patch

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

> Source/WebCore/page/EventHandler.cpp:827
> +        // movements is inside user select all area

I don't think this comment adds much value.

> Source/WebCore/page/EventHandler.cpp:828
> +        newSelection.setBase(Position(rootUserSelectAllForMousePressNode, Position::PositionIsBeforeAnchor));

You should just use positionBeforeNode.

> Source/WebCore/page/EventHandler.cpp:829
> +        newSelection.setExtent(Position(rootUserSelectAllForMousePressNode, Position::PositionIsAfterAnchor));

positionAfterNode.

> Source/WebCore/page/EventHandler.cpp:832
> +    } else {
> +        // reset base for user select all when necessary
> +        if (rootUserSelectAllForMousePressNode && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0)

I'd prefer using else if here.

> Source/WebCore/page/EventHandler.cpp:834
> +        newSelection.setExtent(targetPosition);

Don't we need to set this to the beginning of rootUserSelectAllForMousePressNode when the previous if condition is true?

> LayoutTests/editing/selection/user-select-all-selection.html:20
> +                log("extending forward: selection should be the entire user-select-all area, which is (body, 1, body, 2).");

This is redundant since it just repeats the assert. We need to explain why it should be at (body, 1, body, 2).
Comment 42 Alice Cheng 2012-08-17 14:44:33 PDT
Comment on attachment 159176 [details]
proposed patch

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

Thanks Ryosuke for your review, will fix them. I have my questions below. Also, do you have any idea on the failure in cr-linux? I am quite confused with it.

>> Source/WebCore/page/EventHandler.cpp:834
>> +        newSelection.setExtent(targetPosition);
> 
> Don't we need to set this to the beginning of rootUserSelectAllForMousePressNode when the previous if condition is true?

Why? Do you mean setting the base or extent?

>> LayoutTests/editing/selection/user-select-all-selection.html:20
>> +                log("extending forward: selection should be the entire user-select-all area, which is (body, 1, body, 2).");
> 
> This is redundant since it just repeats the assert. We need to explain why it should be at (body, 1, body, 2).

Do you have any suggestion on explaining why it should be at (body, 1, body, 2)? Is it okay to just explain: "selection should be the entire user-select-all area"?
Comment 43 Ryosuke Niwa 2012-08-17 14:50:34 PDT
Comment on attachment 159176 [details]
proposed patch

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

>>> Source/WebCore/page/EventHandler.cpp:834
>>> +        newSelection.setExtent(targetPosition);
>> 
>> Don't we need to set this to the beginning of rootUserSelectAllForMousePressNode when the previous if condition is true?
> 
> Why? Do you mean setting the base or extent?

Extent. If rootUserSelectAllForMousePressNode is not null, then we're still in -webkit-user-select: all, right?
Comment 44 Ryosuke Niwa 2012-08-17 14:57:51 PDT
(In reply to comment #42)
> >> LayoutTests/editing/selection/user-select-all-selection.html:20
> >> +                log("extending forward: selection should be the entire user-select-all area, which is (body, 1, body, 2).");
> > 
> > This is redundant since it just repeats the assert. We need to explain why it should be at (body, 1, body, 2).
> 
> Do you have any suggestion on explaining why it should be at (body, 1, body, 2)? Is it okay to just explain: "selection should be the entire user-select-all area"?

It's still not clear why (body, 1, body, 2) is the entire user-select-all area.
You had defined elementWithUserSelectAll, you can do (elementWithUserSelectAll.parentNode, elementWithUserSelectAll.nodeIndex, elementWithUserSelectAll.parentNode, elementWithUserSelectAll.nodeIndex + 1) instead.
Comment 45 Alice Cheng 2012-08-17 14:58:21 PDT
Comment on attachment 159176 [details]
proposed patch

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

>>>> Source/WebCore/page/EventHandler.cpp:834
>>>> +        newSelection.setExtent(targetPosition);
>>> 
>>> Don't we need to set this to the beginning of rootUserSelectAllForMousePressNode when the previous if condition is true?
>> 
>> Why? Do you mean setting the base or extent?
> 
> Extent. If rootUserSelectAllForMousePressNode is not null, then we're still in -webkit-user-select: all, right?

mousePressNode is in -webkit-user-select: all, but targetPosition is not, because Position::rootUserSelectAllForNode(target) != rootUserSelectAllForMousePressNode. It might be null, might be in some other user-select all.
Comment 46 Alice Cheng 2012-08-17 14:59:43 PDT
(In reply to comment #44)
> (In reply to comment #42)
> > >> LayoutTests/editing/selection/user-select-all-selection.html:20
> > >> +                log("extending forward: selection should be the entire user-select-all area, which is (body, 1, body, 2).");
> > > 
> > > This is redundant since it just repeats the assert. We need to explain why it should be at (body, 1, body, 2).
> > 
> > Do you have any suggestion on explaining why it should be at (body, 1, body, 2)? Is it okay to just explain: "selection should be the entire user-select-all area"?
> 
> It's still not clear why (body, 1, body, 2) is the entire user-select-all area.
> You had defined elementWithUserSelectAll, you can do (elementWithUserSelectAll.parentNode, elementWithUserSelectAll.nodeIndex, elementWithUserSelectAll.parentNode, elementWithUserSelectAll.nodeIndex + 1) instead.

sounds good! will do
Comment 47 Ryosuke Niwa 2012-08-17 15:02:54 PDT
I must say I don't really like the output of assertSelectionAt. It's quite incomprehensible. I would add a new function that works & creates output like below if I were you:

code:
selectionShouldBe("[elementWithUserSelectAll.parentNode, elementWithUserSelectAll.nodeIndex, elementWithUserSelectAll.parentNode, elementWithUserSelectAll.nodeIndex + 1]");

output:
PASS selection was [elementWithUserSelectAll.parentNode, elementWithUserSelectAll.nodeIndex, elementWithUserSelectAll.parentNode, elementWithUserSelectAll.nodeIndex + 1]

You can then do things like:
selectionShouldBe("selectionAround(elementWithUserSelectAll)");

that outputs:
PASS selection was selectionAround(elementWithUserSelectAll)
Comment 48 Alice Cheng 2012-08-20 10:06:00 PDT
Created attachment 159463 [details]
proposed patch

revised test

will write up an email to webkit-dev

will start discussion on a build flag possibly
Comment 49 WebKit Review Bot 2012-08-20 13:39:07 PDT
Comment on attachment 159463 [details]
proposed patch

Attachment 159463 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13544469

New failing tests:
editing/selection/user-select-all-selection.html
Comment 50 WebKit Review Bot 2012-08-20 13:39:13 PDT
Created attachment 159510 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 51 Alice Cheng 2012-08-20 14:13:17 PDT
Created attachment 159516 [details]
proposed patch for buildbot
Comment 52 Ryosuke Niwa 2012-08-20 14:27:42 PDT
Comment on attachment 159516 [details]
proposed patch for buildbot

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

The code change looks great to me. I've pointed out some nits. I apply the patch locally and play around to make sure it works as intended.

> Source/WebCore/dom/Position.cpp:874
> +    return nodeIsUserSelectAll(node) && node->parentNode() && !nodeIsUserSelectAll(node->parentNode());

You should probably check that
nodeIsUserSelectAll(node) && (!node->parentNode() || !nodeIsUserSelectAll(node->parentNode()));

> Source/WebCore/dom/Position.cpp:883
> +    Node* parent = node->parentNode();
> +    if (!parent)
> +        return node;

Then you don't need to special case this.

> Source/WebCore/page/EventHandler.cpp:831
> +        // reset base for user select all when necessary
> +        if (rootUserSelectAllForMousePressNode && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0)

Instead of adding a comment like this, can we extract this long condition as a helper function with a descriptive name?
e.g. shouldResetBaseForUserSelectAll.
By the way, a more useful comment is to describe what "when necessary" exactly is.

> LayoutTests/editing/selection/user-select-all-selection.html:23
> +                    testFailed("Selection should be " + str +
> +                               " at anchorNode: " + sel.anchorNode + " anchorOffset: " + sel.anchorOffset +
> +                               " focusNode: " + sel.focusNode + " focusOffset: " + sel.focusOffset);

Wrong style. + should appear at the beginning of next line, and the text should be indented by exactly 4 spaces to the right of testFailed.

> LayoutTests/editing/selection/user-select-all-selection.html:32
> +                    testSelectionAt(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, "right before user-select-all element");

userSelectAllElement.previousSibling has a child, then textContent's length is different from that of child node count.
On the other hand, it is a text node, then calling firstChild doesn't make sense so something is awfully wrong here.

> LayoutTests/editing/selection/user-select-all-selection.html:38
> +                // test extend forward

These one line comment just repeats what the code tells us. Please remove.

> LayoutTests/editing/selection/user-select-all-selection.html:39
> +                execSetSelectionCommand(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length);

!? The offsets should be userSelectAllElement.previousSibling.firstChild.childNodes.length, right? no?
Also, you can just do:
getSelection().collapse(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.firstChild.childNodes.length);

> LayoutTests/editing/selection/user-select-all-selection.html:40
> +                execExtendSelectionForwardByCharacterCommand();

It would be better if this call was done inside evalAndLog so that the expected result contains the line showing this command is called before the assertion.
Ditto for other test cases.

> LayoutTests/editing/selection/user-select-all-selection.html:76
> +                clickAt(clickTarget.offsetLeft + 10 , clickTarget.offsetTop + 10);

Ditto about calling this in evalAndLog.

> LayoutTests/editing/selection/user-select-all-selection.html:83
> +                eventSender.mouseDown();
> +                eventSender.mouseMoveTo(leftTarget.offsetLeft, leftTarget.offsetTop + 10);
> +                eventSender.mouseUp();

Ditto. You may consider adding a helper function like moveFromTo that takes two coordinate points.

> LayoutTests/editing/selection/user-select-all-selection.html:92
> +                eventSender.mouseMoveTo(clickTarget.offsetLeft + 10, clickTarget.offsetTop + 10);
> +                eventSender.mouseDown();
> +                eventSender.mouseMoveTo(userSelectAllElement.offsetLeft + userSelectAllElement.offsetWidth +rightTarget.offsetWidth, rightTarget.offsetTop + 10);
> +                eventSender.mouseUp();

Ditto.
Comment 53 WebKit Review Bot 2012-08-20 14:32:26 PDT
Comment on attachment 159463 [details]
proposed patch

Attachment 159463 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13535924

New failing tests:
editing/selection/user-select-all-selection.html
Comment 54 WebKit Review Bot 2012-08-20 14:32:37 PDT
Created attachment 159523 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 55 WebKit Review Bot 2012-08-20 17:27:03 PDT
Comment on attachment 159516 [details]
proposed patch for buildbot

Attachment 159516 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13547216

New failing tests:
editing/selection/user-select-all-selection.html
Comment 56 WebKit Review Bot 2012-08-20 17:27:11 PDT
Created attachment 159571 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 57 Alice Cheng 2012-08-20 18:31:07 PDT
Comment on attachment 159516 [details]
proposed patch for buildbot

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

Thanks for your review =)

>> LayoutTests/editing/selection/user-select-all-selection.html:23
>> +                               " focusNode: " + sel.focusNode + " focusOffset: " + sel.focusOffset);
> 
> Wrong style. + should appear at the beginning of next line, and the text should be indented by exactly 4 spaces to the right of testFailed.

fixed

>> LayoutTests/editing/selection/user-select-all-selection.html:32
>> +                    testSelectionAt(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, "right before user-select-all element");
> 
> userSelectAllElement.previousSibling has a child, then textContent's length is different from that of child node count.
> On the other hand, it is a text node, then calling firstChild doesn't make sense so something is awfully wrong here.

fixed: should be userSelectAllElement.previousSibling.firstChild.textContent.length. userSelectAllElement.previousSibling is a <font> container, whose firstChild is a text node.

>> LayoutTests/editing/selection/user-select-all-selection.html:38
>> +                // test extend forward
> 
> These one line comment just repeats what the code tells us. Please remove.

fixed

>> LayoutTests/editing/selection/user-select-all-selection.html:39
>> +                execSetSelectionCommand(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length);
> 
> !? The offsets should be userSelectAllElement.previousSibling.firstChild.childNodes.length, right? no?
> Also, you can just do:
> getSelection().collapse(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.firstChild.childNodes.length);

fixed. offset is  userSelectAllElement.previousSibling.firstChild.textContent.length

>> LayoutTests/editing/selection/user-select-all-selection.html:40
>> +                execExtendSelectionForwardByCharacterCommand();
> 
> It would be better if this call was done inside evalAndLog so that the expected result contains the line showing this command is called before the assertion.
> Ditto for other test cases.

fixed
Comment 58 Alice Cheng 2012-08-21 10:23:54 PDT
Created attachment 159717 [details]
poposed patch

revised test according to previous reviews
Comment 59 WebKit Review Bot 2012-08-21 12:59:55 PDT
Comment on attachment 159717 [details]
poposed patch

Attachment 159717 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13548640

New failing tests:
editing/selection/user-select-all-selection.html
Comment 60 WebKit Review Bot 2012-08-21 13:00:03 PDT
Created attachment 159743 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 61 Alice Cheng 2012-08-21 15:57:44 PDT
Created attachment 159785 [details]
proposed patch for buildbot
Comment 62 WebKit Review Bot 2012-08-21 17:58:04 PDT
Comment on attachment 159785 [details]
proposed patch for buildbot

Attachment 159785 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13567060

New failing tests:
editing/selection/user-select-all-selection.html
Comment 63 WebKit Review Bot 2012-08-21 17:58:09 PDT
Created attachment 159826 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 64 Alice Cheng 2012-08-22 10:33:53 PDT
Created attachment 159961 [details]
proposed patch for buildbot
Comment 65 Ryosuke Niwa 2012-08-23 23:43:44 PDT
Comment on attachment 159961 [details]
proposed patch for buildbot

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

Sorry more nits.

> Source/WebCore/page/EventHandler.cpp:830
> +        // reset base for user select all when base is inside user-select-all area and extent < base.

Nit: Capitalize R.

> LayoutTests/editing/selection/user-select-all-selection-expected.txt:8
> +window.getSelection().modify('extend', 'forward', 'character')
> +PASS Selection is the entire user-select-all element

This looks much better!

> LayoutTests/editing/selection/user-select-all-selection.html:48
> +                

Please remove leading whitespace and ditto for other lines.

> LayoutTests/editing/selection/user-select-all-selection.html:57
> +                execSetSelectionCommand(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length);

Maybe you can create a helper function like placeCaretInsideUserSelectAllElement and call it inside evalAndLog as in:
evalAndLog("placeCaretInsideUserSelectAllElement(); getSelection().modify('extend', 'forward', 'character')");

> LayoutTests/editing/selection/user-select-all-selection.html:61
> +                evalAndLog("window.getSelection().modify('extend', 'backward', 'character')");

Shouldn't we reset the selection using execSetSelectionCommand?

> LayoutTests/editing/selection/user-select-all-selection.html:83
> +                // test click

This comment is probably not that helpful.

> LayoutTests/editing/selection/user-select-all-selection.html:87
> +                clickAt(clickTarget.offsetLeft + 10 , clickTarget.offsetTop + 10);

Maybe we can make this function take an element instead of x & y so that you can do:
evalAndLog("click(document.getElementById('elementInsideUserSelectAll'));");

> LayoutTests/editing/selection/user-select-all-selection.html:88
> +                log("clicking on descendent of user-select-all element");

Then you wouldn't need this description.

> LayoutTests/editing/selection/user-select-all-selection.html:95
> +                mouseMoveFromTo(leftTarget.offsetLeft, leftTarget.offsetTop + 10, clickTarget.offsetLeft + 20 , clickTarget.offsetTop + 10);
> +                log("mouse extending from left to user-select-all element");

Here, you can pass in two x coordinates as in:
evalAndLog("moveMouseInElement(userSelectAllElement.offsetTop + 10, userSelectAllElement.previousSibling.offsetLeft, document.getElementById('elementInsideUserSelectAll').offsetLeft + 10)");

> LayoutTests/editing/selection/user-select-all-selection.html:102
> +                var rightTargetTextLength = rightTarget.firstChild.textContent.length;
> +                mouseMoveFromTo(userSelectAllElement.offsetLeft + userSelectAllElement.offsetWidth +rightTarget.offsetWidth, rightTarget.offsetTop + 10, clickTarget.offsetLeft + 10, clickTarget.offsetTop + 10);

Ditto.

> LayoutTests/editing/selection/user-select-all-selection.html:108
> +<body contentEditable="true" id = "body"><font>Test -webkit-user-select all</font><font class="userSelectAll" id ="allArea"><font id = "descendent">user select all area</font>user select all area</font><font>Test -webkit-user-select all</font>

Please be consistent about spaces around =. In most tests, we omit spaces around =.

> LayoutTests/editing/selection/user-select-all-selection.html:114
> +        <script>
> +            description(" Test -webkit-user-select all selection movements and extensions (left right forward backward) ");
> +            testKeyboard();
> +            testMouse();
> +        </script>

Why do we need to put this in a separate script element?
Comment 66 Alice Cheng 2012-08-24 15:36:00 PDT
Comment on attachment 159961 [details]
proposed patch for buildbot

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

Thanks Ryosuke for your review!

>> Source/WebCore/page/EventHandler.cpp:830
>> +        // reset base for user select all when base is inside user-select-all area and extent < base.
> 
> Nit: Capitalize R.

fixed

>> LayoutTests/editing/selection/user-select-all-selection-expected.txt:8
>> +PASS Selection is the entire user-select-all element
> 
> This looks much better!

Thanks! hope this coming version becomes better too.

>> LayoutTests/editing/selection/user-select-all-selection.html:48
>> +                
> 
> Please remove leading whitespace and ditto for other lines.

fixed

>> LayoutTests/editing/selection/user-select-all-selection.html:57
>> +                execSetSelectionCommand(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length);
> 
> Maybe you can create a helper function like placeCaretInsideUserSelectAllElement and call it inside evalAndLog as in:
> evalAndLog("placeCaretInsideUserSelectAllElement(); getSelection().modify('extend', 'forward', 'character')");

fixed

>> LayoutTests/editing/selection/user-select-all-selection.html:87
>> +                clickAt(clickTarget.offsetLeft + 10 , clickTarget.offsetTop + 10);
> 
> Maybe we can make this function take an element instead of x & y so that you can do:
> evalAndLog("click(document.getElementById('elementInsideUserSelectAll'));");

fixed, not exactly the same, but used evalAndLog

>> LayoutTests/editing/selection/user-select-all-selection.html:95
>> +                log("mouse extending from left to user-select-all element");
> 
> Here, you can pass in two x coordinates as in:
> evalAndLog("moveMouseInElement(userSelectAllElement.offsetTop + 10, userSelectAllElement.previousSibling.offsetLeft, document.getElementById('elementInsideUserSelectAll').offsetLeft + 10)");

fixed, not exactly the same, but logged, and called the same function.

>> LayoutTests/editing/selection/user-select-all-selection.html:108
>> +<body contentEditable="true" id = "body"><font>Test -webkit-user-select all</font><font class="userSelectAll" id ="allArea"><font id = "descendent">user select all area</font>user select all area</font><font>Test -webkit-user-select all</font>
> 
> Please be consistent about spaces around =. In most tests, we omit spaces around =.

fixed

>> LayoutTests/editing/selection/user-select-all-selection.html:114
>> +        </script>
> 
> Why do we need to put this in a separate script element?

tried to move it to <head>, but did not work, because it needs things in <body>?
Comment 67 Alice Cheng 2012-08-24 15:52:33 PDT
Created attachment 160516 [details]
proposed patch

Tried to fix ryosuke's review comments.
Modified firstPositionInNode and lastPositionInNode for assertion hit in Position Constructor
Comment 68 Enrica Casucci 2012-10-10 13:21:43 PDT
Created attachment 168061 [details]
Latest patch

this is the latest version of Alice's patch. I've merged to trunk, updated the ChangeLog and added some small cosmetic change to the test.
AFAICT Alice addressed all the latest comments from rniwa for last August.
Comment 69 Ryosuke Niwa 2012-10-10 15:22:08 PDT
Created attachment 168077 [details]
Test

I've applied the patch locally, but it seems like it doesn't work well when select-all region is inside a contenteditable region.
Take a look at this example. If you click on "world", it highlights the entire line (as opposed to just "world"), and when you move caret across the select-all region, the caret disappears.
Comment 70 Enrica Casucci 2012-10-10 15:30:41 PDT
(In reply to comment #69)
> Created an attachment (id=168077) [details]
> Test
> 
> I've applied the patch locally, but it seems like it doesn't work well when select-all region is inside a contenteditable region.
> Take a look at this example. If you click on "world", it highlights the entire line (as opposed to just "world"), and when you move caret across the select-all region, the caret disappears.

You're right. I will investigate.
Comment 71 Enrica Casucci 2012-10-15 16:22:14 PDT
Created attachment 168804 [details]
patch revised

I decided for a totally different approach.
Comment 72 WebKit Review Bot 2012-10-15 19:25:16 PDT
Comment on attachment 168804 [details]
patch revised

Attachment 168804 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14293914
Comment 73 Enrica Casucci 2012-10-25 15:28:39 PDT
Created attachment 170744 [details]
Patch with compilation switch
Comment 74 Ryosuke Niwa 2012-10-25 15:35:43 PDT
Comment on attachment 170744 [details]
Patch with compilation switch

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

> Source/WTF/wtf/Platform.h:1168
> +#define ENABLE_USERSELECT_ALL   1

We need to add this as a feature flag. See http://trac.webkit.org/changeset/119895 for example.
Comment 75 Enrica Casucci 2012-10-30 16:53:56 PDT
Created attachment 171550 [details]
Patch4

Returns correct information about isContentEditable when called from JS. Fixed bug in keyboard selection when in non editable content.
Comment 76 Ryosuke Niwa 2012-10-30 17:12:57 PDT
Comment on attachment 171550 [details]
Patch4

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

> Source/WebCore/dom/Node.h:381
> +    enum ContentEditableUse {
> +        APIUse,
> +        InternalUse
> +    };

I'm not loving the enum & value names here. How about something like
enum ShouldTreatSelectAllAsEditable {
   TreatSelectAllAsEditable,
   DoNoTreatSelectAllAsEditable,
}

> Source/WebCore/dom/Position.cpp:875
> +#if ENABLE(USERSELECT_ALL)
> +    return node && node->renderer() && node->renderer()->style()->userSelect() == SELECT_ALL;
> +#else
> +    UNUSED_PARAM(node);
> +    return false;
> +#endif

I would prefer putting this if-def in the header file so that we can compiler-time eliminate the member functions altogether on ports/platforms where this feature is not enabled to avoid any perf. hits.

> Source/WebCore/editing/FrameSelection.cpp:599
> +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode());
> +    if (rootUserSelectAll)

You should define Node* inside the if.

> Source/WebCore/editing/FrameSelection.cpp:645
> +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode());
> +    if (rootUserSelectAll)
> +        pos = VisiblePosition((directionOfEnclosingBlock() == LTR) ? positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary) : positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));
> +#endif    

It seems like these code blocks are identical. Can we extract a helper function so that we don't have to duplicate it everywhere?

> Source/WebCore/editing/FrameSelection.cpp:776
> +#if ENABLE(USERSELECT_ALL)
> +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode());
> +    if (rootUserSelectAll)
> +        pos = VisiblePosition((directionOfEnclosingBlock() == LTR) ? positionBeforeNode(rootUserSelectAll).downstream(CanCrossEditingBoundary) : positionAfterNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));
> +#endif

Ditto.

> Source/WebCore/editing/FrameSelection.cpp:825
> +#if ENABLE(USERSELECT_ALL)
> +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode());
> +    if (rootUserSelectAll)
> +        pos = VisiblePosition((directionOfEnclosingBlock() == LTR) ? positionBeforeNode(rootUserSelectAll).downstream(CanCrossEditingBoundary) : positionAfterNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));
> +#endif

Ditto.

> LayoutTests/editing/selection/user-select-all-selection-expected.txt:31
> +

Can we add a test case to set the selection programmatically?
e.g. if we had <html><head></head><body><div>hello, <span style="-moz-user-select: all;">wor<b>l</b>d</span> WebKit</div></body></html>, Mozilla still lets us select just b by running getSelection().selectAllChildren(document.querySelector('b'));
Comment 77 Enrica Casucci 2012-10-30 17:25:43 PDT
(In reply to comment #76)
> I'm not loving the enum & value names here. How about something like
> enum ShouldTreatSelectAllAsEditable {
>    TreatSelectAllAsEditable,
>    DoNoTreatSelectAllAsEditable,
> }
> 
I can change it, I couldn't come up with anything better.

> 
> I would prefer putting this if-def in the header file so that we can compiler-time eliminate the member functions altogether on ports/platforms where this feature is not enabled to avoid any perf. hits.
Ok.

> 
> > Source/WebCore/editing/FrameSelection.cpp:599
> > +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode());
> > +    if (rootUserSelectAll)
> 
> You should define Node* inside the if.
>
Ok.

> > Source/WebCore/editing/FrameSelection.cpp:645
> > +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode());
> > +    if (rootUserSelectAll)
> > +        pos = VisiblePosition((directionOfEnclosingBlock() == LTR) ? positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary) : positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));
> > +#endif    
> 
> It seems like these code blocks are identical. Can we extract a helper function so that we don't have to duplicate it everywhere?
> 
They are not all the same, only 2 and 2. I can add a helper.

> > LayoutTests/editing/selection/user-select-all-selection-expected.txt:31
> > +
> 
> Can we add a test case to set the selection programmatically?
> e.g. if we had <html><head></head><body><div>hello, <span style="-moz-user-select: all;">wor<b>l</b>d</span> WebKit</div></body></html>, Mozilla still lets us select just b by running getSelection().selectAllChildren(document.querySelector('b'));

Sure.
Comment 78 WebKit Review Bot 2012-10-30 21:31:58 PDT
Comment on attachment 171550 [details]
Patch4

Attachment 171550 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14626812

New failing tests:
editing/selection/select-across-readonly-input-5.html
editing/selection/5195166-1.html
editing/selection/last-empty-inline.html
editing/selection/inline-closest-leaf-child.html
editing/selection/select-out-of-floated-contenteditable.html
editing/selection/select-across-readonly-input-2.html
editing/selection/selection-actions.html
editing/selection/paragraph-granularity.html
editing/selection/5232159.html
editing/selection/select-out-of-floated-input.html
editing/selection/extend-after-mouse-selection.html
editing/selection/anchor-focus3.html
editing/selection/drag-select-1.html
editing/shadow/delete-list-in-shadow.html
editing/selection/collapse-selection-in-bidi.html
editing/selection/select-out-of-editable.html
editing/selection/anchor-focus2.html
editing/selection/14971.html
editing/selection/select-bidi-run.html
editing/selection/select-across-readonly-input-3.html
editing/selection/select-out-of-floated-textarea.html
editing/selection/fake-drag.html
editing/selection/5333725.html
editing/selection/drag-select-rapidly.html
editing/selection/select-line-break-with-opposite-directionality.html
editing/selection/select-from-textfield-outwards.html
editing/undo/undo-smart-delete-reversed-selection.html
editing/shadow/compare-positions-in-nested-shadow.html
editing/selection/rtl-move-selection-right-left.html
Comment 79 WebKit Review Bot 2012-10-30 22:28:56 PDT
Comment on attachment 171550 [details]
Patch4

Attachment 171550 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14678029

New failing tests:
editing/selection/select-across-readonly-input-5.html
editing/selection/5195166-1.html
editing/selection/last-empty-inline.html
editing/selection/inline-closest-leaf-child.html
editing/selection/select-out-of-floated-contenteditable.html
editing/selection/select-across-readonly-input-2.html
editing/selection/selection-actions.html
editing/selection/paragraph-granularity.html
editing/selection/5232159.html
editing/selection/select-out-of-floated-input.html
editing/selection/extend-after-mouse-selection.html
editing/selection/anchor-focus3.html
editing/selection/drag-select-1.html
editing/shadow/delete-list-in-shadow.html
editing/selection/collapse-selection-in-bidi.html
editing/selection/select-out-of-editable.html
editing/selection/anchor-focus2.html
editing/selection/14971.html
editing/selection/select-bidi-run.html
editing/selection/select-across-readonly-input-3.html
editing/selection/select-out-of-floated-textarea.html
editing/selection/fake-drag.html
editing/selection/5333725.html
editing/selection/drag-select-rapidly.html
editing/selection/select-line-break-with-opposite-directionality.html
editing/selection/select-from-textfield-outwards.html
editing/undo/undo-smart-delete-reversed-selection.html
editing/shadow/compare-positions-in-nested-shadow.html
editing/selection/rtl-move-selection-right-left.html
Comment 80 Enrica Casucci 2012-10-31 17:12:05 PDT
Created attachment 171744 [details]
Patch5

Addresses comments.
Comment 81 Early Warning System Bot 2012-10-31 17:18:27 PDT
Comment on attachment 171744 [details]
Patch5

Attachment 171744 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14677309
Comment 82 Early Warning System Bot 2012-10-31 17:22:21 PDT
Comment on attachment 171744 [details]
Patch5

Attachment 171744 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14661417
Comment 83 EFL EWS Bot 2012-10-31 17:24:09 PDT
Comment on attachment 171744 [details]
Patch5

Attachment 171744 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14677312
Comment 84 Enrica Casucci 2012-10-31 17:33:43 PDT
Created attachment 171746 [details]
Patch 6

Fixes build issues on non Mac platforms.
Comment 85 Ryosuke Niwa 2012-10-31 17:46:39 PDT
Comment on attachment 171744 [details]
Patch5

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

> Source/WebCore/dom/Node.cpp:753
> +            // Elements with user-select: all style are considered atomic
> +            // therefore non editable.

It seems like this can fit in one line.

> Source/WebCore/dom/Node.cpp:794
> +    return isContentEditable(Node::UserSelectAllIsAlwaysNonEditable);

We’re already in Node. We don’t need to qualify it with Node, right?

> Source/WebCore/dom/Node.cpp:2755
> +    return isContentEditable(Node::UserSelectAllIsAlwaysNonEditable) || hasEventListeners(eventNames().mouseupEvent) || hasEventListeners(eventNames().mousedownEvent) || hasEventListeners(eventNames().clickEvent) || hasEventListeners(eventNames().DOMActivateEvent);

Ditto about Node::.

> Source/WebCore/dom/Position.cpp:881
> +    

Whitespace here.

> Source/WebCore/dom/Position.cpp:883
> +    while (parent && nodeIsUserSelectAll(parent)) {

Don’t we need to skip the display:none nodes? e.g. <div style="-webkit-user-select: all;">hello<span style="display: none;"><br></span>world</div>.
If we started the search from br, we still want to detect the root node to be the div, right?

> Source/WebCore/editing/FrameSelection.cpp:560
> +static void adjustForwardPositionForUserSelectAll(VisiblePosition& pos, bool directionIsLTR)

Should these two functions be combined and take isForward?
We can then do directionIsLTR == isForward ? positionAfter… : positionBefore…;
Or, we can rename the argument to be something like “adjustToPositionAfterNode”.

> Source/WebCore/editing/FrameSelection.cpp:563
> +        pos = VisiblePosition((directionIsLTR) ? positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary) : positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));

No need for parentheses around directionIsLTR.

> Source/WebCore/editing/FrameSelection.cpp:570
> +        pos = VisiblePosition((directionIsLTR) ? positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary) : positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary));

Ditto.

> Source/WebCore/page/EventHandler.cpp:424
> +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(targetNode);
> +    if (rootUserSelectAll) {

We can declare rootUserSelectAll in the if statement.

> Source/WebCore/page/EventHandler.cpp:425
> +        selection.setBase(positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));

Why don’t we call adjustForwardPositionForUserSelectAll here to share more code?

> Source/WebCore/page/EventHandler.cpp:426
> +        selection.setExtent(positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary));

Ditto.

> Source/WebCore/page/EventHandler.cpp:859
> +        newSelection.setBase(positionBeforeNode(rootUserSelectAllForMousePressNode).upstream(CanCrossEditingBoundary));
> +        newSelection.setExtent(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary));

Ditto about re-using adjustForwardPositionForUserSelectAll. Maybe adjustForwardPositionForUserSelectAll should take root user select node as an argument?

> Source/WebCore/page/EventHandler.cpp:862
> +        if (rootUserSelectAllForMousePressNode && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0)

Is target guaranteed to have renderer?

> Source/WebCore/page/EventHandler.cpp:863
> +            newSelection.setBase(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary));

We should probably re-use adjustForwardPositionForUserSelectAll here as well.

> Source/WebCore/page/EventHandler.cpp:866
> +        if (rootUserSelectAllForTarget && m_mousePressNode->renderer() && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0)

Ditto about target having renderer.

> Source/WebCore/page/EventHandler.cpp:869
> +            newSelection.setExtent(positionBeforeNode(rootUserSelectAllForTarget).upstream(CanCrossEditingBoundary));
> +        else if (rootUserSelectAllForTarget && m_mousePressNode->renderer())
> +            newSelection.setExtent(positionAfterNode(rootUserSelectAllForTarget).downstream(CanCrossEditingBoundary));

Ditto about re-using adjustForwardPositionForUserSelectAll.
Comment 86 Ryosuke Niwa 2012-10-31 17:48:32 PDT
Comment on attachment 171746 [details]
Patch 6

r+ provided my previous comments are addressed.
Comment 87 Enrica Casucci 2012-10-31 22:19:54 PDT
(In reply to comment #85)
> (From update of attachment 171744 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171744&action=review
> It seems like this can fit in one line.
We normally mark this comments as nits.
> 
> > Source/WebCore/dom/Node.cpp:794
> > +    return isContentEditable(Node::UserSelectAllIsAlwaysNonEditable);
> 
> We’re already in Node. We don’t need to qualify it with Node, right?
> 
> > Source/WebCore/dom/Node.cpp:2755
> > +    return isContentEditable(Node::UserSelectAllIsAlwaysNonEditable) || hasEventListeners(eventNames().mouseupEvent) || hasEventListeners(eventNames().mousedownEvent) || hasEventListeners(eventNames().clickEvent) || hasEventListeners(eventNames().DOMActivateEvent);
> 
> Ditto about Node::.
> 
> > Source/WebCore/dom/Position.cpp:881
> > +    
> 
> Whitespace here.
> 
> > Source/WebCore/dom/Position.cpp:883
> > +    while (parent && nodeIsUserSelectAll(parent)) {
> 
> Don’t we need to skip the display:none nodes? e.g. <div style="-webkit-user-select: all;">hello<span style="display: none;"><br></span>world</div>.
> If we started the search from br, we still want to detect the root node to be the div, right?
Yes, and why is this not the case? We are looking for the root element with user-select and in your example this code still finds div.

> 
> > Source/WebCore/editing/FrameSelection.cpp:560
> > +static void adjustForwardPositionForUserSelectAll(VisiblePosition& pos, bool directionIsLTR)
> 
> Should these two functions be combined and take isForward?
> We can then do directionIsLTR == isForward ? positionAfter… : positionBefore…;
> Or, we can rename the argument to be something like “adjustToPositionAfterNode”.
No, I don't think we need to do that. The difference is also in the use of downstream and upstream.

> 
> No need for parentheses around directionIsLTR.
Sure...
> 
> > Source/WebCore/page/EventHandler.cpp:424
> > +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(targetNode);
> > +    if (rootUserSelectAll) {
> 
> We can declare rootUserSelectAll in the if statement.
Yes.
> 
> > Source/WebCore/page/EventHandler.cpp:425
> > +        selection.setBase(positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));
> 
> Why don’t we call adjustForwardPositionForUserSelectAll here to share more code?

> 
> > Source/WebCore/page/EventHandler.cpp:426
> > +        selection.setExtent(positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary));
> 
> Ditto.

> 
> > Source/WebCore/page/EventHandler.cpp:859
> > +        newSelection.setBase(positionBeforeNode(rootUserSelectAllForMousePressNode).upstream(CanCrossEditingBoundary));
> > +        newSelection.setExtent(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary));
> 
> Ditto about re-using adjustForwardPositionForUserSelectAll. Maybe adjustForwardPositionForUserSelectAll should take root user select node as an argument?
> 
> > Source/WebCore/page/EventHandler.cpp:862
> > +        if (rootUserSelectAllForMousePressNode && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0)
> 
> Is target guaranteed to have renderer?
> 
> > Source/WebCore/page/EventHandler.cpp:863
> > +            newSelection.setBase(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary));
> 
> We should probably re-use adjustForwardPositionForUserSelectAll here as well.

No, the other functions take into account the text direction that we don't need to take into account here.
I don't believe there is any need to change this.

> 
> > Source/WebCore/page/EventHandler.cpp:866
> > +        if (rootUserSelectAllForTarget && m_mousePressNode->renderer() && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0)
> 
> Ditto about target having renderer.
I don't understand this comment at all.
> 
> > Source/WebCore/page/EventHandler.cpp:869
> > +            newSelection.setExtent(positionBeforeNode(rootUserSelectAllForTarget).upstream(CanCrossEditingBoundary));
> > +        else if (rootUserSelectAllForTarget && m_mousePressNode->renderer())
> > +            newSelection.setExtent(positionAfterNode(rootUserSelectAllForTarget).downstream(CanCrossEditingBoundary));
> 
> Ditto about re-using adjustForwardPositionForUserSelectAll.
Again, I disagree for the reason above.
Comment 88 Ryosuke Niwa 2012-10-31 22:41:11 PDT
(In reply to comment #87)
> (In reply to comment #85)
> > (From update of attachment 171744 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171744&action=review
> >
> > > Source/WebCore/dom/Position.cpp:883
> > > +    while (parent && nodeIsUserSelectAll(parent)) {
> > 
> > Don’t we need to skip the display:none nodes? e.g. <div style="-webkit-user-select: all;">hello<span style="display: none;"><br></span>world</div>.
> > If we started the search from br, we still want to detect the root node to be the div, right?
> Yes, and why is this not the case? We are looking for the root element with user-select and in your example this code still finds div.

Because nodeIsUserSelectAll checks that:
node && node->renderer() && node->renderer()->style()->userSelect() == SELECT_ALL
but neither br nor its parent div has renderer this in this case. So nodeIsUserSelectAll() will return false for those two nodes and stops the loop.

I think we need a loop like:
while (node && !node->renderer())
    node = node->parentNode();
at the very beginning to skip all non-visible nodes first. Once we hit some node with a render object, we can start looking at its style.

>>> Source/WebCore/editing/FrameSelection.cpp:560
>>> +static void adjustForwardPositionForUserSelectAll(VisiblePosition& pos, bool directionIsLTR)
>> 
>> Should these two functions be combined and take isForward?
>> We can then do directionIsLTR == isForward ? positionAfter… : positionBefore…;
>> Or, we can rename the argument to be something like “adjustToPositionAfterNode”.
> 
> No, I don't think we need to do that. The difference is also in the use of downstream and upstream.

But we call downstream whenever we call positionAfterNode and we call upstream whenever we call positionBeforeNode in both of these functions.

>>> Source/WebCore/page/EventHandler.cpp:859
>>> +        newSelection.setExtent(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary));
>> 
>> Ditto about re-using adjustForwardPositionForUserSelectAll. Maybe adjustForwardPositionForUserSelectAll should take root user select node as an argument?
> 
> No, the other functions take into account the text direction that we don't need to take into account here.
> I don't believe there is any need to change this.

If we merged the two functions and just had adjustToPositionAfterNode as the argument, we can call it twice here as in:
newSelection.setBase(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionBeforeNode));
newSelection.setExtent(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionAfterNode));
Comment 89 WebKit Review Bot 2012-10-31 22:49:13 PDT
Comment on attachment 171746 [details]
Patch 6

Attachment 171746 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14697172

New failing tests:
http/tests/security/contentSecurityPolicy/object-src-url-allowed.html
editing/selection/select-across-readonly-input-5.html
editing/selection/5195166-1.html
editing/selection/last-empty-inline.html
editing/selection/inline-closest-leaf-child.html
editing/selection/select-out-of-floated-contenteditable.html
editing/selection/select-across-readonly-input-2.html
editing/selection/selection-actions.html
editing/selection/paragraph-granularity.html
editing/selection/5232159.html
editing/selection/select-out-of-floated-input.html
editing/selection/extend-after-mouse-selection.html
editing/selection/anchor-focus3.html
editing/selection/drag-select-1.html
editing/shadow/delete-list-in-shadow.html
editing/selection/collapse-selection-in-bidi.html
editing/selection/select-out-of-editable.html
editing/selection/anchor-focus2.html
editing/selection/14971.html
editing/selection/select-bidi-run.html
editing/selection/select-across-readonly-input-3.html
editing/selection/select-out-of-floated-textarea.html
editing/selection/fake-drag.html
editing/selection/5333725.html
editing/selection/drag-select-rapidly.html
editing/selection/select-line-break-with-opposite-directionality.html
editing/selection/select-from-textfield-outwards.html
editing/undo/undo-smart-delete-reversed-selection.html
editing/shadow/compare-positions-in-nested-shadow.html
editing/selection/rtl-move-selection-right-left.html
Comment 90 Enrica Casucci 2012-10-31 23:29:27 PDT
(In reply to comment #88)
> (In reply to comment #87)
> > (In reply to comment #85)
> > > (From update of attachment 171744 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=171744&action=review
> > >
> > > > Source/WebCore/dom/Position.cpp:883
> > > > +    while (parent && nodeIsUserSelectAll(parent)) {
> > > 
> > > Don’t we need to skip the display:none nodes? e.g. <div style="-webkit-user-select: all;">hello<span style="display: none;"><br></span>world</div>.
> > > If we started the search from br, we still want to detect the root node to be the div, right?
> > Yes, and why is this not the case? We are looking for the root element with user-select and in your example this code still finds div.
> 
> Because nodeIsUserSelectAll checks that:
> node && node->renderer() && node->renderer()->style()->userSelect() == SELECT_ALL
> but neither br nor its parent div has renderer this in this case. So nodeIsUserSelectAll() will return false for those two nodes and stops the loop.
> 
> I think we need a loop like:
> while (node && !node->renderer())
>     node = node->parentNode();
> at the very beginning to skip all non-visible nodes first. Once we hit some node with a render object, we can start looking at its style.
> 
> >>> Source/WebCore/editing/FrameSelection.cpp:560
> >>> +static void adjustForwardPositionForUserSelectAll(VisiblePosition& pos, bool directionIsLTR)
> >> 
> >> Should these two functions be combined and take isForward?
> >> We can then do directionIsLTR == isForward ? positionAfter… : positionBefore…;
> >> Or, we can rename the argument to be something like “adjustToPositionAfterNode”.
> > 
> > No, I don't think we need to do that. The difference is also in the use of downstream and upstream.
> 
> But we call downstream whenever we call positionAfterNode and we call upstream whenever we call positionBeforeNode in both of these functions.
> 
> >>> Source/WebCore/page/EventHandler.cpp:859
> >>> +        newSelection.setExtent(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary));
> >> 
> >> Ditto about re-using adjustForwardPositionForUserSelectAll. Maybe adjustForwardPositionForUserSelectAll should take root user select node as an argument?
> > 
> > No, the other functions take into account the text direction that we don't need to take into account here.
> > I don't believe there is any need to change this.
> 
> If we merged the two functions and just had adjustToPositionAfterNode as the argument, we can call it twice here as in:
> newSelection.setBase(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionBeforeNode));
> newSelection.setExtent(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionAfterNode));
I will do that to stop all this. I don't believe this is necessary, but we have spent way too much time on this patch already.
Comment 91 Enrica Casucci 2012-10-31 23:30:33 PDT
(In reply to comment #90)
> (In reply to comment #88)
> > (In reply to comment #87)
> > > (In reply to comment #85)
> > > > (From update of attachment 171744 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=171744&action=review
> > > >
> > > > > Source/WebCore/dom/Position.cpp:883
> > > > > +    while (parent && nodeIsUserSelectAll(parent)) {
> > > > 
> > > > Don’t we need to skip the display:none nodes? e.g. <div style="-webkit-user-select: all;">hello<span style="display: none;"><br></span>world</div>.
> > > > If we started the search from br, we still want to detect the root node to be the div, right?
> > > Yes, and why is this not the case? We are looking for the root element with user-select and in your example this code still finds div.
> > 
> > Because nodeIsUserSelectAll checks that:
> > node && node->renderer() && node->renderer()->style()->userSelect() == SELECT_ALL
> > but neither br nor its parent div has renderer this in this case. So nodeIsUserSelectAll() will return false for those two nodes and stops the loop.
> > 
> > I think we need a loop like:
> > while (node && !node->renderer())
> >     node = node->parentNode();
> > at the very beginning to skip all non-visible nodes first. Once we hit some node with a render object, we can start looking at its style.

You are correct about this.
> > 
> > >>> Source/WebCore/editing/FrameSelection.cpp:560
> > >>> +static void adjustForwardPositionForUserSelectAll(VisiblePosition& pos, bool directionIsLTR)
> > >> 
> > >> Should these two functions be combined and take isForward?
> > >> We can then do directionIsLTR == isForward ? positionAfter… : positionBefore…;
> > >> Or, we can rename the argument to be something like “adjustToPositionAfterNode”.
> > > 
> > > No, I don't think we need to do that. The difference is also in the use of downstream and upstream.
> > 
> > But we call downstream whenever we call positionAfterNode and we call upstream whenever we call positionBeforeNode in both of these functions.
> > 
> > >>> Source/WebCore/page/EventHandler.cpp:859
> > >>> +        newSelection.setExtent(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary));
> > >> 
> > >> Ditto about re-using adjustForwardPositionForUserSelectAll. Maybe adjustForwardPositionForUserSelectAll should take root user select node as an argument?
> > > 
> > > No, the other functions take into account the text direction that we don't need to take into account here.
> > > I don't believe there is any need to change this.
> > 
> > If we merged the two functions and just had adjustToPositionAfterNode as the argument, we can call it twice here as in:
> > newSelection.setBase(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionBeforeNode));
> > newSelection.setExtent(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionAfterNode));
> I will do that to stop all this. I don't believe this is necessary, but we have spent way too much time on this patch already.
Comment 92 Ryosuke Niwa 2012-10-31 23:43:04 PDT
> > >>> Source/WebCore/page/EventHandler.cpp:859
> > >>> +        newSelection.setExtent(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary));
> > >> 
> > >> Ditto about re-using adjustForwardPositionForUserSelectAll. Maybe adjustForwardPositionForUserSelectAll should take root user select node as an argument?
> > > 
> > > No, the other functions take into account the text direction that we don't need to take into account here.
> > > I don't believe there is any need to change this.
> > 
> > If we merged the two functions and just had adjustToPositionAfterNode as the argument, we can call it twice here as in:
> > newSelection.setBase(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionBeforeNode));
> > newSelection.setExtent(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionAfterNode));
> I will do that to stop all this. I don't believe this is necessary, but we have spent way too much time on this patch already.

Hehe, sorry about the bikeshedding. You can leave as is if you want. I just thought it's nice to encapsulate positionAfter/BeforeNode(~).downstream/upstream(~) inside some function.
Comment 93 WebKit Review Bot 2012-10-31 23:49:41 PDT
Comment on attachment 171746 [details]
Patch 6

Attachment 171746 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14692363

New failing tests:
http/tests/security/contentSecurityPolicy/object-src-url-allowed.html
editing/selection/select-across-readonly-input-5.html
editing/selection/5195166-1.html
editing/selection/last-empty-inline.html
editing/selection/inline-closest-leaf-child.html
editing/selection/select-out-of-floated-contenteditable.html
editing/selection/select-across-readonly-input-2.html
editing/selection/selection-actions.html
editing/selection/paragraph-granularity.html
editing/selection/5232159.html
editing/selection/select-out-of-floated-input.html
editing/selection/extend-after-mouse-selection.html
editing/selection/anchor-focus3.html
editing/selection/drag-select-1.html
editing/shadow/delete-list-in-shadow.html
editing/selection/collapse-selection-in-bidi.html
editing/selection/select-out-of-editable.html
editing/selection/anchor-focus2.html
editing/selection/14971.html
editing/selection/select-bidi-run.html
editing/selection/select-across-readonly-input-3.html
editing/selection/select-out-of-floated-textarea.html
editing/selection/fake-drag.html
editing/selection/5333725.html
editing/selection/drag-select-rapidly.html
editing/selection/select-line-break-with-opposite-directionality.html
editing/selection/select-from-textfield-outwards.html
editing/undo/undo-smart-delete-reversed-selection.html
editing/shadow/compare-positions-in-nested-shadow.html
editing/selection/rtl-move-selection-right-left.html
Comment 94 Enrica Casucci 2012-11-01 13:55:03 PDT
Committed revision 133224.
Comment 95 Stephen White 2012-11-01 15:38:54 PDT
(In reply to comment #93)
> (From update of attachment 171746 [details])
> Attachment 171746 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14692363
> 
> New failing tests:
> http/tests/security/contentSecurityPolicy/object-src-url-allowed.html
> editing/selection/select-across-readonly-input-5.html
> editing/selection/5195166-1.html
> editing/selection/last-empty-inline.html
> editing/selection/inline-closest-leaf-child.html
> editing/selection/select-out-of-floated-contenteditable.html
> editing/selection/select-across-readonly-input-2.html
> editing/selection/selection-actions.html
> editing/selection/paragraph-granularity.html
> editing/selection/5232159.html
> editing/selection/select-out-of-floated-input.html
> editing/selection/extend-after-mouse-selection.html
> editing/selection/anchor-focus3.html
> editing/selection/drag-select-1.html
> editing/shadow/delete-list-in-shadow.html
> editing/selection/collapse-selection-in-bidi.html
> editing/selection/select-out-of-editable.html
> editing/selection/anchor-focus2.html
> editing/selection/14971.html
> editing/selection/select-bidi-run.html
> editing/selection/select-across-readonly-input-3.html
> editing/selection/select-out-of-floated-textarea.html
> editing/selection/fake-drag.html
> editing/selection/5333725.html
> editing/selection/drag-select-rapidly.html
> editing/selection/select-line-break-with-opposite-directionality.html
> editing/selection/select-from-textfield-outwards.html
> editing/undo/undo-smart-delete-reversed-selection.html
> editing/shadow/compare-positions-in-nested-shadow.html
> editing/selection/rtl-move-selection-right-left.html

These same tests (and about 50 more) failed on Chromium (all platforms) when this was committed.
Comment 96 Ryosuke Niwa 2012-11-01 15:59:33 PDT
Build fix landed in http://trac.webkit.org/changeset/133238.