Swap touch selection handles when they are interchanged

Swap touch selection handles when they are interchanged

When the end selection handle and start selection handle are
interchanged when dragging selection, swapping of handles is
done to maintain states of appropriate handles.

BUG=544095
Committed: https://crrev.com/0ee7ae6db8d2ecb16154b1678827991a2f3f9160
Cr-Commit-Position: refs/heads/master@{#366031}

[AviD, 2015-10-16 12:57:38]

avi.nitk@samsung.com changed reviewers:
+ ajith.v@chromium.org

[AKV, 2015-10-21 06:19:29]

Please check inline comments.

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:93: if (active_status_ ==
SELECTION_ACTIVE &&
We can break this condition to 2 ifs to add more readability.
if (active_status_ == SELECTION_ACTIVE) {
  if (----- && -----) {
  }
}

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:663: if (active_status_ !=
SELECTION_ACTIVE)
May be we can return actual orientation and do this validity check inside the
test code.

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:670: if (active_status_ !=
SELECTION_ACTIVE)
ditto

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_unittest.cc:1324:
TEST_F(TouchSelectionControllerTest, SelectionNoOrientationChangeWhenSwapped) {
Can we name it as OrientationUnchangedDuringHandleSwap ?

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_unittest.cc:1329: float
line_height = 10.f;
nit: 10.0f

[AviD, 2015-10-21 06:40:02]

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:93: if (active_status_ ==
SELECTION_ACTIVE &&
On 2015/10/21 06:19:29, AKV wrote:
> We can break this condition to 2 ifs to add more readability.
> if (active_status_ == SELECTION_ACTIVE) {
>   if (----- && -----) {
>   }
> }

Done.

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:663: if (active_status_ !=
SELECTION_ACTIVE)
On 2015/10/21 06:19:29, AKV wrote:
> May be we can return actual orientation and do this validity check inside the
> test code.

I think this is better as it will also check for NULL case for handles in case
of selection.

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_unittest.cc:1324:
TEST_F(TouchSelectionControllerTest, SelectionNoOrientationChangeWhenSwapped) {
On 2015/10/21 06:19:29, AKV wrote:
> Can we name it as OrientationUnchangedDuringHandleSwap ?

Most of the test cases here start with Selection/Insertion probably for clarity
of the test case premise. WDYT?

https://codereview.chromium.org/1404163004/diff/40001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_unittest.cc:1329: float
line_height = 10.f;
On 2015/10/21 06:19:29, AKV wrote:
> nit: 10.0f

This is short for 10.0f. Used in other test cases too :)

[AKV, 2015-10-21 13:21:33]

Thanks.
peer review looks good to me!

[AviD, 2015-10-21 13:39:58]

avi.nitk@samsung.com changed reviewers:
+ jdduke@chromium.org

[AviD, 2015-10-21 13:39:58]

@jdduke: Could you PTAL?

[jdduke (slow), 2015-10-21 16:29:44]

On 2015/10/21 13:39:58, AviD wrote:
> @jdduke: Could you PTAL?

This has been a known issue, since, well, forever. But are there any user-facing
side effects of not swapping them? The handles still render correctly, right,
even while dragging? Or does this conflict with the new handle orienting logic
(re-orienting the handles based on the viewport) in touch_handle.cc?

[AviD, 2015-10-22 11:24:14]

On 2015/10/21 16:29:44, jdduke wrote:
> On 2015/10/21 13:39:58, AviD wrote:
> > @jdduke: Could you PTAL?
> 
> This has been a known issue, since, well, forever. But are there any
user-facing
> side effects of not swapping them? The handles still render correctly, right,
> even while dragging? Or does this conflict with the new handle orienting logic
> (re-orienting the handles based on the viewport) in touch_handle.cc?

The issue is that there is a gap between the touch position and the handle
position. It is evident with Adaptive handles enabled.
I have attached videos on crbug.com/544095, which shows the same. It appears
like the handle being dragged is not moving along with the finger.
Even without Adaptive handles, there is some gap between the handle and touch
position when handles are swapped, but since it is very little it is not
noticeable. This is probably the reason why there are no user-facing effects
with the previous approach.
Also, logically, I feel this is more appropriate because - in a scenario where
end handle is being dragged and position swap happens, then the end handle stops
moving and start handle begins to move; even though start handle is
moving(/being dragged), is_dragging for start handle is set to false and end for
handle it is set to true. This takes care of setting the flag for the correct
handle.
Please correct me if I am wrong.

[jdduke (slow), 2015-10-22 15:32:48]

jdduke@chromium.org changed reviewers:
+ mohsen@chromium.org

[jdduke (slow), 2015-10-22 15:32:48]

+mohsen might have some thoughts here.

I guess the right (long term) thing to do here would be to route selection in
terms of base/extent, instead of start/end. Then, when we start a drag, we can
swap the base/extent handles, and we don't have to try to guess that we're doing
the right thing in terms of updating the right handle while dragging. I had a
patch to do that a while ago, but never bothered to push it through.

Are there any circumstances where this fix would fail, perhaps if the content is
moving while the handles swap start/end locations?

[AviD, 2015-10-26 14:32:06]

On 2015/10/22 15:32:48, jdduke wrote:
> +mohsen might have some thoughts here.
> 
> I guess the right (long term) thing to do here would be to route selection in
> terms of base/extent, instead of start/end. Then, when we start a drag, we can
> swap the base/extent handles, and we don't have to try to guess that we're
doing
> the right thing in terms of updating the right handle while dragging. I had a
> patch to do that a while ago, but never bothered to push it through.
> 
> Are there any circumstances where this fix would fail, perhaps if the content
is
> moving while the handles swap start/end locations?

The swap logic would work only when either handles are being dragged. So, even
if content moves, the swap logic will only make sure that the touch handle which
is currently being dragged does not change orientation. (If orientation changes
while dragging, it might appear as if the handle is not in control of the user)

[AviD, 2015-11-16 06:57:48]

@mohsen: Could you PTAL?

[mohsen, 2015-11-19 21:35:36]

This patch generally looks good to me. However, because of an issue in
TouchHandleDrawableAura, it does not work properly on Aura. The issue is that
when handle's orientation is changed after its creation, the handle window is
not invalidated properly. This will also probably affect adaptive handles in
which handle's orientation can change after creation. Can you also fix that
problem here? You can simply call SchedulePaintInRect() on the handle window
when its orientation changes.

https://codereview.chromium.org/1404163004/diff/60001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1404163004/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:663: const {
Can you add this logic directly in |TouchSelectionControllerTestApi| and get rid
of these functions here?

https://codereview.chromium.org/1404163004/diff/60001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/1404163004/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_unittest.cc:1359: event_time +=
base::TimeDelta::FromMilliseconds(300);
Can you write this based on |kDefaultTapTimeoutMs|?

https://codereview.chromium.org/1404163004/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_unittest.cc:1360: event =
MockMotionEvent(MockMotionEvent::ACTION_UP, event_time, 10, 5);
Where does this (10, 5) location come from? Shouldn't it match |offset_rect|?

[AviD, 2015-12-11 11:25:54]

Added call to SchedulePaintInRect(relative_bounds_) in Aura handles.
Could you PTAL?

https://codereview.chromium.org/1404163004/diff/60001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/1404163004/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:663: const {
On 2015/11/19 21:35:36, mohsen wrote:
> Can you add this logic directly in |TouchSelectionControllerTestApi| and get
rid
> of these functions here?

Done.

https://codereview.chromium.org/1404163004/diff/60001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/1404163004/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_unittest.cc:1359: event_time +=
base::TimeDelta::FromMilliseconds(300);
On 2015/11/19 21:35:36, mohsen wrote:
> Can you write this based on |kDefaultTapTimeoutMs|?

Done.

https://codereview.chromium.org/1404163004/diff/60001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_unittest.cc:1360: event =
MockMotionEvent(MockMotionEvent::ACTION_UP, event_time, 10, 5);
On 2015/11/19 21:35:36, mohsen wrote:
> Where does this (10, 5) location come from? Shouldn't it match |offset_rect|?

Here, the touch_up position does not matter as the selection is being simulated
by ChangeSelection() method. I have changed it to match |offset_rect|.

[mohsen, 2015-12-14 15:55:40]

Thanks, LGTM.

[AviD, 2015-12-15 05:48:46]

The CQ bit was checked by avi.nitk@samsung.com

[commit-bot: I haz the power, 2015-12-15 05:49:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1404163004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1404163004/80001

[commit-bot: I haz the power, 2015-12-15 05:59:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-15 05:59:11]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[AviD, 2015-12-18 05:31:21]

The CQ bit was checked by avi.nitk@samsung.com

[AviD, 2015-12-18 05:31:22]

The patchset sent to the CQ was uploaded after l-g-t-m from mohsen@chromium.org
Link to the patchset: https://codereview.chromium.org/1404163004/#ps100001
(title: "linux build error")

[commit-bot: I haz the power, 2015-12-18 05:32:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1404163004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1404163004/100001

[commit-bot: I haz the power, 2015-12-18 06:34:33]

Description was changed from

==========
Swap touch selection handles when they are interchanged

When the end selection handle and start selection handle are
interchanged when dragging selection, swapping of handles is
done to maintain states of appropriate handles.

BUG=544095
==========

to

==========
Swap touch selection handles when they are interchanged

When the end selection handle and start selection handle are
interchanged when dragging selection, swapping of handles is
done to maintain states of appropriate handles.

BUG=544095
==========

[commit-bot: I haz the power, 2015-12-18 06:34:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-18 06:36:03]

Description was changed from

==========
Swap touch selection handles when they are interchanged

When the end selection handle and start selection handle are
interchanged when dragging selection, swapping of handles is
done to maintain states of appropriate handles.

BUG=544095
==========

to

==========
Swap touch selection handles when they are interchanged

When the end selection handle and start selection handle are
interchanged when dragging selection, swapping of handles is
done to maintain states of appropriate handles.

BUG=544095

Committed: https://crrev.com/0ee7ae6db8d2ecb16154b1678827991a2f3f9160
Cr-Commit-Position: refs/heads/master@{#366031}
==========

[commit-bot: I haz the power, 2015-12-18 06:36:04]

Patchset 6 (id:??) landed as
https://crrev.com/0ee7ae6db8d2ecb16154b1678827991a2f3f9160
Cr-Commit-Position: refs/heads/master@{#366031}

[mohsen, 2015-12-21 21:05:50]

https://codereview.chromium.org/1404163004/diff/100001/ui/touch_selection/tou...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/1404163004/diff/100001/ui/touch_selection/tou...
ui/touch_selection/touch_handle_drawable_aura.cc:121: relative_bounds_.width(),
relative_bounds_.height());
I think you should've used gfx::ToEnclosingRect() function to convert gfx::RectF
to gfx::Rect.