Bug 91912 – Adding "all" to -webkit-user-select

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Alice Cheng]

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

[Alice Cheng]

<rdar://problem/10161404>

[Alice Cheng]

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.

[WebKit Review Bot]

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.

[Gyuyoung Kim]

Comment on attachment 153631 [details]
Prototype

Attachment 153631 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13303533

[Early Warning System Bot]

Comment on attachment 153631 [details]
Prototype

Attachment 153631 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13309421

[Early Warning System Bot]

Comment on attachment 153631 [details]
Prototype

Attachment 153631 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13307444

[Gustavo Noronha (kov)]

Comment on attachment 153631 [details]
Prototype

Attachment 153631 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13316395

[WebKit Review Bot]

Comment on attachment 153631 [details]
Prototype

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Looking at https://developer.mozilla.org/en/CSS/-moz-user-select, isn't "all" what we want here?

[Ryosuke Niwa]

e.g. try <html><head></head><body contenteditable="">a<div style="-moz-user-select: all;">hello, world</div>baa</body></html> on Firefox.

[Alice Cheng]

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

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 153671 [details]
prototype

Attachment 153671 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13316547

[WebKit Review Bot]

Comment on attachment 153671 [details]
prototype

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

[Early Warning System Bot]

Comment on attachment 153671 [details]
prototype

Attachment 153671 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13316548

[Gyuyoung Kim]

Comment on attachment 153671 [details]
prototype

Attachment 153671 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13318448

[Simon Fraser (smfr)]

This does look the same as the 'all' value. We could change it, but I somewhat prefer 'atomic'.

[Ryosuke Niwa]

(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.

[Adele Peterson]

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.

[Ehsan Akhgari [:ehsan]]

(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.

[Alice Cheng]

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.

[Alice Cheng]

Created attachment 157026 [details]
part1

Parse the new "all" value for -webkit-user-select

[WebKit Review Bot]

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.

[Ryosuke Niwa]

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?

[Alice Cheng]

Created attachment 158896 [details]
proposed patch for part2

[WebKit Review Bot]

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.

[Alice Cheng]

Created attachment 158900 [details]
proposed patch for stylebot

[Ryosuke Niwa]

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.

[Alice Cheng]

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?

[Ryosuke Niwa]

(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.

[Alice Cheng]

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.

[Ryosuke Niwa]

(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".

[Alice Cheng]

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.

[Ryosuke Niwa]

(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>

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alice Cheng]

Created attachment 159176 [details]
proposed patch

Fixed Ryosuke's review comments

Added more info to test results, added a descendent test

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ryosuke Niwa]

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).

[Alice Cheng]

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"?

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

(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.

[Alice Cheng]

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.

[Alice Cheng]

(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

[Ryosuke Niwa]

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)

[Alice Cheng]

Created attachment 159463 [details]
proposed patch

revised test

will write up an email to webkit-dev

will start discussion on a build flag possibly

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alice Cheng]

Created attachment 159516 [details]
proposed patch for buildbot

[Ryosuke Niwa]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alice Cheng]

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

[Alice Cheng]

Created attachment 159717 [details]
poposed patch

revised test according to previous reviews

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alice Cheng]

Created attachment 159785 [details]
proposed patch for buildbot

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alice Cheng]

Created attachment 159961 [details]
proposed patch for buildbot

[Ryosuke Niwa]

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?

[Alice Cheng]

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>?

[Alice Cheng]

Created attachment 160516 [details]
proposed patch

Tried to fix ryosuke's review comments.
Modified firstPositionInNode and lastPositionInNode for assertion hit in Position Constructor

[Enrica Casucci]

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.

[Ryosuke Niwa]

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.

[Enrica Casucci]

(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.

[Enrica Casucci]

Created attachment 168804 [details]
patch revised

I decided for a totally different approach.

[WebKit Review Bot]

Comment on attachment 168804 [details]
patch revised

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

[Enrica Casucci]

Created attachment 170744 [details]
Patch with compilation switch

[Ryosuke Niwa]

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.

[Enrica Casucci]

Created attachment 171550 [details]
Patch4

Returns correct information about isContentEditable when called from JS. Fixed bug in keyboard selection when in non editable content.

[Ryosuke Niwa]

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'));

[Enrica Casucci]

(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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Enrica Casucci]

Created attachment 171744 [details]
Patch5

Addresses comments.

[Early Warning System Bot]

Comment on attachment 171744 [details]
Patch5

Attachment 171744 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14677309

[Early Warning System Bot]

Comment on attachment 171744 [details]
Patch5

Attachment 171744 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14661417

[EFL EWS Bot]

Comment on attachment 171744 [details]
Patch5

Attachment 171744 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14677312

[Enrica Casucci]

Created attachment 171746 [details]
Patch 6

Fixes build issues on non Mac platforms.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Comment on attachment 171746 [details]
Patch 6

r+ provided my previous comments are addressed.

[Enrica Casucci]

(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.

[Ryosuke Niwa]

(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));

[WebKit Review Bot]

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

[Enrica Casucci]

(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.

[Enrica Casucci]

(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.

[Ryosuke Niwa]

> > >>> 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.

[WebKit Review Bot]

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

[Enrica Casucci]

Committed revision 133224.

[Stephen White]

(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.

[Ryosuke Niwa]

Build fix landed in http://trac.webkit.org/changeset/133238.