Discard scrolling to the saved scroll points if there is any selection done by user.

Discard scrolling to the saved scroll points if there is any selection done by user.

BUG=458429
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192799

[pals, 2015-02-12 12:06:27]

Patchset #2 (id:20001) has been deleted

[pals, 2015-02-12 12:06:33]

Patchset #2 (id:40001) has been deleted

[pals, 2015-02-12 12:06:38]

Patchset #2 (id:60001) has been deleted

[pals, 2015-02-12 12:06:43]

Patchset #2 (id:80001) has been deleted

[pals, 2015-02-12 12:06:51]

Patchset #2 (id:100001) has been deleted

[pals, 2015-02-13 05:46:13]

sanjoy.pal@samsung.com changed reviewers:
+ tkent@chromium.org

[pals, 2015-02-13 05:54:06]

I could find two ways to fix this issue

Patch set 1: Discard scrolling to the saved scroll points if there is any
selection done by user.

Cons: Improved but there is still some flickers because
restoreScrollPositionAndViewState() is called first before revealSelection(). So
page first jumps to saved scroll position and then scrolls to the focused
element. Firefox behavior is similar to this.

Patch set 2: Ignore scrolling to the focused element if the page already
scrolled to stored scroll position while page is loading.

Cons: 2 layout tests were failing because <div id="copy"> was placed at the
bottom of the page. So, when getElementById("copy").focus() happens, page scroll
position is stored in history which is same as current scroll position. So, the
input element was not revealed.

[tkent, 2015-02-13 05:56:24]

tkent@chromium.org changed reviewers:
+ yoichio@chromium.org, yosin@chromium.org
- tkent@chromium.org

[tkent, 2015-02-13 05:56:25]

I'm not familiar with FrameSelection.
-tkent
+yosin, yoichio

[yoichio, 2015-02-13 06:32:08]

Do you intend to avoid autofocus or some scrolling with js in loading?

[yoichio, 2015-02-13 06:32:18]

https://codereview.chromium.org/914573004/diff/120001/LayoutTests/editing/inp...
File LayoutTests/editing/input/reveal-contenteditable-on-paste-vertically.html
(right):

https://codereview.chromium.org/914573004/diff/120001/LayoutTests/editing/inp...
LayoutTests/editing/input/reveal-contenteditable-on-paste-vertically.html:11:
<div contenteditable="true" id="copy">WebKit</div>
Why do you need to update these test? Looks not related to historical scrolling.

[pals, 2015-02-13 06:35:33]

On 2015/02/13 06:32:18, yoichio wrote:
>
https://codereview.chromium.org/914573004/diff/120001/LayoutTests/editing/inp...
> File LayoutTests/editing/input/reveal-contenteditable-on-paste-vertically.html
> (right):
> 
>
https://codereview.chromium.org/914573004/diff/120001/LayoutTests/editing/inp...
> LayoutTests/editing/input/reveal-contenteditable-on-paste-vertically.html:11:
> <div contenteditable="true" id="copy">WebKit</div>
> Why do you need to update these test? Looks not related to historical
scrolling.

layout tests were failing because <div id="copy"> was placed at the
bottom of the page. So, when getElementById("copy").focus() happens, page scroll
position is stored in history which is same as current scroll position. So, the
input element was not revealed.

[pals, 2015-02-13 06:36:34]

On 2015/02/13 06:32:08, yoichio wrote:
> Do you intend to avoid autofocus or some scrolling with js in loading?
I could find two ways to fix this issue

Patch set 1: Discard scrolling to the saved scroll points if there is any
selection done by user.

Cons: Improved but there is still some flickers because
restoreScrollPositionAndViewState() is called first before revealSelection(). So
page first jumps to saved scroll position and then scrolls to the focused
element. Firefox behavior is similar to this.

Patch set 2: Ignore scrolling to the focused element if the page already
scrolled to stored scroll position while page is loading.

Cons: 2 layout tests were failing because <div id="copy"> was placed at the
bottom of the page. So, when getElementById("copy").focus() happens, page scroll
position is stored in history which is same as current scroll position. So, the
input element was not revealed.

[yoichio, 2015-02-13 07:10:09]

On 2015/02/13 06:36:34, pals wrote:
> On 2015/02/13 06:32:08, yoichio wrote:
> > Do you intend to avoid autofocus or some scrolling with js in loading?
> I could find two ways to fix this issue
> 
> Patch set 1: Discard scrolling to the saved scroll points if there is any
> selection done by user.
> 
> Cons: Improved but there is still some flickers because
> restoreScrollPositionAndViewState() is called first before revealSelection().
So
> page first jumps to saved scroll position and then scrolls to the focused
> element. Firefox behavior is similar to this.
Could you make layout test to fix and yet another flicking?

> 
> Patch set 2: Ignore scrolling to the focused element if the page already
> scrolled to stored scroll position while page is loading.
> 
> Cons: 2 layout tests were failing because <div id="copy"> was placed at the
> bottom of the page. So, when getElementById("copy").focus() happens, page
scroll
> position is stored in history which is same as current scroll position. So,
the
> input element was not revealed.
Does it mean if webpage focuses twice or more in loading the page does not
scroll to last
 focused element?

[pals, 2015-02-13 08:12:09]

> > page first jumps to saved scroll position and then scrolls to the focused
> > element. Firefox behavior is similar to this.
> Could you make layout test to fix and yet another flicking?

I can make a layout a layout for this.
I think it is difficult to fix the flicking due to jump from the scrolled
position to the focused element. Because the element.focus() call comes much
later, by that time page is already scrolled.

> Does it mean if webpage focuses twice or more in loading the page does not
> scroll to last
>  focused element?

Yes.

[pals, 2015-03-10 09:46:10]

Hi yoichio,

Sorry for this late update. I have added a layout test for Patch set 1 which
discards scrolling to the saved scroll points if there is any selection
happened. Please take a look.

Thanks.

[yoichio, 2015-03-11 04:16:52]

https://codereview.chromium.org/914573004/diff/140001/LayoutTests/editing/inp...
File
LayoutTests/editing/input/reveal-selection-having-stored-scroll-position.html
(right):

https://codereview.chromium.org/914573004/diff/140001/LayoutTests/editing/inp...
LayoutTests/editing/input/reveal-selection-having-stored-scroll-position.html:17:
} else {
Above if statement ends with return so we don't need this else.

https://codereview.chromium.org/914573004/diff/140001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/914573004/diff/140001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1773:
m_frame->view()->setWasScrolledByUser(true);
Could you confirm this code path is called by only user action?

[pals, 2015-03-11 06:09:28]

Please take a look.

https://codereview.chromium.org/914573004/diff/140001/LayoutTests/editing/inp...
File
LayoutTests/editing/input/reveal-selection-having-stored-scroll-position.html
(right):

https://codereview.chromium.org/914573004/diff/140001/LayoutTests/editing/inp...
LayoutTests/editing/input/reveal-selection-having-stored-scroll-position.html:17:
} else {
On 2015/03/11 04:16:52, yoichio wrote:
> Above if statement ends with return so we don't need this else.

Done.

https://codereview.chromium.org/914573004/diff/140001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/914573004/diff/140001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1773:
m_frame->view()->setWasScrolledByUser(true);
On 2015/03/11 04:16:52, yoichio wrote:
> Could you confirm this code path is called by only user action?
I am not sure about this, I am not very familiar about this part of the code and
there are many calls to revealSelection(). As there are no layout test failure,
I thought this patch can be a fix.

[yoichio, 2015-03-12 05:00:00]

Since layout test is not perfect, this can cause regression issues.
Do you think it is OK to call "setWasScrolledByUser" in the case where user
doesn't scroll?
You should confirm  or refactor/rename |setWasScrolledByUser| appropriately.

[pals, 2015-03-27 13:04:53]

On 2015/03/12 05:00:00, yoichio wrote:
> Since layout test is not perfect, this can cause regression issues.
> Do you think it is OK to call "setWasScrolledByUser" in the case where user
> doesn't scroll?
> You should confirm  or refactor/rename |setWasScrolledByUser| appropriately.

I have spent more time in debugging this issue. |revealSelection| is used called
to scroll the focused element in the viewport for user's convenience and
|wasScrolledByUser| is used to stop annoying user by programmatic scroll. I
think it is fine to call |setWasScrolledByUser| inside |revealSelection|.

[yoichio, 2015-03-30 05:33:05]

lgtm

O.K.

[pals, 2015-03-30 09:50:30]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2015-03-30 09:50:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/914573004/160001

[commit-bot: I haz the power, 2015-03-30 09:54:30]

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

[commit-bot: I haz the power, 2015-03-30 09:54:31]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/49520)

[pals, 2015-03-30 10:24:16]

Patchset #5 (id:180001) has been deleted

[pals, 2015-03-30 10:39:46]

Patchset #5 (id:200001) has been deleted

[pals, 2015-03-30 10:40:16]

The CQ bit was checked by sanjoy.pal@samsung.com

[pals, 2015-03-30 10:40:16]

The patchset sent to the CQ was uploaded after l-g-t-m from yoichio@chromium.org
Link to the patchset: https://codereview.chromium.org/914573004/#ps220001
(title: "Rebased")

[commit-bot: I haz the power, 2015-03-30 10:40:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/914573004/220001

[commit-bot: I haz the power, 2015-03-30 10:49:36]

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

[commit-bot: I haz the power, 2015-03-30 10:49:37]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[pals, 2015-03-30 11:01:43]

sanjoy.pal@samsung.com changed reviewers:
+ tkent@chromium.org, yutak@chromium.org

[pals, 2015-03-30 11:02:28]

sanjoy.pal@samsung.com changed reviewers:
- tkent@chromium.org

[pals, 2015-03-30 11:03:29]

On 2015/03/30 10:49:37, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

+yutak
Need owner's lgtm for Source/core/editing/FrameSelection.cpp.

[Yuta Kitamura, 2015-03-31 01:35:26]

lgtm

[pals, 2015-03-31 05:04:38]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2015-03-31 05:05:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/914573004/220001

[commit-bot: I haz the power, 2015-03-31 05:09:18]

Committed patchset #5 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192799