Support dragging texts into Chrome on Android.

Support dragging texts into Chrome on Android.

To do so, we just need to plumb the DragEvent from ContentView.
This CL also corrects the event coordinates passed to blink, which expects CSS
pixels.

BUG=584789
Committed: https://crrev.com/767aab0b0e6f75efec38c160e092c31647810be6
Cr-Commit-Position: refs/heads/master@{#404248}

[hush (inactive), 2016-07-01 00:29:43]

hush@chromium.org changed reviewers:
+ tedchoc@chromium.org

[hush (inactive), 2016-07-01 00:29:43]

PTAL

I tested on chrome with a <TextArea> zoomed in and scrolled both horizontally
and vertically, and it worked.

[Ted C, 2016-07-01 17:55:05]

https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3318:
float xPix = event.getX() + mRenderCoordinates.getScrollXPixInt();
why not use mCurrentTouchOffsetX and mCurrentTouchOffsetY (instead of
getContentOffsetYPix())?  Seems like we should try to be consistent with touch
events (either convert them to this or to use the same)

[hush (inactive), 2016-07-01 18:00:06]

https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3318:
float xPix = event.getX() + mRenderCoordinates.getScrollXPixInt();
On 2016/07/01 17:55:04, Ted C wrote:
> why not use mCurrentTouchOffsetX and mCurrentTouchOffsetY (instead of
> getContentOffsetYPix())?  Seems like we should try to be consistent with touch
> events (either convert them to this or to use the same)

I'm not sure how mCurrentTouchOffsetX and mCurrentTouchOffsetY is set, but I
always get 0. So their values are wrong.

[Ted C, 2016-07-01 18:05:59]

On 2016/07/01 18:00:06, hush wrote:
>
https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3318:
> float xPix = event.getX() + mRenderCoordinates.getScrollXPixInt();
> On 2016/07/01 17:55:04, Ted C wrote:
> > why not use mCurrentTouchOffsetX and mCurrentTouchOffsetY (instead of
> > getContentOffsetYPix())?  Seems like we should try to be consistent with
touch
> > events (either convert them to this or to use the same)
> 
> I'm not sure how mCurrentTouchOffsetX and mCurrentTouchOffsetY is set, but I
> always get 0. So their values are wrong.

Did you scroll off the omnibox?  That is what causes it to change.

Updated here:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Seeing that is how we handle touch events, I strongly doubt your claim they are
wrong is correct.

[hush (inactive), 2016-07-01 21:42:23]

On 2016/07/01 18:05:59, Ted C wrote:
> On 2016/07/01 18:00:06, hush wrote:
> >
>
https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3318:
> > float xPix = event.getX() + mRenderCoordinates.getScrollXPixInt();
> > On 2016/07/01 17:55:04, Ted C wrote:
> > > why not use mCurrentTouchOffsetX and mCurrentTouchOffsetY (instead of
> > > getContentOffsetYPix())?  Seems like we should try to be consistent with
> touch
> > > events (either convert them to this or to use the same)
> > 
> > I'm not sure how mCurrentTouchOffsetX and mCurrentTouchOffsetY is set, but I
> > always get 0. So their values are wrong.
> 
> Did you scroll off the omnibox?  That is what causes it to change.
> 
> Updated here:
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> 
> Seeing that is how we handle touch events, I strongly doubt your claim they
are
> wrong is correct.

No I didn't scroll offset the omnibox. It was there. But mCurrentTouchOffsetX
and mCurrentTouchOffsetY are 0.
I logged in
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

So, mCurrentTouchOffsetX and mCurrentTouchOffsetY are reset back to 0 for
ACTION_UP, ACTION_CANCEL and ACTION_HOVER_EXIT.

To put it in another way, mCurrentTouchOffsetX and mCurrentTouchOffsetY are only
meaningful "during" a motion event, that is between ACTION_DOWN and ACTION_up,
for example.
Unfortunately, there is no motion event fired when dragging texts into chrome.
I can't rely on mCurrentTouchOffsetX and mCurrentTouchOffsetY for compensating
the x and y of the DragEvent.

ContentViewCore.java has already utilized
mRenderCoordinates.getContentOffsetYPix() in other places like
createVirtualStructure(), probably because of the same reason. Maybe Selim can
confirm that.

[Ted C, 2016-07-01 22:19:05]

On 2016/07/01 21:42:23, hush wrote:
> On 2016/07/01 18:05:59, Ted C wrote:
> > On 2016/07/01 18:00:06, hush wrote:
> > >
> >
>
https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
> > > File
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3318:
> > > float xPix = event.getX() + mRenderCoordinates.getScrollXPixInt();
> > > On 2016/07/01 17:55:04, Ted C wrote:
> > > > why not use mCurrentTouchOffsetX and mCurrentTouchOffsetY (instead of
> > > > getContentOffsetYPix())?  Seems like we should try to be consistent with
> > touch
> > > > events (either convert them to this or to use the same)
> > > 
> > > I'm not sure how mCurrentTouchOffsetX and mCurrentTouchOffsetY is set, but
I
> > > always get 0. So their values are wrong.
> > 
> > Did you scroll off the omnibox?  That is what causes it to change.
> > 
> > Updated here:
> >
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> > 
> > Seeing that is how we handle touch events, I strongly doubt your claim they
> are
> > wrong is correct.
> 
> No I didn't scroll offset the omnibox. It was there. But mCurrentTouchOffsetX
> and mCurrentTouchOffsetY are 0.
> I logged in
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> 
> So, mCurrentTouchOffsetX and mCurrentTouchOffsetY are reset back to 0 for
> ACTION_UP, ACTION_CANCEL and ACTION_HOVER_EXIT.
> 
> To put it in another way, mCurrentTouchOffsetX and mCurrentTouchOffsetY are
only
> meaningful "during" a motion event, that is between ACTION_DOWN and ACTION_up,
> for example.
> Unfortunately, there is no motion event fired when dragging texts into chrome.
> I can't rely on mCurrentTouchOffsetX and mCurrentTouchOffsetY for compensating
> the x and y of the DragEvent.
> 
> ContentViewCore.java has already utilized
> mRenderCoordinates.getContentOffsetYPix() in other places like
> createVirtualStructure(), probably because of the same reason. Maybe Selim can
> confirm that.

The problem that I see RenderCoordinates are updated as part of #updateFrameInfo
which is the last known positioning from the renderer, which can be out of
sync with the view system.

I do see the problem you are stating with CompositorViewHolder about it being
cleared, but I still wonder if we should be setting the offsets in the same
manner but for DragEvents.

I'm going to assume you can't start a drag gesture but then use your other
handle to trigger touches right?

If we did something like this, would it work?

private void setContentViewMotionEventOffsets(
        MotionEvent e, DragEvent drag, boolean canClear) {
    ...
    boolean isExitEvent = false;
    boolean isEnterEvent = false;
    if (e != null) {
        int actionMasked = e.getActionMasked();
        if (SPenSupport.isSPenSupported(getContext())) {
            actionMasked = SPenSupport.convertSPenEventAction(actionMasked);
        }
        isEnterEvent = actionMasked == MotionEvent.ACTION_DOWN
                || actionMasked == MotionEvent.ACTION_HOVER_ENTER;
        isExitEvent = actionMasked == MotionEvent.ACTION_UP
                || actionMasked == MotionEvent.ACTION_CANCEL
                || actionMasked == MotionEvent.ACTION_HOVER_EXIT;
    } else if (drag != null) {
        int action = drag.getAction();
        ... same logic for isEnterEvent/isExitEvent
    }

    ... existing logic to use new booleans instead of actions
}

@Override
public boolean onDragEvent(DragEvent event) {
    setContentViewMotionEventOffsets(null, event, false);
    boolean retVal = super.onDragEvent(event);
    setContentViewMotionEventOffsets(null, event, true);
    return retVal;
}

Probably would need to rename setContentViewMotionEventOffsets to
setContentViewTouchEventOffsets since it would apply to non-motion events only.

Again, I think we should attempt to unify the existing behavior for offsetting
if at all possible.

[chromium-reviews, 2016-07-01 22:55:00]

> I'm going to assume you can't start a drag gesture but then use your other
handle to trigger touches right?
Right. The user probably won't touch the screen with another finger while
dragging something.

> If we did something like this, would it work?
Looks like it's gonna work. I'll try it out.

On Fri, Jul 1, 2016 at 3:19 PM, <tedchoc@chromium.org> wrote:

> On 2016/07/01 21:42:23, hush wrote:
> > On 2016/07/01 18:05:59, Ted C wrote:
> > > On 2016/07/01 18:00:06, hush wrote:
> > > >
> > >
> >
>
>
https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
> > > > File
> > > >
> > >
> >
>
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> > > > (right):
> > > >
> > > >
> > >
> >
>
>
https://codereview.chromium.org/2113183003/diff/20001/content/public/android/...
> > > >
> > >
> >
>
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3318:
> > > > float xPix = event.getX() + mRenderCoordinates.getScrollXPixInt();
> > > > On 2016/07/01 17:55:04, Ted C wrote:
> > > > > why not use mCurrentTouchOffsetX and mCurrentTouchOffsetY (instead
> of
> > > > > getContentOffsetYPix())? Seems like we should try to be consistent
> with
> > > touch
> > > > > events (either convert them to this or to use the same)
> > > >
> > > > I'm not sure how mCurrentTouchOffsetX and mCurrentTouchOffsetY is
> set, but
> I
> > > > always get 0. So their values are wrong.
> > >
> > > Did you scroll off the omnibox? That is what causes it to change.
> > >
> > > Updated here:
> > >
> >
>
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> > >
> > > Seeing that is how we handle touch events, I strongly doubt your claim
> they
> > are
> > > wrong is correct.
> >
> > No I didn't scroll offset the omnibox. It was there. But
> mCurrentTouchOffsetX
> > and mCurrentTouchOffsetY are 0.
> > I logged in
> >
>
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> >
> > So, mCurrentTouchOffsetX and mCurrentTouchOffsetY are reset back to 0 for
> > ACTION_UP, ACTION_CANCEL and ACTION_HOVER_EXIT.
> >
> > To put it in another way, mCurrentTouchOffsetX and mCurrentTouchOffsetY
> are
> only
> > meaningful "during" a motion event, that is between ACTION_DOWN and
> ACTION_up,
> > for example.
> > Unfortunately, there is no motion event fired when dragging texts into
> chrome.
> > I can't rely on mCurrentTouchOffsetX and mCurrentTouchOffsetY for
> compensating
> > the x and y of the DragEvent.
> >
> > ContentViewCore.java has already utilized
> > mRenderCoordinates.getContentOffsetYPix() in other places like
> > createVirtualStructure(), probably because of the same reason. Maybe
> Selim can
> > confirm that.
>
> The problem that I see RenderCoordinates are updated as part of
> #updateFrameInfo
> which is the last known positioning from the renderer, which can be out of
> sync with the view system.
>
> I do see the problem you are stating with CompositorViewHolder about it
> being
> cleared, but I still wonder if we should be setting the offsets in the same
> manner but for DragEvents.
>
> I'm going to assume you can't start a drag gesture but then use your other
> handle to trigger touches right?
>
> If we did something like this, would it work?
>
> private void setContentViewMotionEventOffsets(
> MotionEvent e, DragEvent drag, boolean canClear) {
> ...
> boolean isExitEvent = false;
> boolean isEnterEvent = false;
> if (e != null) {
> int actionMasked = e.getActionMasked();
> if (SPenSupport.isSPenSupported(getContext())) {
> actionMasked = SPenSupport.convertSPenEventAction(actionMasked);
> }
> isEnterEvent = actionMasked == MotionEvent.ACTION_DOWN
> || actionMasked == MotionEvent.ACTION_HOVER_ENTER;
> isExitEvent = actionMasked == MotionEvent.ACTION_UP
> || actionMasked == MotionEvent.ACTION_CANCEL
> || actionMasked == MotionEvent.ACTION_HOVER_EXIT;
> } else if (drag != null) {
> int action = drag.getAction();
> ... same logic for isEnterEvent/isExitEvent
> }
>
> ... existing logic to use new booleans instead of actions
> }
>
> @Override
> public boolean onDragEvent(DragEvent event) {
> setContentViewMotionEventOffsets(null, event, false);
> boolean retVal = super.onDragEvent(event);
> setContentViewMotionEventOffsets(null, event, true);
> return retVal;
> }
>
> Probably would need to rename setContentViewMotionEventOffsets to
> setContentViewTouchEventOffsets since it would apply to non-motion events
> only.
>
> Again, I think we should attempt to unify the existing behavior for
> offsetting
> if at all possible.
>
> https://codereview.chromium.org/2113183003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hush (inactive), 2016-07-02 00:24:55]

Patchset #3 (id:40001) has been deleted

[hush (inactive), 2016-07-02 00:26:46]

Patchset #3 (id:60001) has been deleted

[hush (inactive), 2016-07-02 00:28:00]

PTAL

[Ted C, 2016-07-02 18:20:28]

lgtm

[hush (inactive), 2016-07-02 19:24:31]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2016-07-02 19:24:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-02 20:39:55]

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

[commit-bot: I haz the power, 2016-07-02 20:39:56]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[hush (inactive), 2016-07-06 18:58:58]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2016-07-06 18:59:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-06 21:40:23]

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

[commit-bot: I haz the power, 2016-07-06 21:40:24]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[hush (inactive), 2016-07-07 19:24:25]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2016-07-07 19:24:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 22:13:08]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-07-07 22:13:34]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 22:15:26]

Description was changed from

==========
Support dragging texts into Chrome on Android.

To do so, we just need to plumb the DragEvent from ContentView.
This CL also corrects the event coordinates passed to blink, which expects CSS
pixels.

BUG=584789
==========

to

==========
Support dragging texts into Chrome on Android.

To do so, we just need to plumb the DragEvent from ContentView.
This CL also corrects the event coordinates passed to blink, which expects CSS
pixels.

BUG=584789

Committed: https://crrev.com/767aab0b0e6f75efec38c160e092c31647810be6
Cr-Commit-Position: refs/heads/master@{#404248}
==========

[commit-bot: I haz the power, 2016-07-07 22:15:27]

Patchset 3 (id:??) landed as
https://crrev.com/767aab0b0e6f75efec38c160e092c31647810be6
Cr-Commit-Position: refs/heads/master@{#404248}