Android-side of insertion/selection handles visibility.

Android-side of insertion/selection handles visibility.

This patch relies on other two patches:
https://codereview.chromium.org/177903010/
https://codereview.chromium.org/186753002/
to correctly compute the visibility of selection and
insertion handles when editing text input, text area
and contenteditable elements.

BUG=236033
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258839

[timvolodine, 2014-03-19 11:31:10]

+ jdduke@ - I think you are the right person to have a look at this

[jdduke (slow), 2014-03-19 11:42:05]

Adding cjhopman@, who has a bit more experience in this area.

[cjhopman, 2014-03-19 23:25:19]

This lgtm.

I have concerns about us only having knowledge of the selection root element's
bounding box to determine if the bound of the selection is visible. I've
mentioned that in the bug, and this change will be a huge improvement, so go for
it.

https://codereview.chromium.org/188023002/diff/70001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/188023002/diff/70001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:363:
private final RenderCoordinates.NormalizedPoint
mBottomRightVisiblityClippingPoint;
s/mBottomRightVisiblityClippingPoint/mBottomRightVisibilityClippingPoint/

or more concisely

s/Visiblity/Visibility

https://codereview.chromium.org/188023002/diff/70001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:455:
mTopLeftVisibilityClippingPoint = mRenderCoordinates.createNormalizedPoint();
Nit: initialize these just after mInsertionHandlePoint

[aelias_OOO_until_Jul13, 2014-03-19 23:39:14]

Hmm, I guess this patch can go through since it makes things better in the short
term, but note that everything you're doing will be deleted and done differently
after https://code.google.com/p/chromium/issues/detail?id=135959 happens (since
we'll just clip them on the renderer side).  It's not a good use of time to make
all these incremental fixes on top of an approach we know is fundamentally
busted.

[timvolodine, 2014-03-20 17:17:53]

cjhopman@: thanks for the review.

aelias@: Thanks for the information, I was not aware of a larger effort to
improve the selection framework. Looks like it makes sense to handle the
selection visibility closer to the source instead of propagating to browser-side
Android code.

Also looking at the bug it appears this is a long-standing issue, so it makes
sense to provide reasonable short term fixes. Are there any design docs/patches
regarding selection? I am asking this because there are other selection bugs
which need fixing as well.

Also, this patch targets selection bug on Android, but I think we also display
handles on ChromeOS (on touch screen devices), so it's worth checking if the
same issues apply there.

https://codereview.chromium.org/188023002/diff/70001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/188023002/diff/70001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:363:
private final RenderCoordinates.NormalizedPoint
mBottomRightVisiblityClippingPoint;
On 2014/03/19 23:25:19, cjhopman wrote:
> s/mBottomRightVisiblityClippingPoint/mBottomRightVisibilityClippingPoint/
> 
> or more concisely
> 
> s/Visiblity/Visibility

Done.

https://codereview.chromium.org/188023002/diff/70001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:455:
mTopLeftVisibilityClippingPoint = mRenderCoordinates.createNormalizedPoint();
On 2014/03/19 23:25:19, cjhopman wrote:
> Nit: initialize these just after mInsertionHandlePoint

Done.

[aelias_OOO_until_Jul13, 2014-03-21 03:15:01]

Sorry, I don't have a design doc yet, although in a nutshell the idea is to make
each selection handle a WebLayer created by Blink, and move it around using a
fake touch event handler region.  Anyway, for the purpose of knowing what kind
of bug you can usefully fix, any fix 100% in Blink is likely to apply in the
future, while anything touching the browser process or Java code is guaranteed
to be deleted.

It's definitely not worth making browser-side changes on ChromeOS/Windows 8
either since those platforms will also be able to switch to renderer-side
handles when we have them.

[aelias_OOO_until_Jul13, 2014-03-21 03:17:16]

Also, agreed it's a long-standing issue but mainly because this area has not had
anyone owning it until now.  Recently Jared freed up and will work on moving it
to the renderer.  It should be a medium-sized project, only a few months,
particularly since the approach will be less complicated than it is currently.

[jdduke (slow), 2014-03-21 08:49:51]

On 2014/03/21 03:17:16, aelias wrote:
> Also, agreed it's a long-standing issue but mainly because this area has not
had
> anyone owning it until now.  Recently Jared freed up and will work on moving
it
> to the renderer.  It should be a medium-sized project, only a few months,
> particularly since the approach will be less complicated than it is currently.

I'll work with Alex to get a plan/design on record, hopefully within a week or
so.  We'll post it for comments to ensure non-Android platform requirements will
be met.

[timvolodine, 2014-03-21 15:18:09]

On 2014/03/21 08:49:51, jdduke wrote:
> On 2014/03/21 03:17:16, aelias wrote:
> > Also, agreed it's a long-standing issue but mainly because this area has not
> had
> > anyone owning it until now.  Recently Jared freed up and will work on moving
> it
> > to the renderer.  It should be a medium-sized project, only a few months,
> > particularly since the approach will be less complicated than it is
currently.
> 
> I'll work with Alex to get a plan/design on record, hopefully within a week or
> so.  We'll post it for comments to ensure non-Android platform requirements
will
> be met.

ok good to hear! looking forward to the design doc.

[timvolodine, 2014-03-21 18:24:10]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-03-21 18:24:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/188023002/90001

[commit-bot: I haz the power, 2014-03-21 18:58:24]

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

[commit-bot: I haz the power, 2014-03-21 18:58:24]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[timvolodine, 2014-03-21 19:05:46]

+ tedchoc@ : as owner for public/android/* and content/browser/android/*.

[jdduke (slow), 2014-03-21 20:29:11]

On 2014/03/21 19:05:46, timvolodine wrote:
> + tedchoc@ : as owner for public/android/* and content/browser/android/*.

I think you need an owner for content/browser/renderer_host (aelias@), and I'm
an owner for the above directories. 

It's a shame to add all of this wiring only to have to rip it out in a few
weeks.  I somewhat disagree with the argument that long-standing issues deserve
short term fixes, but I'll happily defer to cjhopman@ and aelias@ on this. lgtm.

[cjhopman, 2014-03-21 20:40:09]

I thought "long-standing issue" was referring to
http://crbug.com/135959which was originally filed (as b/
5525927 <http://b.corp.google.com/5525927>) about 2.5 years ago. A lot of
bugs with text selection have gone unfixed for much of that period because
we thought we should just wait for crbug/135959. I don't know that that was
a good decision.


On Fri, Mar 21, 2014 at 1:29 PM, <jdduke@chromium.org> wrote:

> On 2014/03/21 19:05:46, timvolodine wrote:
>
>> + tedchoc@ : as owner for public/android/* and content/browser/android/*.
>>
>
> I think you need an owner for content/browser/renderer_host (aelias@),
> and I'm
> an owner for the above directories.
>
> It's a shame to add all of this wiring only to have to rip it out in a few
> weeks.  I somewhat disagree with the argument that long-standing issues
> deserve
> short term fixes, but I'll happily defer to cjhopman@ and aelias@ on
> this. lgtm.
>
> https://codereview.chromium.org/188023002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aelias_OOO_until_Jul13, 2014-03-21 20:48:50]

The bad decision was "waiting for" it, as opposed to "working on" it :).  It's
long-standing just because it had no owner rather than because it's particularly
intractable.  Anyway, the main point is that we're finally starting now, so even
if the argument to avoid short-term fixes didn't apply 1 year ago, it does now.

Anyway, this lgtm because it's already written and does not make it
significantly harder to transition to the new approach in the future.

[jdduke (slow), 2014-03-21 20:52:18]

On 2014/03/21 20:40:09, cjhopman wrote:
> I thought "long-standing issue" was referring to
> http://crbug.com/135959which was originally filed (as b/
> 5525927 <http://b.corp.google.com/5525927>) about 2.5 years ago. A lot of
> bugs with text selection have gone unfixed for much of that period because
> we thought we should just wait for crbug/135959. I don't know that that was
> a good decision.

Indeed, hopefully we can make something happen soon and not step on too many
toes in the meantime.

[mlamouri (slow - plz ping), 2014-03-22 12:42:39]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-22 12:42:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/188023002/90001

[commit-bot: I haz the power, 2014-03-24 02:24:12]

Change committed as 258839

[timvolodine, 2014-03-24 12:40:19]

On 2014/03/24 02:24:12, I haz the power (commit-bot) wrote:
> Change committed as 258839

thanks guys!
yes we should sync better in the future..

[jdduke (slow), 2014-05-09 23:26:27]

A revert of this CL has been created in
https://codereview.chromium.org/278923003/ by jdduke@chromium.org.

The reason for reverting is: There are a number of cases where this incorrectly
hides the selection/insertion
handles in an editable node, see crbug.com/372056. .