Please find the Video & Logcat @ go/chrome-androidlogs1/6/698752

changwan@,

Since it is Samsung, we cant get per-cl bisect. Could you look into this issue?
Could this be possible suspect : https://chromium.googlesource.com/chromium/src/+/8f2a1baf12aa71d59d72e24583983b8a6b78340a ?

Thanks!

satyavathir@, I thought we have one userdebug Samsung phone, do we still have one? If we have one we should be able to do per-cl bisect...

Regarding bisect range, cc'ing yosin@ because https://codereview.chromium.org/2680943004 is in the range.

It seems from the video that some JavaScript event listener is causing the webpage to scroll back to the original offset. Or somehow looking up the selection after scrolling is causing another scrolling internally, for example, because IME sets the selection again.

yosin@, any idea on what might be happening here?

Maybe it would repro with the same Samsung Email apk on a non-Samsung device.  For example there are some here: http://www.apkmirror.com/?s=samsung+email&post_type=app_release&searchtype=apk

changwan@ - yes, we have 2 Samsung devices/L-eng builds with Test keys. I cant install anything like WebView stand alone (or) do much.

We will try #6 - aelias@ suggested apk's on non-samsung devices and update the bug.

Thanks!

Issue 699047 has been merged into this issue.

It looks like I can't sideload the Samsung Email APK on a Nexus device, I get INSTALL_FAILED_MISSING_SHARED_LIBRARY.

Yeah, the vendor preinstalled APKs almost always depend on vendor-specific libraries that prevent them being used on other devices (sometimes even not working on other devices of that same vendor - they're really only intended for the specific devices they are preinstalled on) :/

My theory is that Samsung Email app probably has some JS to scroll the viewport to the current selection position.  It's intended to run in specific narrow situations (for example, after text was just pasted).  But it seems Samsung email app is now in an infinite churn constantly thinking that the selection position just changed and that it needs to scroll to it.

This does not reproduce when I locally revert https://codereview.chromium.org/2680943004 from 58.0.3.3013.

As mentioned in #13, Samsung indeed does some scrolling on selection changes.
This code path was analyzed in  https://bugs.chromium.org/p/chromium/issues/detail?id=699943#c7

But it is not very clear why scrolling causes selection changes.

yosin@, do you have anything to suspect here? Before we bring this to Samsung's attention, I'll try to create a reduced APK that simulates Samsung behavior to pinpoint the problem.

Actually satyavathir@ just told me that M58 beta push is scheduled on Wednesday. yosin@, in case we cannot figure this out until Wednesday, can we consider reverting it for M58 beta and root-cause this on M59? That will buy us more time for testing before shipping it out.

Ok, as shown in the video at #1, the reply message has the following structure:

============================================
New message

Signature

--------------------------------------------
Original Message
--------------------------------------------
From: abc@def.com
Date: 3/17/17
To: zyx@wv.com
Subject:
<Content of the original message>
============================================

In Samsung's email, New Message and Original Message are separate WebView objects.

If you move focus from New Message to Original Message (either by selecting all or simply touching Original Message), then the New Message WebView becomes unfocused.

After we landed https://codereview.chromium.org/2680943004, even though the new message part was unfocused, the Selection and range is still available, and that seems to confuse Samsung email.

yosin@, the last part conforms to the Selection APIs, right? If so could you point to the reference about unfocused selection? Once I get it, I'll file a bug against Samsung and close this one.

>After we landed https://codereview.chromium.org/2680943004, even though the new >message part was unfocused, the Selection and range is still available, and that >seems to confuse Samsung email.

Unfocusing frame doesn't change selection. I think the patch doesn't change this.

The patch changes equality of selection.
Before the patch, it compares VisibleSelection == specified position + canonicalization.
After the path, it compares SelecitonInDOMTree.

I guess before this patch, when selection is the bottom of frame, canonicalization may extend selection to out side of frame edge. Or, App gets selection change events by specifying extra Selection#addRange() calls[2] but it is different from VS.

BTW, I could not scroll in Google Inbox app in viewing mode. It seems Samsung Email App attempt to solve this?

[2] crbug.com/699943 ⚐	[OEM Mail] [Samsung] Cursor blinks stops when we enter next line

Because this issue is severe, it is probably not enough to just punt it onto Samsung and close this.  Even if Samsung fixes their app in time, the regression would still affect phones that got a WebView update but not a Samsung email update (often these OEM stock apps are non-updatable).  So we should find a workaround on our end to restore the previous behavior.

I understand the previous behavior is considered buggy and not spec-compliant, in which case we have a tool "targetSdkVersion" to roll out such changes without breaking existing apps.  We can apply the workaround for apps specifying targetSdkVersion <= Android N, and apply the web-compliant behavior for apps with higher targetSdkVersion.  This is a number in the app manifest that developers control, so that end-users would never be exposed to the problem.

Re #18, yes I agree. I'll try to find a workaround on our end if possible.

BTW, I couldn't debug the apk because of the following issues:
1) Dev tools don’t work correctly. JS files are not added to sources section, so I cannot debug them.
2) Stack tools don’t work correctly. Possibly because it’s Samsung, possibly because it’s 64-bit.
3) selectionchange does not have an entry in sources -> Event Listener Breakpoints.

I had to rely on static analysis and 'pause', which gives the current line it is running. From there on, I can step over / step into.

In any case, after some trials and errors, I was able to reproduce this issue (and issue 699943).

https://jsfiddle.net/galmacky/aqvwdzwe/6/

I think the scrolling logic is there because of the following use case: you type a letter, and then scroll down until caret disappears from the screen, and then type another letter. In this case, Samsung seems to want to show the caret on the screen. But somehow, they get infinite selectionchange events, and that's causing scroll block and other nasty issues.

The problematic logic is in SelectionController.js's getSelectionInfo():

var rectList = range.getClientRects();
if (rectList.length == 0) {
 // Insert a zero-width space to range, and run range.getClientRects() again.
 // Delete the zero-width space.
}
rectList = range.getCilentRects();

Previously, inserting and deleting a zero-width space did not seem to cause selectionchange event. That or Range.getClientRects() returns null in a different situation.

I think this is obviously Samsung's own bug and indiscretion to have this nasty code, but I'll check if we can land a workaround for this.

It seems the selection ultimately did not change so it could be considered our bug.  Perhaps if we aggregate the selection changes and wait until JS releases the message loop before comparing before and after, this workaround might be web-standard compatible and we can avoid targetSdkVersion?

I need to understand why this type of hack is needed.

yosin@, tkent@, does it make sense for Range.getClientRects() to return null even when the caret is blinking? This is observed even on desktop when I reload the url in #19 and first focus on the content editable.

Re #20, suppressing the event may change selectionchange event behavior.
According to http://w3c.github.io/selection-api/#selectionchange-event,
the user agent must queue a task to fire selectionchange event.

I think the original intention is to make each selection change accountable and allow JS to handle them separately.

> I think the original intention is to make each selection change accountable and allow JS to handle them separately.

I don't think that could solve any use case because they would both be fired after JS has released message loop, not at an appropriate timing.  We could finesse the spec text question by saying this situation is not really a "mutation" in the first place since nothing changed in the end.

Re #23, that's very true. tkent@, yosin@, what do you think?

Also another approach I can think of is to fix Range.getClientRects() to return non-null when it should. Let me check if that can alleviate the symptoms in this case.

Thanks for investigation and creating sample[1]!

TL;DR: We should ask Samsung to change their implementation of getSelectionInfo().


The sample[1] dispatches "selectionchange" event repeatedly on both Chrome Canary and Firefox.

This is caused by *live* Range object modification; *live* Range means change of *live* Range changes selection.
 - var range = sel.getRangeAt(0); // Get *live* range of selection
 - range.insertNode(span); // Increase offset of Range.{start,end}Offset == Move selection -> "selectionchange"
 - range.selectNode(span); // Change selection to cover SPAN with U+200B -> "selectionchange"
 - range.deleteContents(); // Remove SPAN == Move selection to before SPAN -> "selectionchange"

I think f we use non-live Range for this hack, it might work, e.g.

var range = sel.getRangeAt(0).cloneRange();

We may want to change the spec[2] for Range#getClientRects().length === 0 for collapsed Range[3] and
empty element case.

[1] https://jsfiddle.net/galmacky/aqvwdzwe/6/ In #c9
[2] https://drafts.csswg.org/cssom-view/#extensions-to-the-range-interface Range#getClientRects()
[3]  crbug.com/238976  ⚐	GetBoundingRect for Selection type Caret returns always empty rectangle.

Hmm... Regarding the last paragraph at #25,

Could you explain how the spec translates to collapsed range and empty element cases? The range is still in the doc, so to me it sounds as if we should return one empty rect.

BTW, I think it is better to provide auto scroll like functionality for touch UI.
In Docs app, when I extend the selection, it automatically scroll up when selection
handle near the edge.

>Could you explain how the spec translates to collapsed range and empty element >cases? The range is still in the doc, so to me it sounds as if we should return one >empty rect.

Returning empty list of Range#getClientRects() is a bug of Blink.

I'm not clear in the spec[1], for the case <div>|</div>. It seems current impl returns (0,0)x(0,0) then returning empty rect list.

I found feature request[1].

[1] httP//crbug.com/697001 Selection handle near edge of scrollable should start autoscroll

I've uploaded a preliminary CL before adding tests: https://codereview.chromium.org/2776073002/

yosin@ & aelias@, does this make sense? This issue and issue crbug.com/699943 disappear when testing locally.

The patch[1] mentioned in #c30 to change Range#getClientRects() to avoid Samsung's
hack which fires "selectionchange".

>#c30, yosin@ & aelias@, does this make sense?
Yes, it is interesting approach.

[1] http://crrev.com/2776073002: Make getClientRects return non-null object for empty Range

yosin@, on code review, asked me to test this on other browsers, and here are the results:

A. Chrome 57
The behavior is different even within Chrome 57 when div content is truly empty vs when div content is \n\n.

A.1. When DIV is empty: https://jsfiddle.net/galmacky/aqvwdzwe/17/
Range#getClientRects() returns a sequence of ClientRect (width=0, height=18).

A.2. When DIV is \n\n: https://jsfiddle.net/galmacky/aqvwdzwe/16/
Range#getClientRects() returns an empty sequence.

B. Mac Safari
The same behavior as in Chrome 57.

C. Mac/Android Firefox
You can't click on the DIV either on 1), 2). But if you type 'hello' in the DIV content and click on DIV, and then delete it, Range#getClientRects() returns an empty sequence.

D. Windows Internet Explorer 11

You get a sequence of ClientRect(width = 0, height = 18) for both cases as in A.1 and A.2.

E. Windows Edge
I could not test because I've uninstalled it on my personal laptop.

A.3. When DIV is \n\n\, but type 'a' and then delete it using backspace
https://jsfiddle.net/galmacky/aqvwdzwe/16/

Range#getClientRects() returns a sequence of ClientRect (width=527, height=18).

My CL (https://codereview.chromium.org/2776073002/) proposes that we handle A.1. in the same way as A.3 as a bandaid.

I just tested against Windows Edge.

E. Windows Edge

Same as D, you get a sequence of ClientRect(width = 0, height = 18) for both cases as in A.1 and A.2.

We are about to change behavior of Range#getClientRects()[1] for collapsed
Range not to return empty list. 

It seems the spec[1] doesn't explicitly specify behavior of collapsed Range on Range#getClientRects(), but it can read we should return non-empty client rects list
rather than empty list.

foolip@, Do you think it is worth to change Blink behavior not to return non-empty list for connected collapsed Range? Event if Edge, Firefox, Safari, do not so?

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

It seems patch[1] helps Samsung but it affects others which assumes Range#getClientRects() returns zero.

How difficult to ask Samsung to change their code?


[1] http://crrev.com/2776073002: Make getClientRects return non-null object for collapsed Range

It's too difficult.  They can change it but not necessarily deploy it.  If the web-facing change is unacceptable moving forward, we can guard the workaround with targetSdkVersion as I mentioned in #18.  Although I'm not sure yet it actually would be unacceptable (these "others" seem hypothetical right now?).

It looks to me like the spec needs to be clarified, and that discussion needs to be informed by current behavior. If I understand this correctly, the problem is with a collapsed range. https://jsbin.com/jodiharuni/edit?html,output is my attempt at testing this, and that test passes only in Gecko.

Please file bugs at https://github.com/w3c/csswg-drafts/labels/cssom-view-1

Does this issue need Restrict-View-Google?

I don't think so, there's an internal link in #1 but it won't work and the link itself doesn't reveal anything interesting.

Thanks, foolip@, for the tests. But I think the expected size of the returned list should arguably be 1, not 0.

I just picked a number for what I thought would happen, I didn't think through what the behavior should be. If the spec ends up requiring 1, that's fine.

Re #40, I've filed a bug at https://github.com/w3c/csswg-drafts/issues/1156 .

I just tested against Samsung S7 edge on Android N.

package name: com.samsung.android.email.provider
package versionName: 4.0.43-1
against Chrome Canary / Dev (59.0.3056.4), but I could not reproduce this issue with both versions. So this indeed does not happen on N.

FYI, on M device that I'm testing, the package versionName is 3.1.18.

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

commit e54ccdb9fbd84362cba3c5858deaad215e572f5c
Author: changwan <changwan@chromium.org>
Date: Wed Apr 05 19:20:54 2017

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}

[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/android_webview/java/src/org/chromium/android_webview/AwSettings.java
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/android_webview/native/aw_settings.cc
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellActivity.java
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/content/public/common/common_param_traits_macros.h
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/content/public/common/web_preferences.cc
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/content/public/common/web_preferences.h
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/content/renderer/render_view_impl.cc
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/third_party/WebKit/Source/core/dom/Range.cpp
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/third_party/WebKit/Source/core/frame/Settings.json5
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/third_party/WebKit/Source/web/WebSettingsImpl.cpp
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/third_party/WebKit/Source/web/WebSettingsImpl.h
[modify] https://crrev.com/e54ccdb9fbd84362cba3c5858deaad215e572f5c/third_party/WebKit/public/web/WebSettings.h

requesting merge of the above to M58

Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit deb6d19e2d5be3f8f13fcefdaf20a67743bcc207
Author: Changwan Ryu <changwan@chromium.org>
Date: Wed Apr 05 20:52:24 2017

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}
(cherry picked from commit e54ccdb9fbd84362cba3c5858deaad215e572f5c)

Review-Url: https://codereview.chromium.org/2800683003 .
Cr-Commit-Position: refs/branch-heads/3029@{#594}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/android_webview/java/src/org/chromium/android_webview/AwSettings.java
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/android_webview/native/aw_settings.cc
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellActivity.java
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/content/public/common/common_param_traits_macros.h
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/content/public/common/web_preferences.cc
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/content/public/common/web_preferences.h
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/content/renderer/render_view_impl.cc
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/third_party/WebKit/Source/core/dom/Range.cpp
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/third_party/WebKit/Source/core/frame/Settings.json5
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/third_party/WebKit/Source/web/WebSettingsImpl.cpp
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/third_party/WebKit/Source/web/WebSettingsImpl.h
[modify] https://crrev.com/deb6d19e2d5be3f8f13fcefdaf20a67743bcc207/third_party/WebKit/public/web/WebSettings.h

Unable to repro on latest M58: 58.0.3029.56
Tested devices:Samsung On Nxt / MMB29K,Samsung Galaxy S7 / MMB29K

Thanks!