New issue
Advanced search Search tips

Issue 692923 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocked on:
issue 748361



Sign in to add a comment

Get rid of m_granularity from VisibleSelection and SelectionTemplate

Project Member Reported by yosin@chromium.org, Feb 16 2017

Issue description

m_granularity is used for mouse double-clicking and triple-click, we should use it in SelectionController and FrameSelection only.
 

Comment 1 by yosin@chromium.org, Feb 16 2017

Labels: -Pri-3 Pri-1
Owner: yosin@chromium.org
Status: Started (was: Available)

Comment 2 by yosin@chromium.org, Feb 16 2017

See also [1], we also think getting rid of granularity from FrameSelection.


[1]  http://crbug.com/427801  Get rid of FrameSelection::granularity()

Comment 3 by yosin@chromium.org, Mar 25 2017

Labels: Type-Task

Comment 4 by yosin@chromium.org, Apr 25 2017

Labels: -Pri-1 Pri-3
Owner: ----
Status: Available (was: Started)
Lower to Pri-3, since this is for code health.

Comment 5 by yosin@chromium.org, Jun 23 2017

VisbileSelection::granularity_ is introduced by patch[1].

[1] http://crrev.com/1648253002: Make VisibleSelection::updateIfNeeded() to consider text granularity
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/385b35c99e76cccd7d7866a9545b6715951b53d1

commit 385b35c99e76cccd7d7866a9545b6715951b53d1
Author: yosin <yosin@chromium.org>
Date: Mon Jun 26 07:25:18 2017

Get rid of *live granularity* behavior from selection

This patch gets rid of *live granularity* unintentionally introduced by the
patch[1] by making |ExpandSelectionToRespectUserSelectAll()| in
|SelectionController| to return expanded selection for improving
interoperability.

The *live granularity* behavior is invoked by double-click or triple-click to
select word/paragraph. Once *live granularity* behavior attached to selection,
selection is automatically expanded in word/paragraph granularity.

Example of live granularity:
 1. On HTML fragment <span>xyz</span>, user does double click at "y".
 2. Selection becomes <span>^xyz|</span>, where "^" is anchor and "|" is focus.
 3. Inserting |Text| node "abc" before |Text| node "xyz" => "abcxyz"
 4. Selection becomes <span>^abcxyz|</span>
 5. Inserting |Text| node "def" after "xyz" => "abcxyzdef"
 6. Selection becomes <span>^abcxyzdef|</span>
If selection doesn't have live granularity behavior, it stays "abc^xyz|def".

This is done by on-demand computation of |VisibleSelection| implemented in
|SelectionEditor| which re-compute selection by using granularity in
|SelectionInDOM| passed by |SelectionCotnroller| as result of user's double-
click or triple-click.

In other words, we propagate granularity property, which is set by double-
click or triple-click from |SelectionCotnroller| to |SelectionEditor| and
we keep it until explicit change of selection other than DOM mutation.

This patch is a follow-up patch of [1].

Following patches will remove granularity member from |VisibleSelectionTemplate|
and |SelectionTemplate|.

[1] http://crrev.com/1648253002: Make VisibleSelection::updateIfNeeded() to
consider text granularity

BUG= 692923 
TEST=run_layout_tests editing/selection/double_click_and_modify.html

Review-Url: https://codereview.chromium.org/2951353003
Cr-Commit-Position: refs/heads/master@{#482209}

[add] https://crrev.com/385b35c99e76cccd7d7866a9545b6715951b53d1/third_party/WebKit/LayoutTests/editing/selection/double_click_and_modify.html
[modify] https://crrev.com/385b35c99e76cccd7d7866a9545b6715951b53d1/third_party/WebKit/Source/core/editing/SelectionController.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/47e7201e24fc08e37f0680f96b21bcbeff37230c

commit 47e7201e24fc08e37f0680f96b21bcbeff37230c
Author: yosin <yosin@chromium.org>
Date: Tue Jun 27 01:30:56 2017

Make UpdateSelectionForMouseDownDispatchingSelectStart() to take SelectionInFlatTree

This patch makes |UpdateSelectionForMouseDownDispatchingSelectStart()| in
|SelectionController| class to take |SelectionInFlatTree| to reduce usage of
|VisibleSelection| for improving code health.

BUG= 692923 
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2950283002
Cr-Commit-Position: refs/heads/master@{#482499}

[modify] https://crrev.com/47e7201e24fc08e37f0680f96b21bcbeff37230c/third_party/WebKit/Source/core/editing/SelectionController.cpp
[modify] https://crrev.com/47e7201e24fc08e37f0680f96b21bcbeff37230c/third_party/WebKit/Source/core/editing/SelectionController.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/09dcc24b3adaa3fa2db4cbef0a8e2d4bcf0f1724

commit 09dcc24b3adaa3fa2db4cbef0a8e2d4bcf0f1724
Author: yosin <yosin@chromium.org>
Date: Wed Jul 05 01:58:00 2017

Get rid of VisibleSelectionTemplate::has_trailing_whitespace_

This patch gets rid of |has_trailing_whitespace_| member variable from
|VisibleSelectionTemplate| class since after the patch[1], this variable
doesn't do nothing for improving code health.

[1] http://crrev.com/2951353003 Get rid of *live granularity* behavior from
selection

BUG= 692923 
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2966433002
Cr-Commit-Position: refs/heads/master@{#484177}

[modify] https://crrev.com/09dcc24b3adaa3fa2db4cbef0a8e2d4bcf0f1724/third_party/WebKit/Source/core/editing/VisibleSelection.cpp
[modify] https://crrev.com/09dcc24b3adaa3fa2db4cbef0a8e2d4bcf0f1724/third_party/WebKit/Source/core/editing/VisibleSelection.h

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/28c838c598c2e513ac94512f828c4ee43af4e5d6

commit 28c838c598c2e513ac94512f828c4ee43af4e5d6
Author: yosin <yosin@chromium.org>
Date: Wed Jul 05 08:37:50 2017

Use FrameSelection::Granularity in TextControlElement::ComputeSelection{Start,End}()

This patch changes |TextControlElement::ComputeSelection{Start,End}()| to use
|FrameSelection::Granularity()| instead of |VisibleSelection::Granularity()|
as a preparation of getting rid of |VisibleSelection::Granularity()|.

BUG= 692923 
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2969183002
Cr-Commit-Position: refs/heads/master@{#484218}

[modify] https://crrev.com/28c838c598c2e513ac94512f828c4ee43af4e5d6/third_party/WebKit/Source/core/html/TextControlElement.cpp

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5166ccf41871457e8922b1a701bd6a2817dc2537

commit 5166ccf41871457e8922b1a701bd6a2817dc2537
Author: yosin <yosin@chromium.org>
Date: Wed Jul 05 10:15:53 2017

Expose Compute{Start,End}RespectingGranularity()

This patch exposes flat tree version of |ComputeStartRespectingGranularity()|
and |ComputeEndRespectingGranularity()| used in |VisibleSelection::Validate()|
as a preparation of getting rid of |granularity_| member variable from
|VisibleSelectionTemplate|.

The patch[1] contains usages of |Compute{Start,End}RespectingGranularity()|
in "SelectionCotroller.cpp".

[1] http://crrev.com/2959153002: Get rid of SelectionTemplate::granularity_

BUG= 692923 
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2967113002
Cr-Commit-Position: refs/heads/master@{#484234}

[modify] https://crrev.com/5166ccf41871457e8922b1a701bd6a2817dc2537/third_party/WebKit/Source/core/editing/VisibleSelection.cpp
[modify] https://crrev.com/5166ccf41871457e8922b1a701bd6a2817dc2537/third_party/WebKit/Source/core/editing/VisibleSelection.h

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9d878d2268ec9db32e6f682c96bfe3cf93a1f4e

commit a9d878d2268ec9db32e6f682c96bfe3cf93a1f4e
Author: yosin <yosin@chromium.org>
Date: Wed Jul 05 11:02:18 2017

Get rid of SelectionTemplate::has_trailing_whitespace_

This patch gets rid of redundant member variable |has_trailing_whitespace_| from
|SelectionTemplate| for improving code health, since the patch[1], nobody sets
this member variable true,

[1] http://crrev.com/2966433002: Get rid of
VisibleSelectionTemplate::has_trailing_whitespace_

BUG= 692923 
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2973583002
Cr-Commit-Position: refs/heads/master@{#484239}

[modify] https://crrev.com/a9d878d2268ec9db32e6f682c96bfe3cf93a1f4e/third_party/WebKit/Source/core/editing/FrameSelection.cpp
[modify] https://crrev.com/a9d878d2268ec9db32e6f682c96bfe3cf93a1f4e/third_party/WebKit/Source/core/editing/SelectionEditor.cpp
[modify] https://crrev.com/a9d878d2268ec9db32e6f682c96bfe3cf93a1f4e/third_party/WebKit/Source/core/editing/SelectionTemplate.cpp
[modify] https://crrev.com/a9d878d2268ec9db32e6f682c96bfe3cf93a1f4e/third_party/WebKit/Source/core/editing/SelectionTemplate.h
[modify] https://crrev.com/a9d878d2268ec9db32e6f682c96bfe3cf93a1f4e/third_party/WebKit/Source/core/editing/SelectionTemplateTest.cpp

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/28f8baaa24a926b54e677c334a867b3dc63bef06

commit 28f8baaa24a926b54e677c334a867b3dc63bef06
Author: yosin <yosin@chromium.org>
Date: Thu Jul 06 02:02:31 2017

Make FrameSelection::MoveRangeSelection() not to set live granularity selection

This patch chagnes |FrameSelection::MoveRangeSelection()| not to set live
granularity selection as similar as the patch[1], which fixes selection created
by double-click.

[1] http://crrev.com/2951353003: Get rid of *live granularity* behavior from
selection

BUG= 692923 
TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.MoveRangeSelectionNoLiveness

Review-Url: https://codereview.chromium.org/2972753002
Cr-Commit-Position: refs/heads/master@{#484443}

[modify] https://crrev.com/28f8baaa24a926b54e677c334a867b3dc63bef06/third_party/WebKit/Source/core/editing/FrameSelection.cpp
[modify] https://crrev.com/28f8baaa24a926b54e677c334a867b3dc63bef06/third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9ad2300eb815732fda127eb344a43a4eff0a073e

commit 9ad2300eb815732fda127eb344a43a4eff0a073e
Author: yosin <yosin@chromium.org>
Date: Thu Jul 06 09:51:53 2017

Introduce CreateVisibleSelectionWithGranularity()

This patch Introduces |CreateVisibleSelectionWithGranularity()| to reduce usages
of |SelectionTemplate::SetGranularity()| for a preparation of getting rid of
|VisibleSelectio::granularity_|[1].

[1] http://crrev.com/959153002: Get rid of SelectionTemplate::granularity_

BUG= 692923 
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2972863003
Cr-Commit-Position: refs/heads/master@{#484518}

[modify] https://crrev.com/9ad2300eb815732fda127eb344a43a4eff0a073e/third_party/WebKit/Source/core/editing/FrameSelection.cpp
[modify] https://crrev.com/9ad2300eb815732fda127eb344a43a4eff0a073e/third_party/WebKit/Source/core/editing/SelectionController.cpp
[modify] https://crrev.com/9ad2300eb815732fda127eb344a43a4eff0a073e/third_party/WebKit/Source/core/editing/VisibleSelection.cpp
[modify] https://crrev.com/9ad2300eb815732fda127eb344a43a4eff0a073e/third_party/WebKit/Source/core/editing/VisibleSelection.h
[modify] https://crrev.com/9ad2300eb815732fda127eb344a43a4eff0a073e/third_party/WebKit/Source/core/editing/VisibleSelectionTest.cpp
[modify] https://crrev.com/9ad2300eb815732fda127eb344a43a4eff0a073e/third_party/WebKit/Source/core/editing/WebSubstringUtil.mm
[modify] https://crrev.com/9ad2300eb815732fda127eb344a43a4eff0a073e/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp

Project Member

Comment 14 by bugdroid1@chromium.org, Jul 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ba4cfa3d3beed403d349394e8569905381a21aa

commit 8ba4cfa3d3beed403d349394e8569905381a21aa
Author: yosin <yosin@chromium.org>
Date: Fri Jul 07 05:12:15 2017

Simplify SelectionController::UpdateSelectionForMouseDrag()

This patch changes |SelectionController::UpdateSelectionForMouseDrag()| to
utilize |AdjustPositionRespectUserSelectAll()| and
|ExtendSelectionAsDirectional()| as what we are doing for Shift+Click to
simplify implementation for improving code health.

Processing of Shift+Click and mouse drag are similar, we do as following:
* Shift-Click:
1. Click to set selection base at clicked position
 selection.collapse(clickedPositon)
2. Shift+Click to extend selection to clicked position
 selection.extend(clickedPoosition)

* Mouse Drag:
1. Mouse left-button down to set selection base at mouse down position.
 selection.collapse(downPosition)
2. Mouse move, holding left-button down, to extend selection to position
where move moved.
 selection.extend(movedPosition)

Step 2 of both are same, extend to position where mouse is.

BUG= 692923 
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2970043002
Cr-Commit-Position: refs/heads/master@{#484836}

[modify] https://crrev.com/8ba4cfa3d3beed403d349394e8569905381a21aa/third_party/WebKit/Source/core/editing/SelectionController.cpp

Project Member

Comment 15 by bugdroid1@chromium.org, Jul 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eadc611f5b2dbdc4aeb42c9092119444042f8521

commit eadc611f5b2dbdc4aeb42c9092119444042f8521
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Mon Jul 10 11:17:15 2017

Change handling of Shift+Click and mouse drag selection extension with granularity

This patch change handling Shift+Click and mouse drag selection extension with
granularity, aka granularity expansion, to use current base position of
selection instead of a position where granularity expansion started, e.g.
double-click or triple-click.


# Background
The granularity expansion is started by double-click, for word, or triple-click,
for paragraph. Once granularity extending started, farther
selection extension is done by granularity, e.g. for word granularity, selection
is extended by word, e.g. "^abc de|f" to "^abc def|" where "^" is base position
and "|" is extend position.

Before this patch, we use base position as a position where user starts
granularity expansion and extent which specified by user for constructing
|VisibleSelection| and hold base/extent as user specified positions and adjust
positions with granularity into start/end positions.

After this patch, we use current base position and granularity adjusted extent
position for constructing |VisibleSelection| and base/extent and start/end
hold extended positions.

Note: start/end are result of calling of |Most{Back,For}wardCaretPosition()| of
base/extent.

# Changes in "shift-click.html"
For Mac, it is handled by |ExtendSelectionAsNonDirectional()|, this patch fixes
the issue[1], see in "shift-click.html" changes for "mac" behavior. When extent
position at end of word, granularity expansion moves extent position to start of
words, e.g. "^foo| bar" => "^foo |bar", in other words, word granularity
expansion selection whitespaces.

For non-Mac, is is handle by |ExtendSelectionAsnDirectional()|, this patch
keeps correct direction of selection.

Here is steps of user actions:
1. "<div id="first">|one <span id="start"></span>two^ three</div>"
  base is after "two"
2. "<div id="first">one |<span id="start"></span>two^ three</div>"
  Shift+Click before "span id=start"
3. "<div id="first">one <span id="start"></span>|two^ three</div>"
  Granularity expansion with visible position canonicalization

Before this patch step 3 is
3. "<div id="first">one <span id="start"></span>^two| three</div>"
  Granularity expansion with visible position canonicalization for
  base/extent are before "two" where double-click is occurred.

Note: This patch is also a preparation of unify base/extent and start/end[2]


[1] https://bugs.webkit.org/show_bug.cgi?id=36256 shift+click to extend a
wordgranularity selection backwards selects the space after
[2]  http://crbug.com/230267  We should get rid of the concept of base and extent

Bug:  692923 ,  230267 
Change-Id: Icbb04e4c19637f4a1481891c4d3acdbc885424bc
Reviewed-on: https://chromium-review.googlesource.com/562914
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485230}
[modify] https://crrev.com/eadc611f5b2dbdc4aeb42c9092119444042f8521/third_party/WebKit/LayoutTests/editing/selection/shift-click.html
[modify] https://crrev.com/eadc611f5b2dbdc4aeb42c9092119444042f8521/third_party/WebKit/Source/core/editing/SelectionController.cpp
[modify] https://crrev.com/eadc611f5b2dbdc4aeb42c9092119444042f8521/third_party/WebKit/Source/core/editing/VisibleSelection.cpp

Project Member

Comment 16 by bugdroid1@chromium.org, Jul 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5d524b67a1b83e06c2a982ca6ee6bc77f9582db1

commit 5d524b67a1b83e06c2a982ca6ee6bc77f9582db1
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Tue Jul 11 11:23:26 2017

Get rid of useless member variable VisibleSelection::granularity_

This patch gets rid of useless member variable |granularity_| from
|VisibleSelectionTemplate| for improving code health.

Bug:  692923 
Change-Id: I0ed67a6076e4c34496cf8c6eaf39f497572f614d
Reviewed-on: https://chromium-review.googlesource.com/566200
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485583}
[modify] https://crrev.com/5d524b67a1b83e06c2a982ca6ee6bc77f9582db1/third_party/WebKit/Source/core/editing/VisibleSelection.cpp
[modify] https://crrev.com/5d524b67a1b83e06c2a982ca6ee6bc77f9582db1/third_party/WebKit/Source/core/editing/VisibleSelection.h

Project Member

Comment 17 by bugdroid1@chromium.org, Jul 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5cb481239f13fdd3454fe56aadcb56d7ffb822d8

commit 5cb481239f13fdd3454fe56aadcb56d7ffb822d8
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Wed Jul 12 06:47:09 2017

Get rid of SelectionTemplate::granularity_

This patch gets rid of |SelectionTemplate::granularity_| since it is always
|TextGranularity::kCharacter| since the patch[1] for improving code health.

This patch also renames |SelectionTypeWithLegacyGranularity()| to |Type()| since
|SelectionTemplate| doesn't have granularity any more.

[1] http://crrev.com/2970043002: Simplify
SelectionController::UpdateSelectionForMouseDrag()

Bug:  692923 
Change-Id: Ic58dfcf8eef7f7fe2121df27b5639d98da16986d
Reviewed-on: https://chromium-review.googlesource.com/567781
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485881}
[modify] https://crrev.com/5cb481239f13fdd3454fe56aadcb56d7ffb822d8/third_party/WebKit/Source/core/editing/Editor.cpp
[modify] https://crrev.com/5cb481239f13fdd3454fe56aadcb56d7ffb822d8/third_party/WebKit/Source/core/editing/FrameSelection.cpp
[modify] https://crrev.com/5cb481239f13fdd3454fe56aadcb56d7ffb822d8/third_party/WebKit/Source/core/editing/GranularityStrategy.h
[modify] https://crrev.com/5cb481239f13fdd3454fe56aadcb56d7ffb822d8/third_party/WebKit/Source/core/editing/SelectionController.cpp
[modify] https://crrev.com/5cb481239f13fdd3454fe56aadcb56d7ffb822d8/third_party/WebKit/Source/core/editing/SelectionEditor.cpp
[modify] https://crrev.com/5cb481239f13fdd3454fe56aadcb56d7ffb822d8/third_party/WebKit/Source/core/editing/SelectionTemplate.cpp
[modify] https://crrev.com/5cb481239f13fdd3454fe56aadcb56d7ffb822d8/third_party/WebKit/Source/core/editing/SelectionTemplate.h
[modify] https://crrev.com/5cb481239f13fdd3454fe56aadcb56d7ffb822d8/third_party/WebKit/Source/core/editing/SelectionTemplateTest.cpp
[modify] https://crrev.com/5cb481239f13fdd3454fe56aadcb56d7ffb822d8/third_party/WebKit/Source/core/editing/VisibleSelection.cpp
[modify] https://crrev.com/5cb481239f13fdd3454fe56aadcb56d7ffb822d8/third_party/WebKit/Source/core/html/TextControlElement.cpp

Comment 18 by yosin@chromium.org, Jul 28 2017

Status: Fixed (was: Available)
Blockedon: 748361

Sign in to add a comment