Prevent AutoscrollForSelection when selecting text in a fixed-position element.

Prevent AutoscrollForSelection when selecting text in a fixed-position element.

AutoscrollForSelection is triggered when selecting text and dragging mouse out
of the container. However, we don't want selecting text in a fixed-position
element triggers autoscroll otherwhere. Inside findAutoscrollable, when we find
the object is inside a fixed-position element, we should prevent
AutoscrollForSelection from happening.

BUG=655489
Committed: https://crrev.com/53546e297f782849d9da8004508d0b85c2fd931c
Cr-Commit-Position: refs/heads/master@{#427146}

[sunyunjia, 2016-10-20 17:25:23]

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

[commit-bot: I haz the power, 2016-10-20 17:25:57]

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

[sunyunjia, 2016-10-20 17:34:36]

Description was changed from

==========
Prevent pdf autoscroll when selecting the text in the tool bar.

BUG=655489
==========

to

==========
Prevent pdf autoscroll when selecting the text in the tool bar.

AutoscrollForSelection is triggered when the selection is within a borderbelt
of the window (See https://codereview.chromium.org/2355643002/). However, the
pdf toolbar is in that region and we don't want the page autoscrolled by
selecting the text in the toolbar. This patch disables autoscroll from any
node inside the "PAPER-TOOLBAR" element.

BUG=655489
==========

[sunyunjia, 2016-10-20 17:34:36]

sunyunjia@chromium.org changed reviewers:
+ bokan@chromium.org

[sunyunjia, 2016-10-20 18:56:25]

Hi David,

Can you please take a look at this? Also, can you give me some suggestions on
how to write tests for the pdf, especially how to get the scrollTop and
scrollHeight properties of the document? Thanks!

[commit-bot: I haz the power, 2016-10-20 18:57:22]

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

[commit-bot: I haz the power, 2016-10-20 18:57:23]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-10-21 14:36:30]

Hi Sandra,

I don't think this is a good way to fix this, it fixes this very specific case
but the larger problem remains (not to mention that we'd be arbitrarily breaking
any page that uses a "PAPER-TOOLBAR" node).

I think the real problem in the bug is that we're autoscrolling even though
we're highlighting text in a position: fixed element. This means that scrolling
doesn't affect the selected text, this could be a problem beyond just the PDF
viewer. There's code in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
to handle this case. My advice would be to investigate why that's not covering
this case. 

Thanks

[sunyunjia, 2016-10-21 18:32:10]

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

[commit-bot: I haz the power, 2016-10-21 18:33:07]

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

[sunyunjia, 2016-10-21 18:58:15]

Description was changed from

==========
Prevent pdf autoscroll when selecting the text in the tool bar.

AutoscrollForSelection is triggered when the selection is within a borderbelt
of the window (See https://codereview.chromium.org/2355643002/). However, the
pdf toolbar is in that region and we don't want the page autoscrolled by
selecting the text in the toolbar. This patch disables autoscroll from any
node inside the "PAPER-TOOLBAR" element.

BUG=655489
==========

to

==========
Prevent AutoscrollForSelection when selecting text in a fixed-position element.

AutoscrollForSelection is triggered when selecting text and dragging mouse out
of the container. However, we don't want selecting text in a fixed-position
element triggers autoscroll otherwhere. Inside findAutoscrollable, when we find
the object is inside a fixed-position element, we should prevent
AutoscrollForSelection from happening.

BUG=655489
==========

[commit-bot: I haz the power, 2016-10-21 20:28:35]

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

[commit-bot: I haz the power, 2016-10-21 20:28:36]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2016-10-24 13:09:35]

PTAL, thanks!

[bokan, 2016-10-24 13:22:00]

Thanks, this looks better.

https://codereview.chromium.org/2441683002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/events/autoscroll-disabled-in-fix.html
(right):

https://codereview.chromium.org/2441683002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/autoscroll-disabled-in-fix.html:18:
document.body.offsetTop;
You shouldn't need this. If you're finding that layout hasn't occurred yet, you
can put all this inside a handler for the load event.

https://codereview.chromium.org/2441683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2441683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1012: if
(layoutObject->isBox() && toLayoutBox(layoutObject)->hasLayer() &&
Could you confirm that a scroller inside a position: fixed can still be auto
scrolled? e.g.

<div style="position: fixed">
  <div style="overflow: scroll">
    Selecting this text should allow autoscrolling within the overflow div
  </div>
</div>

[sunyunjia, 2016-10-24 14:10:25]

https://codereview.chromium.org/2441683002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/events/autoscroll-disabled-in-fix.html
(right):

https://codereview.chromium.org/2441683002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/autoscroll-disabled-in-fix.html:18:
document.body.offsetTop;
On 2016/10/24 13:22:00, bokan wrote:
> You shouldn't need this. If you're finding that layout hasn't occurred yet,
you
> can put all this inside a handler for the load event.

Done.

https://codereview.chromium.org/2441683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2441683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1012: if
(layoutObject->isBox() && toLayoutBox(layoutObject)->hasLayer() &&
Confirmed!

[sunyunjia, 2016-10-24 14:10:31]

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

[commit-bot: I haz the power, 2016-10-24 14:10:50]

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

[bokan, 2016-10-24 14:11:30]

lgtm % nit

https://codereview.chromium.org/2441683002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2441683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1014: return 0;
nit: return nullptr

[sunyunjia, 2016-10-24 14:12:17]

The CQ bit was unchecked by sunyunjia@chromium.org

[sunyunjia, 2016-10-24 14:18:21]

The CQ bit was checked by sunyunjia@chromium.org

[sunyunjia, 2016-10-24 14:18:22]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2441683002/#ps60001
(title: "Change return value from 0 to nullptr.")

[commit-bot: I haz the power, 2016-10-24 14:18:38]

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-10-24 14:45:03]

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

[commit-bot: I haz the power, 2016-10-24 14:45:05]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sunyunjia, 2016-10-24 14:59:11]

The CQ bit was checked by sunyunjia@chromium.org

[sunyunjia, 2016-10-24 14:59:12]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2441683002/#ps80001
(title: "Format")

[commit-bot: I haz the power, 2016-10-24 14:59:41]

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-10-24 16:11:50]

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

[commit-bot: I haz the power, 2016-10-24 16:11:50]

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

[sunyunjia, 2016-10-24 16:44:10]

The CQ bit was checked by sunyunjia@chromium.org

[commit-bot: I haz the power, 2016-10-24 16:44:36]

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-10-24 17:41:58]

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

[commit-bot: I haz the power, 2016-10-24 17:41:59]

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

[sunyunjia, 2016-10-24 17:57:13]

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

[commit-bot: I haz the power, 2016-10-24 17:57:43]

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, 2016-10-24 20:45:37]

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

[commit-bot: I haz the power, 2016-10-24 20:45:38]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2016-10-24 20:46:01]

The CQ bit was checked by sunyunjia@chromium.org

[commit-bot: I haz the power, 2016-10-24 20:46:45]

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-10-24 21:01:01]

Description was changed from

==========
Prevent AutoscrollForSelection when selecting text in a fixed-position element.

AutoscrollForSelection is triggered when selecting text and dragging mouse out
of the container. However, we don't want selecting text in a fixed-position
element triggers autoscroll otherwhere. Inside findAutoscrollable, when we find
the object is inside a fixed-position element, we should prevent
AutoscrollForSelection from happening.

BUG=655489
==========

to

==========
Prevent AutoscrollForSelection when selecting text in a fixed-position element.

AutoscrollForSelection is triggered when selecting text and dragging mouse out
of the container. However, we don't want selecting text in a fixed-position
element triggers autoscroll otherwhere. Inside findAutoscrollable, when we find
the object is inside a fixed-position element, we should prevent
AutoscrollForSelection from happening.

BUG=655489
==========

[commit-bot: I haz the power, 2016-10-24 21:01:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-24 21:04:17]

Description was changed from

==========
Prevent AutoscrollForSelection when selecting text in a fixed-position element.

AutoscrollForSelection is triggered when selecting text and dragging mouse out
of the container. However, we don't want selecting text in a fixed-position
element triggers autoscroll otherwhere. Inside findAutoscrollable, when we find
the object is inside a fixed-position element, we should prevent
AutoscrollForSelection from happening.

BUG=655489
==========

to

==========
Prevent AutoscrollForSelection when selecting text in a fixed-position element.

AutoscrollForSelection is triggered when selecting text and dragging mouse out
of the container. However, we don't want selecting text in a fixed-position
element triggers autoscroll otherwhere. Inside findAutoscrollable, when we find
the object is inside a fixed-position element, we should prevent
AutoscrollForSelection from happening.

BUG=655489

Committed: https://crrev.com/53546e297f782849d9da8004508d0b85c2fd931c
Cr-Commit-Position: refs/heads/master@{#427146}
==========

[commit-bot: I haz the power, 2016-10-24 21:04:18]

Patchset 5 (id:??) landed as
https://crrev.com/53546e297f782849d9da8004508d0b85c2fd931c
Cr-Commit-Position: refs/heads/master@{#427146}