WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
prototype
(49.77 KB, patch)
2012-07-21 09:01 PDT
,
Alice Cheng
adele
: review-
Details
Formatted Diff
Diff
part1
(6.35 KB, patch)
2012-08-07 15:45 PDT
,
Alice Cheng
rniwa
: review-
rniwa
: commit-queue-
Details
Formatted Diff
Diff
proposed patch for part2
(18.09 KB, patch)
2012-08-16 14:11 PDT
,
Alice Cheng
no flags
Details
Formatted Diff
Diff
proposed patch for stylebot
(18.09 KB, patch)
2012-08-16 14:16 PDT
,
Alice Cheng
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-05
(421.07 KB, application/zip)
2012-08-16 15:38 PDT
,
WebKit Review Bot
no flags
Details
proposed patch
(20.35 KB, patch)
2012-08-17 12:14 PDT
,
Alice Cheng
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-02
(313.53 KB, application/zip)
2012-08-17 13:28 PDT
,
WebKit Review Bot
no flags
Details
proposed patch
(18.05 KB, patch)
2012-08-20 10:06 PDT
,
Alice Cheng
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-06
(439.80 KB, application/zip)
2012-08-20 13:39 PDT
,
WebKit Review Bot
no flags
Details
proposed patch for buildbot
(18.10 KB, patch)
2012-08-20 14:13 PDT
,
Alice Cheng
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-05
(325.24 KB, application/zip)
2012-08-20 14:32 PDT
,
WebKit Review Bot
no flags
Details
Archive of layout-test-results from gce-cr-linux-07
(329.80 KB, application/zip)
2012-08-20 17:27 PDT
,
WebKit Review Bot
no flags
Details
poposed patch
(20.90 KB, patch)
2012-08-21 10:23 PDT
,
Alice Cheng
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-04
(391.15 KB, application/zip)
2012-08-21 13:00 PDT
,
WebKit Review Bot
no flags
Details
proposed patch for buildbot
(20.96 KB, patch)
2012-08-21 15:57 PDT
,
Alice Cheng
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-05
(500.88 KB, application/zip)
2012-08-21 17:58 PDT
,
WebKit Review Bot
no flags
Details
proposed patch for buildbot
(21.16 KB, patch)
2012-08-22 10:33 PDT
,
Alice Cheng
no flags
Details
Formatted Diff
Diff
proposed patch
(21.36 KB, patch)
2012-08-24 15:52 PDT
,
Alice Cheng
no flags
Details
Formatted Diff
Diff
Latest patch
(21.61 KB, patch)
2012-10-10 13:21 PDT
,
Enrica Casucci
no flags
Details
Formatted Diff
Diff
Test
(134 bytes, text/html)
2012-10-10 15:22 PDT
,
Ryosuke Niwa
no flags
Details
patch revised
(19.60 KB, patch)
2012-10-15 16:22 PDT
,
Enrica Casucci
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Patch with compilation switch
(23.82 KB, patch)
2012-10-25 15:28 PDT
,
Enrica Casucci
no flags
Details
Formatted Diff
Diff
Patch4
(29.44 KB, patch)
2012-10-30 16:53 PDT
,
Enrica Casucci
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Patch5
(35.31 KB, patch)
2012-10-31 17:12 PDT
,
Enrica Casucci
webkit-ews
: commit-queue-
Details
Formatted Diff
Diff
Patch 6
(35.40 KB, patch)
2012-10-31 17:33 PDT
,
Enrica Casucci
rniwa
: review+
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(24)
View All
Add attachment
proposed patch, testcase, etc.
Alice Cheng
Comment 1
2012-07-20 17:26:42 PDT
<
rdar://problem/10161404
>
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
Comment on
attachment 153631
[details]
Prototype
Attachment 153631
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/13303533
Early Warning System Bot
Comment 5
2012-07-20 17:47:51 PDT
Comment on
attachment 153631
[details]
Prototype
Attachment 153631
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/13309421
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
Comment on
attachment 153631
[details]
Prototype
Attachment 153631
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/13316395
WebKit Review Bot
Comment 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
Comment on
attachment 153671
[details]
prototype
Attachment 153671
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/13316547
WebKit Review Bot
Comment 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
Comment on
attachment 153671
[details]
prototype
Attachment 153671
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/13318448
Simon Fraser (smfr)
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
Comment on
attachment 171744
[details]
Patch5
Attachment 171744
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/14677309
Early Warning System Bot
Comment 82
2012-10-31 17:22:21 PDT
Comment on
attachment 171744
[details]
Patch5
Attachment 171744
[details]
did not pass qt-wk2-ews (qt): Output:
http://queues.webkit.org/results/14661417
EFL EWS Bot
Comment 83
2012-10-31 17:24:09 PDT
Comment on
attachment 171744
[details]
Patch5
Attachment 171744
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/14677312
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
Build fix landed in
http://trac.webkit.org/changeset/133238
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug