Workaround for Samsung email issues

Workaround for Samsung email issues

Samsung's email works around Range.getClientRects()'s behavior to return
an empty sequence for a collapsed range, and the workaround is to insert
a non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. With the changes introduced in
https://codereview.chromium.org/2687273002, now inserting a non-zero
space triggers a selectionchanged event and it causes an infinite loop,
leading to issues with scroll, caret blink, and paste.

Because it is difficult for Samsung to update their email any time soon,
we undo the above changes only for WebView by adding a runtime
flag and set it. The rationale for generalizing this to WebView is:

1) Package name checks generally don't work very well - there may be
variants for different devices or beta channels.
2) JS library developers should still be concerned about out-of-date
WebView libraries anyways.

This should be fixed for version N and above, so we
limit the workaround to Android M and below.

BUG=698752, 699943
Review-Url: https://codereview.chromium.org/2776073002
Cr-Commit-Position: refs/heads/master@{#462173}
Committed: https://chromium.googlesource.com/chromium/src/+/e54ccdb9fbd84362cba3c5858deaad215e572f5c

[Changwan Ryu, 2017-03-27 01:24:28]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 01:24:44]

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

[Changwan Ryu, 2017-03-27 01:29:11]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 01:29:19]

Dry run: 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, 2017-03-27 02:41:06]

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

[commit-bot: I haz the power, 2017-03-27 02:41:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2017-03-27 04:51:24]

yosin@chromium.org changed reviewers:
+ yosin@chromium.org

[yosin_UTC9, 2017-03-27 04:51:25]

%s/empty range/collapsed range/g

Could you check return value of Range#getClientRects() for collapsed Range of
Firefox and Edge?

[Changwan Ryu, 2017-03-29 00:07:51]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 00:08:30]

Dry run: 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, 2017-03-29 01:55:59]

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

[commit-bot: I haz the power, 2017-03-29 01:56:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Changwan Ryu, 2017-03-29 20:28:51]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[Changwan Ryu, 2017-03-29 20:29:25]

Description was changed from

==========
Make getClientRects return non-null object for empty Range

According to
https://drafts.csswg.org/cssom-view/#extensions-to-the-range-interface

Range.getClientRects() should return a sequence with an empty DOMRect even
for an empty Range. The current behavior is somewhat inconsistent -
it returns an empty sequence when the range is an empty text node followed
by <br>, and it returns DOMRect for the container of the range when the
range is empty.

Samsung's Email app tries to work around this behavior by inserting a
non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. However, with some recent selection behavior changes,
inserting and removing non-zero space triggers selectionchanged events,
causing an infinite loop that ended up with scrolling and blinking
issues.

Although this is not a perfect solution, we can return DOMRect for the
range container, we can avoid these issues.

BUG=698752, 699943
==========

to

==========
Make getClientRects return non-null object for collapsed Range

According to
https://drafts.csswg.org/cssom-view/#extensions-to-the-range-interface

Range.getClientRects() should return a sequence with an empty DOMRect
even
for a collapsed Range. The current behavior is somewhat inconsistent:

1) it returns an empty sequence when the range is an empty text node
followed
by <br>
2) and it returns DOMRect for the container of the range when the
range is collapsed.
3) Moreover, if you start with empty text node followed by <br>, type a
character, and then delete the character, then Range.getClientRects()
return a sequence having ClientRect for the textbox.

Samsung's Email app tries to work around this behavior by inserting a
non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. However, with some recent selection behavior changes,
inserting and removing non-zero space triggers selectionchanged events,
causing an infinite loop that ended up with scrolling and blinking
issues.

Although this is not a perfect solution, we can return DOMRect for the
range container, we can avoid these issues.

BUG=698752, 699943
==========

[Changwan Ryu, 2017-03-29 20:29:40]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org

[Changwan Ryu, 2017-03-29 20:29:40]

PTAL

[commit-bot: I haz the power, 2017-03-29 20:29:41]

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

[Changwan Ryu, 2017-03-29 20:57:25]

Description was changed from

==========
Make getClientRects return non-null object for collapsed Range

According to
https://drafts.csswg.org/cssom-view/#extensions-to-the-range-interface

Range.getClientRects() should return a sequence with an empty DOMRect
even
for a collapsed Range. The current behavior is somewhat inconsistent:

1) it returns an empty sequence when the range is an empty text node
followed
by <br>
2) and it returns DOMRect for the container of the range when the
range is collapsed.
3) Moreover, if you start with empty text node followed by <br>, type a
character, and then delete the character, then Range.getClientRects()
return a sequence having ClientRect for the textbox.

Samsung's Email app tries to work around this behavior by inserting a
non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. However, with some recent selection behavior changes,
inserting and removing non-zero space triggers selectionchanged events,
causing an infinite loop that ended up with scrolling and blinking
issues.

Although this is not a perfect solution, we can return DOMRect for the
range container, we can avoid these issues.

BUG=698752, 699943
==========

to

==========
Make getClientRects return non-null object for collapsed Range

According to
https://drafts.csswg.org/cssom-view/#extensions-to-the-range-interface

Range.getClientRects() should return a sequence with an empty DOMRect
even
for a collapsed Range. The current behavior is somewhat inconsistent:

1) it returns an empty sequence when the range is an empty text node
followed
by <br>
2) and it returns DOMRect for the container of the range when the
range is collapsed.
3) Moreover, if you start with empty text node followed by <br>, type a
character, and then delete the character, then Range.getClientRects()
return a sequence having ClientRect for the textbox.

Samsung's Email app tries to work around this behavior by inserting a
non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. However, with some recent selection behavior changes,
inserting and removing non-zero space triggers selectionchanged events,
causing an infinite loop that ended up with scrolling and blinking
issues.

Although this is not a perfect solution, we can return DOMRect for the
range container, we can avoid these issues.

BUG=698752, 699943, 698682
==========

[commit-bot: I haz the power, 2017-03-29 22:18:25]

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

[commit-bot: I haz the power, 2017-03-29 22:18:25]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-03-30 01:51:50]

yosin@chromium.org changed reviewers:
+ foolip@chromium.org, tkent@chromium.org

[yosin_UTC9, 2017-03-30 01:51:50]

Code change it self is good shape.

My question is about behavior change of public API. The spec[1] is still in
Editor's draft
and all browsers behave differently.

foolip@, tkent@, WDYT?

[1] https://drafts.csswg.org/cssom-view/#extensions-to-the-range-interface

[tkent, 2017-03-30 02:01:36]

How Firefox and Edge work in this case?

[Changwan Ryu, 2017-03-30 02:11:27]

On 2017/03/30 02:01:36, tkent wrote:
> How Firefox and Edge work in this case?

Firefox is not well testable, but seems to return an empty sequence in certain
case:
https://bugs.chromium.org/p/chromium/issues/detail?id=698752&desc=2#c32

Edge returns a sequence of size 1, but the ClientRect has width of 0 (with this
patch chrome's result has width of that of the container)
https://bugs.chromium.org/p/chromium/issues/detail?id=698752&desc=2#c35

[tkent, 2017-03-30 02:37:39]

On 2017/03/30 at 02:11:27, changwan wrote:
> On 2017/03/30 02:01:36, tkent wrote:
> > How Firefox and Edge work in this case?
> 
> Firefox is not well testable, but seems to return an empty sequence in certain
case:
> https://bugs.chromium.org/p/chromium/issues/detail?id=698752&desc=2#c32
> 
> Edge returns a sequence of size 1, but the ClientRect has width of 0 (with
this patch chrome's result has width of that of the container)
> https://bugs.chromium.org/p/chromium/issues/detail?id=698752&desc=2#c35

Discussed with yosin@ offline.

In the collapsed case, neither elements nor text nodes are selected, the
specification doesn't define the behavior for such case.
IMO, Edge behavior looks reasonable, and we shouldn't return a rect with non-0
width for a collapsed range.

[foolip, 2017-03-30 05:28:03]

I wanted to test this a bit and wrote https://jsbin.com/seximeb/edit?html,output

The test doesn't work in Edge because it doesn't have the Range constructor,
filed
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11450614/

Anyway, it looks to me like this behavior should be testable using
web-platform-tests, and
https://github.com/w3c/web-platform-tests/tree/master/cssom-view is the right
part of the test suite.

Can you write a test exercising this change in
LayoutTests/external/wpt/cssom-view? We'll have to import that test suite first,
will do that and paste a link to the CL here.

[foolip, 2017-03-30 05:39:48]

I've sent https://codereview.chromium.org/2782423002 but you can go ahead and
just add a test at the right path in the CL and it'll be fine as long as the
next import lands before this CL.

[Changwan Ryu, 2017-03-30 15:19:53]

On 2017/03/30 02:37:39, tkent wrote:
> On 2017/03/30 at 02:11:27, changwan wrote:
> > On 2017/03/30 02:01:36, tkent wrote:
> > > How Firefox and Edge work in this case?
> > 
> > Firefox is not well testable, but seems to return an empty sequence in
certain
> case:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=698752&desc=2#c32
> > 
> > Edge returns a sequence of size 1, but the ClientRect has width of 0 (with
> this patch chrome's result has width of that of the container)
> > https://bugs.chromium.org/p/chromium/issues/detail?id=698752&desc=2#c35
> 
> Discussed with yosin@ offline.
> 
> In the collapsed case, neither elements nor text nodes are selected, the
> specification doesn't define the behavior for such case.
> IMO, Edge behavior looks reasonable, and we shouldn't return a rect with non-0
> width for a collapsed range.

Please note that I’m not trying to fix this once and for all. Rather, this is a
workaround for the Samsung email app, and also this is targeting M58. But we are
already returning inconsistent return values [1], so I expect that webpages
won’t be affected much. BTW, it is difficult to make this return zero-width
ClientRect because LayoutText has no TextBox and fixing it would require
significant changes in many other places. The alternative approach is to check
whether we’re in Samsung email apk and behave differently only when it’s the
case. If we believe that there may a side effect, I can narrow this down to
Samsung email apk with extra plumbing.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=698752&desc=2#c32

[tkent, 2017-03-31 00:16:22]

On 2017/03/30 at 15:19:53, changwan wrote:
> Please note that I’m not trying to fix this once and for all. Rather, this is
a workaround for the Samsung email app, and also this is targeting M58. But we
are already returning inconsistent return values [1], so I expect that webpages
won’t be affected much. BTW, it is difficult to make this return zero-width
ClientRect because LayoutText has no TextBox and fixing it would require
significant changes in many other places. The alternative approach is to check
whether we’re in Samsung email apk and behave differently only when it’s the
case. If we believe that there may a side effect, I can narrow this down to
Samsung email apk with extra plumbing.
> 
> [1] https://bugs.chromium.org/p/chromium/issues/detail?id=698752&desc=2#c32

Even though the current situation isn't interoperable, introducing another
behavior isn't acceptable at all.

Disabling https://codereview.chromium.org/2687273002 can fix all of the issues,
right? 
Can we add a runtime flag for https://codereview.chromium.org/2687273002, and
apply https://bugs.chromium.org/p/chromium/issues/detail?id=698752#c18 ?

[Changwan Ryu, 2017-03-31 00:46:04]

On 2017/03/31 00:16:22, tkent wrote:
> On 2017/03/30 at 15:19:53, changwan wrote:
> > Please note that I’m not trying to fix this once and for all. Rather, this
is
> a workaround for the Samsung email app, and also this is targeting M58. But we
> are already returning inconsistent return values [1], so I expect that
webpages
> won’t be affected much. BTW, it is difficult to make this return zero-width
> ClientRect because LayoutText has no TextBox and fixing it would require
> significant changes in many other places. The alternative approach is to check
> whether we’re in Samsung email apk and behave differently only when it’s the
> case. If we believe that there may a side effect, I can narrow this down to
> Samsung email apk with extra plumbing.
> > 
> > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=698752&desc=2#c32
> 
> Even though the current situation isn't interoperable, introducing another
> behavior isn't acceptable at all.
> 
> Disabling https://codereview.chromium.org/2687273002 can fix all of the
issues,
> right? 
> Can we add a runtime flag for https://codereview.chromium.org/2687273002, and
> apply https://bugs.chromium.org/p/chromium/issues/detail?id=698752#c18 ?

Thanks for the clear answer on this. Let me look into what you suggested and get
back to you soon.

[Changwan Ryu, 2017-03-31 23:03:01]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[Changwan Ryu, 2017-03-31 23:03:39]

Description was changed from

==========
Make getClientRects return non-null object for collapsed Range

According to
https://drafts.csswg.org/cssom-view/#extensions-to-the-range-interface

Range.getClientRects() should return a sequence with an empty DOMRect
even
for a collapsed Range. The current behavior is somewhat inconsistent:

1) it returns an empty sequence when the range is an empty text node
followed
by <br>
2) and it returns DOMRect for the container of the range when the
range is collapsed.
3) Moreover, if you start with empty text node followed by <br>, type a
character, and then delete the character, then Range.getClientRects()
return a sequence having ClientRect for the textbox.

Samsung's Email app tries to work around this behavior by inserting a
non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. However, with some recent selection behavior changes,
inserting and removing non-zero space triggers selectionchanged events,
causing an infinite loop that ended up with scrolling and blinking
issues.

Although this is not a perfect solution, we can return DOMRect for the
range container, we can avoid these issues.

BUG=698752, 699943, 698682
==========

to

==========
Workaround for Samsung email issues

Samsung's email works around Range.getClientRects()'s behavior to return
an empty sequence for a collapsed range, and the workaround is to insert
a non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. With the changes introduced in
https://codereview.chromium.org/2687273002, now inserting a non-zero
space triggers a selectionchanged event and it causes an infinite loop,
leading to issues with scroll, caret blink, and paste.

Because it is difficult for Samsung to update their email any time soon,
we undo the above changes only for Samsung's email by adding a runtime
flag and set it. This should be fixed for version N and above, so we
limit the workaround to Android M and below.

BUG=698752, 699943, 698682
==========

[Changwan Ryu, 2017-03-31 23:03:39]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org

[commit-bot: I haz the power, 2017-03-31 23:04:09]

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

[Changwan Ryu, 2017-03-31 23:06:14]

changwan@chromium.org changed reviewers:
+ sgurun@chromium.org

[Changwan Ryu, 2017-03-31 23:06:15]

tkent@ / sgurun@, could you take a look? Also adding aelias@ as he suggested
https://bugs.chromium.org/p/chromium/issues/detail?id=698752#c18

[aelias_OOO_until_Jul13, 2017-03-31 23:25:41]

https://codereview.chromium.org/2776073002/diff/80001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2776073002/diff/80001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:180:
&& mAppTargetSdkVersion <= Build.VERSION_CODES.M;
Why M and below?  I would've expected N and below.  Some Samsung devices have
already shipped N (some variants of Galaxy S7), have you checked this issue does
not repro on them?

[Changwan Ryu, 2017-03-31 23:28:10]

https://codereview.chromium.org/2776073002/diff/80001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2776073002/diff/80001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:180:
&& mAppTargetSdkVersion <= Build.VERSION_CODES.M;
On 2017/03/31 23:25:41, aelias wrote:
> Why M and below?  I would've expected N and below.  Some Samsung devices have
> already shipped N (some variants of Galaxy S7), have you checked this issue
does
> not repro on them?

Sorry I wasn't super clear about this. Samsung's initial test result indicates
that this does not happen on Android N. But I'll confirm with them.

[aelias_OOO_until_Jul13, 2017-03-31 23:40:21]

https://codereview.chromium.org/2776073002/diff/80001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2776073002/diff/80001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:177:
final String samsungEmailPackageId = "com.samsung.android.email.provider";
I don't think an APK name check is called for here.  Name-checks can cause
mysterious problems (e.g. beta channel or derivative fork behaving different
from main one) so I think they're only appropriate for particularly unsafe/crazy
hacks.  Since this is just restoring the previous behavior that we shipped
before, I think it's cleaner to make it global all WebView with low enough
targetSdkVersion.  We can see that as a slower, safer way of rolling out this
breaking change.

https://codereview.chromium.org/2776073002/diff/80001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:180:
&& mAppTargetSdkVersion <= Build.VERSION_CODES.M;
On 2017/03/31 at 23:28:10, Changwan Ryu wrote:
> On 2017/03/31 23:25:41, aelias wrote:
> > Why M and below?  I would've expected N and below.  Some Samsung devices
have
> > already shipped N (some variants of Galaxy S7), have you checked this issue
does
> > not repro on them?
> 
> Sorry I wasn't super clear about this. Samsung's initial test result indicates
that this does not happen on Android N. But I'll confirm with them.

OK sounds good.  Targeting M is somewhat better if we can get away with it.

[commit-bot: I haz the power, 2017-03-31 23:44:23]

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

[commit-bot: I haz the power, 2017-03-31 23:44:24]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Changwan Ryu, 2017-04-01 00:04:10]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-01 00:04:44]

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

[Changwan Ryu, 2017-04-01 00:05:06]

Description was changed from

==========
Workaround for Samsung email issues

Samsung's email works around Range.getClientRects()'s behavior to return
an empty sequence for a collapsed range, and the workaround is to insert
a non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. With the changes introduced in
https://codereview.chromium.org/2687273002, now inserting a non-zero
space triggers a selectionchanged event and it causes an infinite loop,
leading to issues with scroll, caret blink, and paste.

Because it is difficult for Samsung to update their email any time soon,
we undo the above changes only for Samsung's email by adding a runtime
flag and set it. This should be fixed for version N and above, so we
limit the workaround to Android M and below.

BUG=698752, 699943, 698682
==========

to

==========
Workaround for Samsung email issues

Samsung's email works around Range.getClientRects()'s behavior to return
an empty sequence for a collapsed range, and the workaround is to insert
a non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. With the changes introduced in
https://codereview.chromium.org/2687273002, now inserting a non-zero
space triggers a selectionchanged event and it causes an infinite loop,
leading to issues with scroll, caret blink, and paste.

Because it is difficult for Samsung to update their email any time soon,
we undo the above changes only for WebView by adding a runtime
flag and set it. The rationale for generalizing this to WebView is:

1) Package name checks generally don't work very well - there may be
variants for different devices or beta channels.
2) JS library developers should still be concerned about out-of-date
WebView libraries anyways.

This should be fixed for version N and above, so we
limit the workaround to Android M and below.

BUG=698752, 699943, 698682
==========

[Changwan Ryu, 2017-04-01 00:07:56]

https://codereview.chromium.org/2776073002/diff/80001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2776073002/diff/80001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:177:
final String samsungEmailPackageId = "com.samsung.android.email.provider";
On 2017/03/31 23:40:21, aelias wrote:
> I don't think an APK name check is called for here.  Name-checks can cause
> mysterious problems (e.g. beta channel or derivative fork behaving different
> from main one) so I think they're only appropriate for particularly
unsafe/crazy
> hacks.  Since this is just restoring the previous behavior that we shipped
> before, I think it's cleaner to make it global all WebView with low enough
> targetSdkVersion.  We can see that as a slower, safer way of rolling out this
> breaking change.

Done. Also checking with Samsung about the package name variants.

[commit-bot: I haz the power, 2017-04-01 00:44:01]

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

[commit-bot: I haz the power, 2017-04-01 00:44:02]

Dry run: 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...)

[Changwan Ryu, 2017-04-01 01:05:20]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-01 01:05:50]

Dry run: 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, 2017-04-01 03:05:50]

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

[commit-bot: I haz the power, 2017-04-01 03:05:51]

Dry run: This issue passed the CQ dry run.

[tkent, 2017-04-03 02:01:16]

third_party/WebKit lgtm.

https://codereview.chromium.org/2776073002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Settings.json5 (right):

https://codereview.chromium.org/2776073002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Settings.json5:929: // Whether we should
not update selection attributes when mutating selection range.
We should have a comment that we should remove the flag when we drop support of
Android M(?) or older.

[Changwan Ryu, 2017-04-03 15:31:21]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 15:31:45]

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

[Changwan Ryu, 2017-04-03 15:31:49]

https://codereview.chromium.org/2776073002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Settings.json5 (right):

https://codereview.chromium.org/2776073002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Settings.json5:929: // Whether we should
not update selection attributes when mutating selection range.
On 2017/04/03 02:01:16, tkent wrote:
> We should have a comment that we should remove the flag when we drop support
of
> Android M(?) or older.

Done.

[commit-bot: I haz the power, 2017-04-03 17:41:37]

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

[commit-bot: I haz the power, 2017-04-03 17:41:39]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2017-04-03 17:48:16]

aelias@chromium.org changed reviewers:
+ avi@chromium.org

[aelias_OOO_until_Jul13, 2017-04-03 17:48:31]

aelias@chromium.org changed reviewers:
- avi@chromium.org

[aelias_OOO_until_Jul13, 2017-04-03 17:48:32]

lgtm, adding avi@ for trivial settings change in content/common/public/

[sgurun-gerrit only, 2017-04-04 15:07:22]

it is not super clear in the thread: Is it fixed on N because Samsung changed
their app?

Now we are introducing two paths, one for older devices < M and one for newer
devices. Are there test coverage for both?

[sgurun-gerrit only, 2017-04-04 15:07:25]

it is not super clear in the thread: Is it fixed on N because Samsung changed
their app?

Now we are introducing two paths, one for older devices < M and one for newer
devices. Are there test coverage for both?

[Changwan Ryu, 2017-04-04 18:44:16]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 18:45:09]

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

[Changwan Ryu, 2017-04-04 18:45:57]

On 2017/04/04 15:07:25, sgurun wrote:
> it is not super clear in the thread: Is it fixed on N because Samsung changed
> their app?
> 
> Now we are introducing two paths, one for older devices < M and one for newer
> devices. Are there test coverage for both?

Samsung confirmed that it is fixed on N: please check b/36460657 .
I just added AwSettingsTest to cover the two paths.

[Changwan Ryu, 2017-04-04 18:46:59]

Description was changed from

==========
Workaround for Samsung email issues

Samsung's email works around Range.getClientRects()'s behavior to return
an empty sequence for a collapsed range, and the workaround is to insert
a non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. With the changes introduced in
https://codereview.chromium.org/2687273002, now inserting a non-zero
space triggers a selectionchanged event and it causes an infinite loop,
leading to issues with scroll, caret blink, and paste.

Because it is difficult for Samsung to update their email any time soon,
we undo the above changes only for WebView by adding a runtime
flag and set it. The rationale for generalizing this to WebView is:

1) Package name checks generally don't work very well - there may be
variants for different devices or beta channels.
2) JS library developers should still be concerned about out-of-date
WebView libraries anyways.

This should be fixed for version N and above, so we
limit the workaround to Android M and below.

BUG=698752, 699943, 698682
==========

to

==========
Workaround for Samsung email issues

Samsung's email works around Range.getClientRects()'s behavior to return
an empty sequence for a collapsed range, and the workaround is to insert
a non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. With the changes introduced in
https://codereview.chromium.org/2687273002, now inserting a non-zero
space triggers a selectionchanged event and it causes an infinite loop,
leading to issues with scroll, caret blink, and paste.

Because it is difficult for Samsung to update their email any time soon,
we undo the above changes only for WebView by adding a runtime
flag and set it. The rationale for generalizing this to WebView is:

1) Package name checks generally don't work very well - there may be
variants for different devices or beta channels.
2) JS library developers should still be concerned about out-of-date
WebView libraries anyways.

This should be fixed for version N and above, so we
limit the workaround to Android M and below.

BUG=698752, 699943
==========

[commit-bot: I haz the power, 2017-04-04 20:24:40]

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

[commit-bot: I haz the power, 2017-04-04 20:24:41]

Dry run: This issue passed the CQ dry run.

[sgurun-gerrit only, 2017-04-04 20:32:35]

lgtm

[Changwan Ryu, 2017-04-05 14:18:15]

changwan@chromium.org changed reviewers:
+ avi@chromium.org

[Changwan Ryu, 2017-04-05 14:18:16]

avi@chromium.org: could you take a look at content/public?

[Changwan Ryu, 2017-04-05 14:20:03]

changwan@chromium.org changed reviewers:
+ dcheng@chromium.org

[Changwan Ryu, 2017-04-05 14:20:04]

dcheng@, could you take a look at
content/public/common/common_param_traits_macros.h ?

[Avi (use Gerrit), 2017-04-05 14:30:01]

lgtm with issue addressed.

https://codereview.chromium.org/2776073002/diff/160001/content/public/common/...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2776073002/diff/160001/content/public/common/...
content/public/common/web_preferences.h:290: bool
do_not_update_selection_on_mutating_selection_range;
Needs a comment explaining what this does, why this is here, and when we're
removing it. (We are eventually removing this, yes? Do we work around Samsung
forever?)

[dcheng, 2017-04-05 17:07:31]

ipc lgtm

i'm sad to see we're adding another webview specific setting though--do we have
some way of tracking these and eventually removing obsolete WebSettings?

[Changwan Ryu, 2017-04-05 17:29:37]

dcheng@: this type of webview-specific workarounds are clustered in
WebViewChromium.java, and should be removed altogether once
Build.VERSION_CODES.N becomes meaningless in our code. (I know, it's quite
unfortunate to have these in the first place...)

https://codereview.chromium.org/2776073002/diff/160001/content/public/common/...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2776073002/diff/160001/content/public/common/...
content/public/common/web_preferences.h:290: bool
do_not_update_selection_on_mutating_selection_range;
On 2017/04/05 14:30:01, Avi (slow and ooo 10-23 April) wrote:
> Needs a comment explaining what this does, why this is here, and when we're
> removing it. (We are eventually removing this, yes? Do we work around Samsung
> forever?)

Done.

[Changwan Ryu, 2017-04-05 17:30:55]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2017-04-05 17:30:56]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
aelias@chromium.org, sgurun@chromium.org, avi@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2776073002/#ps180001
(title: "add a comment to web_preferences.h")

[commit-bot: I haz the power, 2017-04-05 17:31:35]

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, 2017-04-05 19:20:58]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491413455584490,
"parent_rev": "3d0402e4e0c59f228bedb8e20a62de48a8267a6f", "commit_rev":
"e54ccdb9fbd84362cba3c5858deaad215e572f5c"}

[commit-bot: I haz the power, 2017-04-05 19:21:59]

Description was changed from

==========
Workaround for Samsung email issues

Samsung's email works around Range.getClientRects()'s behavior to return
an empty sequence for a collapsed range, and the workaround is to insert
a non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. With the changes introduced in
https://codereview.chromium.org/2687273002, now inserting a non-zero
space triggers a selectionchanged event and it causes an infinite loop,
leading to issues with scroll, caret blink, and paste.

Because it is difficult for Samsung to update their email any time soon,
we undo the above changes only for WebView by adding a runtime
flag and set it. The rationale for generalizing this to WebView is:

1) Package name checks generally don't work very well - there may be
variants for different devices or beta channels.
2) JS library developers should still be concerned about out-of-date
WebView libraries anyways.

This should be fixed for version N and above, so we
limit the workaround to Android M and below.

BUG=698752, 699943
==========

to

==========
Workaround for Samsung email issues

Samsung's email works around Range.getClientRects()'s behavior to return
an empty sequence for a collapsed range, and the workaround is to insert
a non-zero space, and call Range.getClientRects(), and then remove the
non-zero space. With the changes introduced in
https://codereview.chromium.org/2687273002, now inserting a non-zero
space triggers a selectionchanged event and it causes an infinite loop,
leading to issues with scroll, caret blink, and paste.

Because it is difficult for Samsung to update their email any time soon,
we undo the above changes only for WebView by adding a runtime
flag and set it. The rationale for generalizing this to WebView is:

1) Package name checks generally don't work very well - there may be
variants for different devices or beta channels.
2) JS library developers should still be concerned about out-of-date
WebView libraries anyways.

This should be fixed for version N and above, so we
limit the workaround to Android M and below.

BUG=698752, 699943

Review-Url: https://codereview.chromium.org/2776073002
Cr-Commit-Position: refs/heads/master@{#462173}
Committed:
https://chromium.googlesource.com/chromium/src/+/e54ccdb9fbd84362cba3c5858dea...
==========

[commit-bot: I haz the power, 2017-04-05 19:22:01]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/e54ccdb9fbd84362cba3c5858dea...