RESOLVED FIXED 91912
Adding "all" to -webkit-user-select
https://bugs.webkit.org/show_bug.cgi?id=91912
Summary Adding "all" to -webkit-user-select
Alice Cheng
Reported 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
Attachments
Prototype (45.73 KB, patch)
2012-07-20 17:41 PDT, Alice Cheng
rniwa: review-
gyuyoung.kim: commit-queue-
prototype (49.77 KB, patch)
2012-07-21 09:01 PDT, Alice Cheng
adele: review-
part1 (6.35 KB, patch)
2012-08-07 15:45 PDT, Alice Cheng
rniwa: review-
rniwa: commit-queue-
proposed patch for part2 (18.09 KB, patch)
2012-08-16 14:11 PDT, Alice Cheng
no flags
proposed patch for stylebot (18.09 KB, patch)
2012-08-16 14:16 PDT, Alice Cheng
webkit.review.bot: commit-queue-
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
proposed patch (20.35 KB, patch)
2012-08-17 12:14 PDT, Alice Cheng
webkit.review.bot: commit-queue-
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
proposed patch (18.05 KB, patch)
2012-08-20 10:06 PDT, Alice Cheng
webkit.review.bot: commit-queue-
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
proposed patch for buildbot (18.10 KB, patch)
2012-08-20 14:13 PDT, Alice Cheng
webkit.review.bot: commit-queue-
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
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
poposed patch (20.90 KB, patch)
2012-08-21 10:23 PDT, Alice Cheng
webkit.review.bot: commit-queue-
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
proposed patch for buildbot (20.96 KB, patch)
2012-08-21 15:57 PDT, Alice Cheng
webkit.review.bot: commit-queue-
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
proposed patch for buildbot (21.16 KB, patch)
2012-08-22 10:33 PDT, Alice Cheng
no flags
proposed patch (21.36 KB, patch)
2012-08-24 15:52 PDT, Alice Cheng
no flags
Latest patch (21.61 KB, patch)
2012-10-10 13:21 PDT, Enrica Casucci
no flags
Test (134 bytes, text/html)
2012-10-10 15:22 PDT, Ryosuke Niwa
no flags
patch revised (19.60 KB, patch)
2012-10-15 16:22 PDT, Enrica Casucci
webkit.review.bot: commit-queue-
Patch with compilation switch (23.82 KB, patch)
2012-10-25 15:28 PDT, Enrica Casucci
no flags
Patch4 (29.44 KB, patch)
2012-10-30 16:53 PDT, Enrica Casucci
webkit.review.bot: commit-queue-
Patch5 (35.31 KB, patch)
2012-10-31 17:12 PDT, Enrica Casucci
webkit-ews: commit-queue-
Patch 6 (35.40 KB, patch)
2012-10-31 17:33 PDT, Enrica Casucci
rniwa: review+
webkit.review.bot: commit-queue-
Alice Cheng
Comment 1 2012-07-20 17:26:42 PDT
Alice Cheng
Comment 2 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.
WebKit Review Bot
Comment 3 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.
Gyuyoung Kim
Comment 4 2012-07-20 17:47:36 PDT
Early Warning System Bot
Comment 5 2012-07-20 17:47:51 PDT
Early Warning System Bot
Comment 6 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
Gustavo Noronha (kov)
Comment 7 2012-07-20 18:03:55 PDT
WebKit Review Bot
Comment 8 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
Ryosuke Niwa
Comment 9 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
Ryosuke Niwa
Comment 10 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?
Ryosuke Niwa
Comment 11 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.
Alice Cheng
Comment 12 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
WebKit Review Bot
Comment 13 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.
Early Warning System Bot
Comment 14 2012-07-21 09:14:54 PDT
WebKit Review Bot
Comment 15 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
Early Warning System Bot
Comment 16 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
Gyuyoung Kim
Comment 17 2012-07-21 09:29:37 PDT
Simon Fraser (smfr)
Comment 18 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'.
Ryosuke Niwa
Comment 19 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.
Adele Peterson
Comment 20 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.
Ehsan Akhgari [:ehsan]
Comment 21 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.
Alice Cheng
Comment 22 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.
Alice Cheng
Comment 23 2012-08-07 15:45:17 PDT
Created attachment 157026 [details] part1 Parse the new "all" value for -webkit-user-select
WebKit Review Bot
Comment 24 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.
Ryosuke Niwa
Comment 25 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?
Alice Cheng
Comment 26 2012-08-16 14:11:32 PDT
Created attachment 158896 [details] proposed patch for part2
WebKit Review Bot
Comment 27 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.
Alice Cheng
Comment 28 2012-08-16 14:16:54 PDT
Created attachment 158900 [details] proposed patch for stylebot
Ryosuke Niwa
Comment 29 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.
Alice Cheng
Comment 30 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?
Ryosuke Niwa
Comment 31 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.
Alice Cheng
Comment 32 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.
Ryosuke Niwa
Comment 33 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".
Alice Cheng
Comment 34 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.
Ryosuke Niwa
Comment 35 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>
WebKit Review Bot
Comment 36 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
WebKit Review Bot
Comment 37 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
Alice Cheng
Comment 38 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
WebKit Review Bot
Comment 39 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
WebKit Review Bot
Comment 40 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
Ryosuke Niwa
Comment 41 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).
Alice Cheng
Comment 42 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"?
Ryosuke Niwa
Comment 43 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?
Ryosuke Niwa
Comment 44 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.
Alice Cheng
Comment 45 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.
Alice Cheng
Comment 46 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
Ryosuke Niwa
Comment 47 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)
Alice Cheng
Comment 48 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
WebKit Review Bot
Comment 49 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
WebKit Review Bot
Comment 50 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
Alice Cheng
Comment 51 2012-08-20 14:13:17 PDT
Created attachment 159516 [details] proposed patch for buildbot
Ryosuke Niwa
Comment 52 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.
WebKit Review Bot
Comment 53 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
WebKit Review Bot
Comment 54 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
WebKit Review Bot
Comment 55 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
WebKit Review Bot
Comment 56 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
Alice Cheng
Comment 57 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
Alice Cheng
Comment 58 2012-08-21 10:23:54 PDT
Created attachment 159717 [details] poposed patch revised test according to previous reviews
WebKit Review Bot
Comment 59 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
WebKit Review Bot
Comment 60 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
Alice Cheng
Comment 61 2012-08-21 15:57:44 PDT
Created attachment 159785 [details] proposed patch for buildbot
WebKit Review Bot
Comment 62 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
WebKit Review Bot
Comment 63 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
Alice Cheng
Comment 64 2012-08-22 10:33:53 PDT
Created attachment 159961 [details] proposed patch for buildbot
Ryosuke Niwa
Comment 65 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?
Alice Cheng
Comment 66 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>?
Alice Cheng
Comment 67 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
Enrica Casucci
Comment 68 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.
Ryosuke Niwa
Comment 69 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.
Enrica Casucci
Comment 70 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.
Enrica Casucci
Comment 71 2012-10-15 16:22:14 PDT
Created attachment 168804 [details] patch revised I decided for a totally different approach.
WebKit Review Bot
Comment 72 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
Enrica Casucci
Comment 73 2012-10-25 15:28:39 PDT
Created attachment 170744 [details] Patch with compilation switch
Ryosuke Niwa
Comment 74 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.
Enrica Casucci
Comment 75 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.
Ryosuke Niwa
Comment 76 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'));
Enrica Casucci
Comment 77 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.
WebKit Review Bot
Comment 78 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
WebKit Review Bot
Comment 79 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
Enrica Casucci
Comment 80 2012-10-31 17:12:05 PDT
Created attachment 171744 [details] Patch5 Addresses comments.
Early Warning System Bot
Comment 81 2012-10-31 17:18:27 PDT
Early Warning System Bot
Comment 82 2012-10-31 17:22:21 PDT
EFL EWS Bot
Comment 83 2012-10-31 17:24:09 PDT
Enrica Casucci
Comment 84 2012-10-31 17:33:43 PDT
Created attachment 171746 [details] Patch 6 Fixes build issues on non Mac platforms.
Ryosuke Niwa
Comment 85 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.
Ryosuke Niwa
Comment 86 2012-10-31 17:48:32 PDT
Comment on attachment 171746 [details] Patch 6 r+ provided my previous comments are addressed.
Enrica Casucci
Comment 87 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.
Ryosuke Niwa
Comment 88 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));
WebKit Review Bot
Comment 89 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
Enrica Casucci
Comment 90 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.
Enrica Casucci
Comment 91 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.
Ryosuke Niwa
Comment 92 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.
WebKit Review Bot
Comment 93 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
Enrica Casucci
Comment 94 2012-11-01 13:55:03 PDT
Committed revision 133224.
Stephen White
Comment 95 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.
Ryosuke Niwa
Comment 96 2012-11-01 15:59:33 PDT
Note You need to log in before you can comment on or make changes to this bug.