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
<rdar://problem/10161404>
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.
Attachment 153631 [details] did not pass style-queue: Source/WebCore/css/CSSPrimitiveValueMappings.h:2351: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Source/WebCore/editing/ReplaceSelectionCommand.cpp:1072: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/editing/ReplaceSelectionCommand.cpp:1073: Missing spaces around == [whitespace/operators] [3] Source/WebCore/editing/ReplaceSelectionCommand.cpp:1073: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/page/DragController.cpp:641: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/page/DragController.cpp:772: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/page/DragController.cpp:772: Missing space before { [whitespace/braces] [5] Source/WebCore/page/DragController.cpp:788: Missing spaces around || [whitespace/operators] [3] Source/WebCore/rendering/style/StyleRareInheritedData.h:89: One space before end of line comments [whitespace/comments] [5] Source/WebCore/editing/VisibleSelection.cpp:518: Missing spaces around | [whitespace/operators] [3] Source/WebCore/editing/VisibleSelection.cpp:518: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/VisibleSelection.cpp:523: Missing space before ( in while( [whitespace/parens] [5] Source/WebCore/editing/VisibleSelection.cpp:523: Missing space before { [whitespace/braces] [5] Source/WebCore/editing/VisibleSelection.cpp:527: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/VisibleSelection.cpp:527: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/editing/VisibleSelection.cpp:540: Missing space after , [whitespace/comma] [3] Source/WebCore/editing/VisibleSelection.cpp:549: Missing space after , [whitespace/comma] [3] Source/WebCore/editing/VisibleSelection.cpp:551: Missing space after , [whitespace/comma] [3] Source/WebCore/editing/VisibleSelection.cpp:555: Missing space after , [whitespace/comma] [3] Source/WebCore/editing/VisibleSelection.cpp:557: Missing space after , [whitespace/comma] [3] Source/WebCore/editing/VisibleSelection.cpp:561: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/editing/VisibleSelection.cpp:562: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/VisibleSelection.cpp:565: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/VisibleSelection.h:119: The parameter name "selection" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/editing/VisibleSelection.h:127: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/editing/FrameSelection.cpp:1845: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/FrameSelection.cpp:1847: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/visible_units.cpp:984: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/editing/visible_units.cpp:987: Missing spaces around == [whitespace/operators] [3] Source/WebCore/editing/visible_units.cpp:989: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/editing/visible_units.cpp:991: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/visible_units.cpp:991: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/editing/visible_units.cpp:1058: Missing spaces around == [whitespace/operators] [3] Source/WebCore/editing/visible_units.cpp:1060: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/editing/visible_units.cpp:1062: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/visible_units.cpp:1062: An else statement can be removed when the prior "if" concFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/editing/selection/icon.png', u..." exit_code: 1 ludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/editing/VisiblePosition.cpp:65: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/editing/VisiblePosition.cpp:68: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/VisiblePosition.cpp:76: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/VisiblePosition.cpp:84: Missing spaces around && [whitespace/operators] [3] Source/WebCore/editing/VisiblePosition.cpp:84: Missing space before ( in while( [whitespace/parens] [5] Source/WebCore/editing/VisiblePosition.cpp:86: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/editing/VisiblePosition.cpp:87: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/VisiblePosition.cpp:95: Missing space before ( in while( [whitespace/parens] [5] Source/WebCore/editing/VisiblePosition.cpp:97: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/editing/VisiblePosition.cpp:98: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/VisiblePosition.cpp:106: Missing spaces around && [whitespace/operators] [3] Source/WebCore/editing/VisiblePosition.cpp:106: Missing space before ( in while( [whitespace/parens] [5] Source/WebCore/editing/VisiblePosition.cpp:108: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/editing/VisiblePosition.cpp:109: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/editing/VisiblePosition.cpp:117: Missing spaces around && [whitespace/operators] [3] Source/WebCore/editing/VisiblePosition.cpp:117: Missing space before ( in while( [whitespace/parens] [5] Source/WebCore/editing/VisiblePosition.cpp:119: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/editing/VisiblePosition.cpp:120: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/page/EventHandler.cpp:1299: Missing space before { [whitespace/braces] [5] Source/WebCore/page/EventHandler.cpp:1300: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/page/EventHandler.cpp:1300: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 57 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 153631 [details] Prototype Attachment 153631 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13303533
Comment on attachment 153631 [details] Prototype Attachment 153631 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13309421
Comment on attachment 153631 [details] Prototype Attachment 153631 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13307444
Comment on attachment 153631 [details] Prototype Attachment 153631 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13316395
Comment on attachment 153631 [details] Prototype Attachment 153631 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13314419
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
Looking at https://developer.mozilla.org/en/CSS/-moz-user-select, isn't "all" what we want here?
e.g. try <html><head></head><body contenteditable="">a<div style="-moz-user-select: all;">hello, world</div>baa</body></html> on Firefox.
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
Attachment 153671 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/css/CSSPrimitiveValueMappings.h:2351: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Source/WebCore/editing/ReplaceSelectionCommand.cpp:1073: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/page/DragController.cpp:788: Missing spaces around || [whitespace/operators] [3] Source/WebCore/rendering/style/StyleRareInheritedData.h:89: One space before end of line comments [whitespace/comments] [5] Source/WebCore/editing/VisibleSelection.cpp:523: Missing space before ( in while( [whitespace/parens] [5] Source/WebCore/editing/VisibleSelection.cpp:527: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/editing/VisibleSelection.h:119: The parameter name "selection" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/editing/visible_units.cpp:984: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/editing/visible_units.cpp:989: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/editing/visible_units.cpp:991: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/editing/visible_units.cpp:1060: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/editing/visible_units.cpp:1062: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/editing/VisiblePosition.cpp:65: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/page/EventHandler.cpp:1299: Missing space before { [whitespace/braces] [5] Source/WebCore/page/EventHandler.cpp:1300: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/page/EventHandler.cpp:1300: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 16 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 153671 [details] prototype Attachment 153671 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13316547
Comment on attachment 153671 [details] prototype Attachment 153671 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13312571
Comment on attachment 153671 [details] prototype Attachment 153671 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13316548
Comment on attachment 153671 [details] prototype Attachment 153671 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13318448
This does look the same as the 'all' value. We could change it, but I somewhat prefer 'atomic'.
(In reply to comment #18) > This does look the same as the 'all' value. We could change it, but I somewhat prefer 'atomic'. I think we should match Gecko unless there is a compelling reason not to.
Comment 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.
(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.
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.
Created attachment 157026 [details] part1 Parse the new "all" value for -webkit-user-select
Attachment 157026 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/rendering/style/StyleRareInheritedData.h:92: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 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?
Created attachment 158896 [details] proposed patch for part2
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.
Created attachment 158900 [details] proposed patch for stylebot
Comment on attachment 158896 [details] proposed patch for part2 View in context: https://bugs.webkit.org/attachment.cgi?id=158896&action=review > Source/WebCore/dom/Position.cpp:874 > + return Position::nodeIsUserSelectAll(node) && node->parentNode() && !Position::nodeIsUserSelectAll(node->parentNode()); No need to qualify nodeIsUserSelectAll. Also, we need a test for this code. > Source/WebCore/dom/Position.cpp:886 > + while (parent && Position::nodeIsUserSelectAll(parent)) { Ditto about Position:: and needing a test. > Source/WebCore/page/EventHandler.cpp:442 > updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection, WordGranularity); Why don't we call this in updateSelectionForMouseDownDispatchingSelectStart instead of repeating it everywhere? > LayoutTests/editing/selection/user-select-all-selection-expected.txt:1 > +Test -webkit-user-select alllluser select all area Nit: alllluser. > LayoutTests/editing/selection/user-select-all-selection-expected.txt:8 > +PASS Selection is [anchorNode: [object HTMLBodyElement](null) anchorOffset: 1 focusNode: [object HTMLBodyElement](null) focusOffset: 2 isCollapsed: false] This output doesn't really tell us what we're testing and why it's passing. > LayoutTests/editing/selection/user-select-all-selection.html:13 > + execSetSelectionCommand(body.firstChild, 30, body.firstChild, 30 ); > + execExtendSelectionForwardByCharacterCommand(); You can put this in evalAndLog so that people can see what you're doing in the expected result. > LayoutTests/editing/selection/user-select-all-selection.html:58 > + eventSender.mouseMoveTo(clickTarget.offsetLeft + 200, clickTarget.offsetTop + 10); > + eventSender.mouseUp(); > + assertSelectionAt(body, 1, body.firstChild.nextSibling.nextSibling, 13); This test depends on font metrics. You need to use Ahem font and specify the font size by pixel or otherwise it'll fail on non-Mac ports. Alternatively, you can figure out the appropriate width dynamically.
Comment 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?
(In reply to comment #30) > (From update of attachment 158896 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158896&action=review > > Thanks Ryosuke for your review, and I have my questions. > > >> Source/WebCore/dom/Position.cpp:886 > >> + while (parent && Position::nodeIsUserSelectAll(parent)) { > > > > Ditto about Position:: and needing a test. > > I think it is tested in LayoutTests/editing/selection/user-select-all-selection.html, but I do need to modify that html to be more readable. Or is there an additional test needed? We don't have test cases where some of ancestors already have -webkit-user-select: all. > >> Source/WebCore/page/EventHandler.cpp:442 > >> updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection, WordGranularity); > > > > Why don't we call this in updateSelectionForMouseDownDispatchingSelectStart instead of repeating it everywhere? > > Because the VisibleSelection in updateSelectionForMouseDownDispatchingSelectStart is passed in as a constant reference, and therefore I cannot change it there. Do you want me to remove the const and call it there? Why do you need to modify newSelection? You can just create a local copy and modify that instead.
Comment 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.
(In reply to comment #32) > (From update of attachment 158896 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158896&action=review > > Thanks for your reply, Ryosuke, more questions: > > >>>> Source/WebCore/dom/Position.cpp:886 > >>>> + while (parent && Position::nodeIsUserSelectAll(parent)) { > >>> > >>> Ditto about Position:: and needing a test. > >> > >> I think it is tested in LayoutTests/editing/selection/user-select-all-selection.html, but I do need to modify that html to be more readable. Or is there an additional test needed? > > > > We don't have test cases where some of ancestors already have -webkit-user-select: all. > > <span class="userSelectAll" id ="allArea"> is the ancestor of the text nodes, anchor node, and br node (user select all area<br><a href="#">user select all area</a>). Doest that count? No, the case I'm thinking of is with <span style="-webkit-user-select: all;">hello <span style="-webkit-user-select: all;">world</span> world</span> and you click somewhere in "world".
Comment 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.
(In reply to comment #34) > (From update of attachment 158896 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158896&action=review > > > No, the case I'm thinking of is with > > <span style="-webkit-user-select: all;">hello <span style="-webkit-user-select: all;">world</span> world</span> > > and you click somewhere in "world". > > I see, you want it more nested. Will add it into the test. Though inner span will inherit user-select-all from the outer span, so I do not think we need that additional style attribute in the inner span. Oh, I guess that's true. But we should definitely have a test for other opposite cases where different values of -webkit-user-select are specified in ancestors. e.g. <span style="-webkit-user-select: all;"> 1 <span style="-webkit-user-select: none;">hello <span style="-webkit-user-select: all;">world</span> 2 </span>
Comment 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
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
Created attachment 159176 [details] proposed patch Fixed Ryosuke's review comments Added more info to test results, added a descendent test
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
Created attachment 159190 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 159176 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=159176&action=review > Source/WebCore/page/EventHandler.cpp:827 > + // movements is inside user select all area I don't think this comment adds much value. > Source/WebCore/page/EventHandler.cpp:828 > + newSelection.setBase(Position(rootUserSelectAllForMousePressNode, Position::PositionIsBeforeAnchor)); You should just use positionBeforeNode. > Source/WebCore/page/EventHandler.cpp:829 > + newSelection.setExtent(Position(rootUserSelectAllForMousePressNode, Position::PositionIsAfterAnchor)); positionAfterNode. > Source/WebCore/page/EventHandler.cpp:832 > + } else { > + // reset base for user select all when necessary > + if (rootUserSelectAllForMousePressNode && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0) I'd prefer using else if here. > Source/WebCore/page/EventHandler.cpp:834 > + newSelection.setExtent(targetPosition); Don't we need to set this to the beginning of rootUserSelectAllForMousePressNode when the previous if condition is true? > LayoutTests/editing/selection/user-select-all-selection.html:20 > + log("extending forward: selection should be the entire user-select-all area, which is (body, 1, body, 2)."); This is redundant since it just repeats the assert. We need to explain why it should be at (body, 1, body, 2).
Comment on attachment 159176 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=159176&action=review Thanks Ryosuke for your review, will fix them. I have my questions below. Also, do you have any idea on the failure in cr-linux? I am quite confused with it. >> Source/WebCore/page/EventHandler.cpp:834 >> + newSelection.setExtent(targetPosition); > > Don't we need to set this to the beginning of rootUserSelectAllForMousePressNode when the previous if condition is true? Why? Do you mean setting the base or extent? >> LayoutTests/editing/selection/user-select-all-selection.html:20 >> + log("extending forward: selection should be the entire user-select-all area, which is (body, 1, body, 2)."); > > This is redundant since it just repeats the assert. We need to explain why it should be at (body, 1, body, 2). Do you have any suggestion on explaining why it should be at (body, 1, body, 2)? Is it okay to just explain: "selection should be the entire user-select-all area"?
Comment 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?
(In reply to comment #42) > >> LayoutTests/editing/selection/user-select-all-selection.html:20 > >> + log("extending forward: selection should be the entire user-select-all area, which is (body, 1, body, 2)."); > > > > This is redundant since it just repeats the assert. We need to explain why it should be at (body, 1, body, 2). > > Do you have any suggestion on explaining why it should be at (body, 1, body, 2)? Is it okay to just explain: "selection should be the entire user-select-all area"? It's still not clear why (body, 1, body, 2) is the entire user-select-all area. You had defined elementWithUserSelectAll, you can do (elementWithUserSelectAll.parentNode, elementWithUserSelectAll.nodeIndex, elementWithUserSelectAll.parentNode, elementWithUserSelectAll.nodeIndex + 1) instead.
Comment 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.
(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
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)
Created attachment 159463 [details] proposed patch revised test will write up an email to webkit-dev will start discussion on a build flag possibly
Comment 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
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
Created attachment 159516 [details] proposed patch for buildbot
Comment on attachment 159516 [details] proposed patch for buildbot View in context: https://bugs.webkit.org/attachment.cgi?id=159516&action=review The code change looks great to me. I've pointed out some nits. I apply the patch locally and play around to make sure it works as intended. > Source/WebCore/dom/Position.cpp:874 > + return nodeIsUserSelectAll(node) && node->parentNode() && !nodeIsUserSelectAll(node->parentNode()); You should probably check that nodeIsUserSelectAll(node) && (!node->parentNode() || !nodeIsUserSelectAll(node->parentNode())); > Source/WebCore/dom/Position.cpp:883 > + Node* parent = node->parentNode(); > + if (!parent) > + return node; Then you don't need to special case this. > Source/WebCore/page/EventHandler.cpp:831 > + // reset base for user select all when necessary > + if (rootUserSelectAllForMousePressNode && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0) Instead of adding a comment like this, can we extract this long condition as a helper function with a descriptive name? e.g. shouldResetBaseForUserSelectAll. By the way, a more useful comment is to describe what "when necessary" exactly is. > LayoutTests/editing/selection/user-select-all-selection.html:23 > + testFailed("Selection should be " + str + > + " at anchorNode: " + sel.anchorNode + " anchorOffset: " + sel.anchorOffset + > + " focusNode: " + sel.focusNode + " focusOffset: " + sel.focusOffset); Wrong style. + should appear at the beginning of next line, and the text should be indented by exactly 4 spaces to the right of testFailed. > LayoutTests/editing/selection/user-select-all-selection.html:32 > + testSelectionAt(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, "right before user-select-all element"); userSelectAllElement.previousSibling has a child, then textContent's length is different from that of child node count. On the other hand, it is a text node, then calling firstChild doesn't make sense so something is awfully wrong here. > LayoutTests/editing/selection/user-select-all-selection.html:38 > + // test extend forward These one line comment just repeats what the code tells us. Please remove. > LayoutTests/editing/selection/user-select-all-selection.html:39 > + execSetSelectionCommand(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length); !? The offsets should be userSelectAllElement.previousSibling.firstChild.childNodes.length, right? no? Also, you can just do: getSelection().collapse(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.firstChild.childNodes.length); > LayoutTests/editing/selection/user-select-all-selection.html:40 > + execExtendSelectionForwardByCharacterCommand(); It would be better if this call was done inside evalAndLog so that the expected result contains the line showing this command is called before the assertion. Ditto for other test cases. > LayoutTests/editing/selection/user-select-all-selection.html:76 > + clickAt(clickTarget.offsetLeft + 10 , clickTarget.offsetTop + 10); Ditto about calling this in evalAndLog. > LayoutTests/editing/selection/user-select-all-selection.html:83 > + eventSender.mouseDown(); > + eventSender.mouseMoveTo(leftTarget.offsetLeft, leftTarget.offsetTop + 10); > + eventSender.mouseUp(); Ditto. You may consider adding a helper function like moveFromTo that takes two coordinate points. > LayoutTests/editing/selection/user-select-all-selection.html:92 > + eventSender.mouseMoveTo(clickTarget.offsetLeft + 10, clickTarget.offsetTop + 10); > + eventSender.mouseDown(); > + eventSender.mouseMoveTo(userSelectAllElement.offsetLeft + userSelectAllElement.offsetWidth +rightTarget.offsetWidth, rightTarget.offsetTop + 10); > + eventSender.mouseUp(); Ditto.
Comment 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
Created attachment 159523 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 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
Created attachment 159571 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 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
Created attachment 159717 [details] poposed patch revised test according to previous reviews
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
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
Created attachment 159785 [details] proposed patch for buildbot
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
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
Created attachment 159961 [details] proposed patch for buildbot
Comment on attachment 159961 [details] proposed patch for buildbot View in context: https://bugs.webkit.org/attachment.cgi?id=159961&action=review Sorry more nits. > Source/WebCore/page/EventHandler.cpp:830 > + // reset base for user select all when base is inside user-select-all area and extent < base. Nit: Capitalize R. > LayoutTests/editing/selection/user-select-all-selection-expected.txt:8 > +window.getSelection().modify('extend', 'forward', 'character') > +PASS Selection is the entire user-select-all element This looks much better! > LayoutTests/editing/selection/user-select-all-selection.html:48 > + Please remove leading whitespace and ditto for other lines. > LayoutTests/editing/selection/user-select-all-selection.html:57 > + execSetSelectionCommand(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length); Maybe you can create a helper function like placeCaretInsideUserSelectAllElement and call it inside evalAndLog as in: evalAndLog("placeCaretInsideUserSelectAllElement(); getSelection().modify('extend', 'forward', 'character')"); > LayoutTests/editing/selection/user-select-all-selection.html:61 > + evalAndLog("window.getSelection().modify('extend', 'backward', 'character')"); Shouldn't we reset the selection using execSetSelectionCommand? > LayoutTests/editing/selection/user-select-all-selection.html:83 > + // test click This comment is probably not that helpful. > LayoutTests/editing/selection/user-select-all-selection.html:87 > + clickAt(clickTarget.offsetLeft + 10 , clickTarget.offsetTop + 10); Maybe we can make this function take an element instead of x & y so that you can do: evalAndLog("click(document.getElementById('elementInsideUserSelectAll'));"); > LayoutTests/editing/selection/user-select-all-selection.html:88 > + log("clicking on descendent of user-select-all element"); Then you wouldn't need this description. > LayoutTests/editing/selection/user-select-all-selection.html:95 > + mouseMoveFromTo(leftTarget.offsetLeft, leftTarget.offsetTop + 10, clickTarget.offsetLeft + 20 , clickTarget.offsetTop + 10); > + log("mouse extending from left to user-select-all element"); Here, you can pass in two x coordinates as in: evalAndLog("moveMouseInElement(userSelectAllElement.offsetTop + 10, userSelectAllElement.previousSibling.offsetLeft, document.getElementById('elementInsideUserSelectAll').offsetLeft + 10)"); > LayoutTests/editing/selection/user-select-all-selection.html:102 > + var rightTargetTextLength = rightTarget.firstChild.textContent.length; > + mouseMoveFromTo(userSelectAllElement.offsetLeft + userSelectAllElement.offsetWidth +rightTarget.offsetWidth, rightTarget.offsetTop + 10, clickTarget.offsetLeft + 10, clickTarget.offsetTop + 10); Ditto. > LayoutTests/editing/selection/user-select-all-selection.html:108 > +<body contentEditable="true" id = "body"><font>Test -webkit-user-select all</font><font class="userSelectAll" id ="allArea"><font id = "descendent">user select all area</font>user select all area</font><font>Test -webkit-user-select all</font> Please be consistent about spaces around =. In most tests, we omit spaces around =. > LayoutTests/editing/selection/user-select-all-selection.html:114 > + <script> > + description(" Test -webkit-user-select all selection movements and extensions (left right forward backward) "); > + testKeyboard(); > + testMouse(); > + </script> Why do we need to put this in a separate script element?
Comment 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>?
Created attachment 160516 [details] proposed patch Tried to fix ryosuke's review comments. Modified firstPositionInNode and lastPositionInNode for assertion hit in Position Constructor
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.
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.
(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.
Created attachment 168804 [details] patch revised I decided for a totally different approach.
Comment on attachment 168804 [details] patch revised Attachment 168804 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14293914
Created attachment 170744 [details] Patch with compilation switch
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.
Created attachment 171550 [details] Patch4 Returns correct information about isContentEditable when called from JS. Fixed bug in keyboard selection when in non editable content.
Comment 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'));
(In reply to comment #76) > I'm not loving the enum & value names here. How about something like > enum ShouldTreatSelectAllAsEditable { > TreatSelectAllAsEditable, > DoNoTreatSelectAllAsEditable, > } > I can change it, I couldn't come up with anything better. > > I would prefer putting this if-def in the header file so that we can compiler-time eliminate the member functions altogether on ports/platforms where this feature is not enabled to avoid any perf. hits. Ok. > > > Source/WebCore/editing/FrameSelection.cpp:599 > > + Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode()); > > + if (rootUserSelectAll) > > You should define Node* inside the if. > Ok. > > Source/WebCore/editing/FrameSelection.cpp:645 > > + Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode()); > > + if (rootUserSelectAll) > > + pos = VisiblePosition((directionOfEnclosingBlock() == LTR) ? positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary) : positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary)); > > +#endif > > It seems like these code blocks are identical. Can we extract a helper function so that we don't have to duplicate it everywhere? > They are not all the same, only 2 and 2. I can add a helper. > > LayoutTests/editing/selection/user-select-all-selection-expected.txt:31 > > + > > Can we add a test case to set the selection programmatically? > e.g. if we had <html><head></head><body><div>hello, <span style="-moz-user-select: all;">wor<b>l</b>d</span> WebKit</div></body></html>, Mozilla still lets us select just b by running getSelection().selectAllChildren(document.querySelector('b')); Sure.
Comment on attachment 171550 [details] Patch4 Attachment 171550 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14626812 New failing tests: editing/selection/select-across-readonly-input-5.html editing/selection/5195166-1.html editing/selection/last-empty-inline.html editing/selection/inline-closest-leaf-child.html editing/selection/select-out-of-floated-contenteditable.html editing/selection/select-across-readonly-input-2.html editing/selection/selection-actions.html editing/selection/paragraph-granularity.html editing/selection/5232159.html editing/selection/select-out-of-floated-input.html editing/selection/extend-after-mouse-selection.html editing/selection/anchor-focus3.html editing/selection/drag-select-1.html editing/shadow/delete-list-in-shadow.html editing/selection/collapse-selection-in-bidi.html editing/selection/select-out-of-editable.html editing/selection/anchor-focus2.html editing/selection/14971.html editing/selection/select-bidi-run.html editing/selection/select-across-readonly-input-3.html editing/selection/select-out-of-floated-textarea.html editing/selection/fake-drag.html editing/selection/5333725.html editing/selection/drag-select-rapidly.html editing/selection/select-line-break-with-opposite-directionality.html editing/selection/select-from-textfield-outwards.html editing/undo/undo-smart-delete-reversed-selection.html editing/shadow/compare-positions-in-nested-shadow.html editing/selection/rtl-move-selection-right-left.html
Comment 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
Created attachment 171744 [details] Patch5 Addresses comments.
Comment on attachment 171744 [details] Patch5 Attachment 171744 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14677309
Comment on attachment 171744 [details] Patch5 Attachment 171744 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14661417
Comment on attachment 171744 [details] Patch5 Attachment 171744 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14677312
Created attachment 171746 [details] Patch 6 Fixes build issues on non Mac platforms.
Comment on attachment 171744 [details] Patch5 View in context: https://bugs.webkit.org/attachment.cgi?id=171744&action=review > Source/WebCore/dom/Node.cpp:753 > + // Elements with user-select: all style are considered atomic > + // therefore non editable. It seems like this can fit in one line. > Source/WebCore/dom/Node.cpp:794 > + return isContentEditable(Node::UserSelectAllIsAlwaysNonEditable); We’re already in Node. We don’t need to qualify it with Node, right? > Source/WebCore/dom/Node.cpp:2755 > + return isContentEditable(Node::UserSelectAllIsAlwaysNonEditable) || hasEventListeners(eventNames().mouseupEvent) || hasEventListeners(eventNames().mousedownEvent) || hasEventListeners(eventNames().clickEvent) || hasEventListeners(eventNames().DOMActivateEvent); Ditto about Node::. > Source/WebCore/dom/Position.cpp:881 > + Whitespace here. > Source/WebCore/dom/Position.cpp:883 > + while (parent && nodeIsUserSelectAll(parent)) { Don’t we need to skip the display:none nodes? e.g. <div style="-webkit-user-select: all;">hello<span style="display: none;"><br></span>world</div>. If we started the search from br, we still want to detect the root node to be the div, right? > Source/WebCore/editing/FrameSelection.cpp:560 > +static void adjustForwardPositionForUserSelectAll(VisiblePosition& pos, bool directionIsLTR) Should these two functions be combined and take isForward? We can then do directionIsLTR == isForward ? positionAfter… : positionBefore…; Or, we can rename the argument to be something like “adjustToPositionAfterNode”. > Source/WebCore/editing/FrameSelection.cpp:563 > + pos = VisiblePosition((directionIsLTR) ? positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary) : positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary)); No need for parentheses around directionIsLTR. > Source/WebCore/editing/FrameSelection.cpp:570 > + pos = VisiblePosition((directionIsLTR) ? positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary) : positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary)); Ditto. > Source/WebCore/page/EventHandler.cpp:424 > + Node* rootUserSelectAll = Position::rootUserSelectAllForNode(targetNode); > + if (rootUserSelectAll) { We can declare rootUserSelectAll in the if statement. > Source/WebCore/page/EventHandler.cpp:425 > + selection.setBase(positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary)); Why don’t we call adjustForwardPositionForUserSelectAll here to share more code? > Source/WebCore/page/EventHandler.cpp:426 > + selection.setExtent(positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary)); Ditto. > Source/WebCore/page/EventHandler.cpp:859 > + newSelection.setBase(positionBeforeNode(rootUserSelectAllForMousePressNode).upstream(CanCrossEditingBoundary)); > + newSelection.setExtent(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary)); Ditto about re-using adjustForwardPositionForUserSelectAll. Maybe adjustForwardPositionForUserSelectAll should take root user select node as an argument? > Source/WebCore/page/EventHandler.cpp:862 > + if (rootUserSelectAllForMousePressNode && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0) Is target guaranteed to have renderer? > Source/WebCore/page/EventHandler.cpp:863 > + newSelection.setBase(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary)); We should probably re-use adjustForwardPositionForUserSelectAll here as well. > Source/WebCore/page/EventHandler.cpp:866 > + if (rootUserSelectAllForTarget && m_mousePressNode->renderer() && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0) Ditto about target having renderer. > Source/WebCore/page/EventHandler.cpp:869 > + newSelection.setExtent(positionBeforeNode(rootUserSelectAllForTarget).upstream(CanCrossEditingBoundary)); > + else if (rootUserSelectAllForTarget && m_mousePressNode->renderer()) > + newSelection.setExtent(positionAfterNode(rootUserSelectAllForTarget).downstream(CanCrossEditingBoundary)); Ditto about re-using adjustForwardPositionForUserSelectAll.
Comment on attachment 171746 [details] Patch 6 r+ provided my previous comments are addressed.
(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.
(In reply to comment #87) > (In reply to comment #85) > > (From update of attachment 171744 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=171744&action=review > > > > > Source/WebCore/dom/Position.cpp:883 > > > + while (parent && nodeIsUserSelectAll(parent)) { > > > > Don’t we need to skip the display:none nodes? e.g. <div style="-webkit-user-select: all;">hello<span style="display: none;"><br></span>world</div>. > > If we started the search from br, we still want to detect the root node to be the div, right? > Yes, and why is this not the case? We are looking for the root element with user-select and in your example this code still finds div. Because nodeIsUserSelectAll checks that: node && node->renderer() && node->renderer()->style()->userSelect() == SELECT_ALL but neither br nor its parent div has renderer this in this case. So nodeIsUserSelectAll() will return false for those two nodes and stops the loop. I think we need a loop like: while (node && !node->renderer()) node = node->parentNode(); at the very beginning to skip all non-visible nodes first. Once we hit some node with a render object, we can start looking at its style. >>> Source/WebCore/editing/FrameSelection.cpp:560 >>> +static void adjustForwardPositionForUserSelectAll(VisiblePosition& pos, bool directionIsLTR) >> >> Should these two functions be combined and take isForward? >> We can then do directionIsLTR == isForward ? positionAfter… : positionBefore…; >> Or, we can rename the argument to be something like “adjustToPositionAfterNode”. > > No, I don't think we need to do that. The difference is also in the use of downstream and upstream. But we call downstream whenever we call positionAfterNode and we call upstream whenever we call positionBeforeNode in both of these functions. >>> Source/WebCore/page/EventHandler.cpp:859 >>> + newSelection.setExtent(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary)); >> >> Ditto about re-using adjustForwardPositionForUserSelectAll. Maybe adjustForwardPositionForUserSelectAll should take root user select node as an argument? > > No, the other functions take into account the text direction that we don't need to take into account here. > I don't believe there is any need to change this. If we merged the two functions and just had adjustToPositionAfterNode as the argument, we can call it twice here as in: newSelection.setBase(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionBeforeNode)); newSelection.setExtent(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionAfterNode));
Comment 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
(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.
(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.
> > >>> Source/WebCore/page/EventHandler.cpp:859 > > >>> + newSelection.setExtent(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary)); > > >> > > >> Ditto about re-using adjustForwardPositionForUserSelectAll. Maybe adjustForwardPositionForUserSelectAll should take root user select node as an argument? > > > > > > No, the other functions take into account the text direction that we don't need to take into account here. > > > I don't believe there is any need to change this. > > > > If we merged the two functions and just had adjustToPositionAfterNode as the argument, we can call it twice here as in: > > newSelection.setBase(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionBeforeNode)); > > newSelection.setExtent(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionAfterNode)); > I will do that to stop all this. I don't believe this is necessary, but we have spent way too much time on this patch already. Hehe, sorry about the bikeshedding. You can leave as is if you want. I just thought it's nice to encapsulate positionAfter/BeforeNode(~).downstream/upstream(~) inside some function.
Comment 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
Committed revision 133224.
(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.
Build fix landed in http://trac.webkit.org/changeset/133238.