Issue 1895323002: Viewport apply scroll should be on the document element not scrollingElement.

bokan

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

4 years, 8 months ago (2016-04-18 22:53:02 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895323002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895323002/1

4 years, 8 months ago (2016-04-18 22:53:34 UTC) #2

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-18 23:07:24 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/123684)

4 years, 8 months ago (2016-04-18 23:07:25 UTC) #4

bokan

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

4 years, 8 months ago (2016-04-18 23:10:28 UTC) #5

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895323002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895323002/20001

4 years, 8 months ago (2016-04-18 23:10:51 UTC) #6

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-18 23:25:11 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/146773)

4 years, 8 months ago (2016-04-18 23:25:12 UTC) #8

bokan

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

4 years, 8 months ago (2016-04-18 23:27:04 UTC) #9

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895323002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895323002/40001

4 years, 8 months ago (2016-04-18 23:27:18 UTC) #10

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-19 00:51:13 UTC) #11

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/207138)

4 years, 8 months ago (2016-04-19 00:51:14 UTC) #12

bokan

Description was changed from ========== Viewport apply scroll should be on the document element not ...

4 years, 8 months ago (2016-04-19 14:39:33 UTC) #13

Description was changed from

==========
Viewport apply scroll should be on the document element not scrollingElement.

In r387224 I moved all the viewport scrolling actions be happen from a special
apply scroll method installed on the scrollingElement. This turned out to be
my misunderstanding of how scrollingElement and viewport scrolling in regards
to the DOM tree worked. This caused a bug whenever a page explicitly made the
<body> element scrollable as the callback would try to scroll the viewport
rather than the <body>.

The solution is to install the apply scroll on the document element (<html>).
This means that the <body> element will still be considered for scrolling while
the scroll bubbles up the DOM. An <html> element never has any overflow of its
own, even if it's explicitly marked as overflow: scroll. In most cases, the
<body> won't have an overflow clip of its own and so the scroll will defer to
the viewport. This is in line with how scrolling worked prior to my change in
r387224.

BUG=604296
==========

to

==========
Viewport apply scroll should be on the document element not scrollingElement.

In r387224 I moved all the viewport scrolling actions be happen from a special
apply scroll method installed on the scrollingElement. This turned out to be
my misunderstanding of how scrollingElement and viewport scrolling in regards
to the DOM tree worked. This caused a bug whenever a page explicitly made the
<body> element scrollable as the callback would try to scroll the viewport
rather than the <body>.

The solution is to install the apply scroll on the document element (<html>).
This means that the <body> element will still be considered for scrolling while
the scroll bubbles up the DOM. An <html> element never has any overflow of its
own, even if it's explicitly marked as overflow: scroll. In most cases, the
<body> won't have an overflow clip of its own and so the scroll will defer to
the viewport. This is in line with how scrolling worked prior to my change in
r387224.

BUG=604296
==========

bokan

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

4 years, 8 months ago (2016-04-19 14:39:34 UTC) #14

bokan

Tim, ptal. Scrolling around <body> and viewport is quite quirky...if the author gives the body ...

4 years, 8 months ago (2016-04-19 14:44:14 UTC) #15

bokan

Description was changed from ========== Viewport apply scroll should be on the document element not ...

4 years, 8 months ago (2016-04-19 14:45:06 UTC) #16

Description was changed from

==========
Viewport apply scroll should be on the document element not scrollingElement.

In r387224 I moved all the viewport scrolling actions be happen from a special
apply scroll method installed on the scrollingElement. This turned out to be
my misunderstanding of how scrollingElement and viewport scrolling in regards
to the DOM tree worked. This caused a bug whenever a page explicitly made the
<body> element scrollable as the callback would try to scroll the viewport
rather than the <body>.

The solution is to install the apply scroll on the document element (<html>).
This means that the <body> element will still be considered for scrolling while
the scroll bubbles up the DOM. An <html> element never has any overflow of its
own, even if it's explicitly marked as overflow: scroll. In most cases, the
<body> won't have an overflow clip of its own and so the scroll will defer to
the viewport. This is in line with how scrolling worked prior to my change in
r387224.

BUG=604296
==========

to

==========
Viewport apply scroll should be on the document element not scrollingElement.

In r387224 I moved all the viewport scrolling actions be happen from a special
apply scroll method installed on the scrollingElement. This turned out to be
my misunderstanding of how scrollingElement and viewport scrolling in regards
to the DOM tree worked. This caused a bug whenever a page explicitly made the
<body> element scrollable as the callback would try to scroll the viewport
rather than the <body>.

The solution is to install the apply scroll on the document element (<html>).
This means that the <body> element will still be considered for scrolling while
the scroll bubbles up the DOM. An <html> element never has any overflow of its
own, even if it's explicitly marked as overflow: scroll. In most cases, the
<body> won't have an overflow clip of its own and so the scroll will defer to
the viewport. This is in line with how scrolling worked prior to my change in
r387224.

Since we no longer query scrollingElement from layout, the failing test
expectation (track-word-breaking.html) is no longer tripped by this so I removed
the expectation.

BUG=604296
==========

tdresser

https://codereview.chromium.org/1895323002/diff/60001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1895323002/diff/60001/third_party/WebKit/Source/core/dom/Document.cpp#newcode589 third_party/WebKit/Source/core/dom/Document.cpp:589: void Document::updateViewportApplyScroll() We shouldn't need to call this nearly ...

4 years, 8 months ago (2016-04-19 15:04:09 UTC) #17

bokan

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

4 years, 8 months ago (2016-04-19 17:31:44 UTC) #18

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895323002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895323002/100001

4 years, 8 months ago (2016-04-19 17:32:23 UTC) #19

bokan

https://codereview.chromium.org/1895323002/diff/60001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1895323002/diff/60001/third_party/WebKit/Source/core/dom/Document.cpp#newcode589 third_party/WebKit/Source/core/dom/Document.cpp:589: void Document::updateViewportApplyScroll() On 2016/04/19 15:04:09, tdresser wrote: > We ...

4 years, 8 months ago (2016-04-19 17:33:17 UTC) #20

tdresser

LGTM https://codereview.chromium.org/1895323002/diff/100001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1895323002/diff/100001/third_party/WebKit/Source/core/dom/Document.cpp#newcode591 third_party/WebKit/Source/core/dom/Document.cpp:591: void Document::updateViewportApplyScroll() We could put this in an ...

4 years, 8 months ago (2016-04-19 17:48:42 UTC) #21

bokan

https://codereview.chromium.org/1895323002/diff/100001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1895323002/diff/100001/third_party/WebKit/Source/core/dom/Document.cpp#newcode591 third_party/WebKit/Source/core/dom/Document.cpp:591: void Document::updateViewportApplyScroll() On 2016/04/19 17:48:42, tdresser wrote: > We ...

4 years, 8 months ago (2016-04-19 18:36:27 UTC) #22

bokan

The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the ...

4 years, 8 months ago (2016-04-19 18:38:41 UTC) #24

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895323002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895323002/120001

4 years, 8 months ago (2016-04-19 18:39:13 UTC) #25

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-19 20:10:06 UTC) #26

commit-bot: I haz the power

Failed to apply patch for third_party/WebKit/LayoutTests/TestExpectations: While running git apply --index -3 -p1; error: patch ...

4 years, 8 months ago (2016-04-19 20:10:07 UTC) #27

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-19 20:10:09 UTC) #28

bokan

The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the ...

4 years, 8 months ago (2016-04-19 20:14:24 UTC) #30

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895323002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895323002/140001

4 years, 8 months ago (2016-04-19 20:14:52 UTC) #31

commit-bot: I haz the power

Description was changed from ========== Viewport apply scroll should be on the document element not ...

4 years, 8 months ago (2016-04-19 21:46:05 UTC) #32

Message was sent while issue was closed.

Description was changed from

==========
Viewport apply scroll should be on the document element not scrollingElement.

In r387224 I moved all the viewport scrolling actions be happen from a special
apply scroll method installed on the scrollingElement. This turned out to be
my misunderstanding of how scrollingElement and viewport scrolling in regards
to the DOM tree worked. This caused a bug whenever a page explicitly made the
<body> element scrollable as the callback would try to scroll the viewport
rather than the <body>.

The solution is to install the apply scroll on the document element (<html>).
This means that the <body> element will still be considered for scrolling while
the scroll bubbles up the DOM. An <html> element never has any overflow of its
own, even if it's explicitly marked as overflow: scroll. In most cases, the
<body> won't have an overflow clip of its own and so the scroll will defer to
the viewport. This is in line with how scrolling worked prior to my change in
r387224.

Since we no longer query scrollingElement from layout, the failing test
expectation (track-word-breaking.html) is no longer tripped by this so I removed
the expectation.

BUG=604296
==========

to

==========
Viewport apply scroll should be on the document element not scrollingElement.

In r387224 I moved all the viewport scrolling actions be happen from a special
apply scroll method installed on the scrollingElement. This turned out to be
my misunderstanding of how scrollingElement and viewport scrolling in regards
to the DOM tree worked. This caused a bug whenever a page explicitly made the
<body> element scrollable as the callback would try to scroll the viewport
rather than the <body>.

The solution is to install the apply scroll on the document element (<html>).
This means that the <body> element will still be considered for scrolling while
the scroll bubbles up the DOM. An <html> element never has any overflow of its
own, even if it's explicitly marked as overflow: scroll. In most cases, the
<body> won't have an overflow clip of its own and so the scroll will defer to
the viewport. This is in line with how scrolling worked prior to my change in
r387224.

Since we no longer query scrollingElement from layout, the failing test
expectation (track-word-breaking.html) is no longer tripped by this so I removed
the expectation.

BUG=604296
==========

commit-bot: I haz the power

Description was changed from ========== Viewport apply scroll should be on the document element not ...

4 years, 8 months ago (2016-04-22 19:16:46 UTC) #34

Message was sent while issue was closed.

Description was changed from

==========
Viewport apply scroll should be on the document element not scrollingElement.

In r387224 I moved all the viewport scrolling actions be happen from a special
apply scroll method installed on the scrollingElement. This turned out to be
my misunderstanding of how scrollingElement and viewport scrolling in regards
to the DOM tree worked. This caused a bug whenever a page explicitly made the
<body> element scrollable as the callback would try to scroll the viewport
rather than the <body>.

The solution is to install the apply scroll on the document element (<html>).
This means that the <body> element will still be considered for scrolling while
the scroll bubbles up the DOM. An <html> element never has any overflow of its
own, even if it's explicitly marked as overflow: scroll. In most cases, the
<body> won't have an overflow clip of its own and so the scroll will defer to
the viewport. This is in line with how scrolling worked prior to my change in
r387224.

Since we no longer query scrollingElement from layout, the failing test
expectation (track-word-breaking.html) is no longer tripped by this so I removed
the expectation.

BUG=604296
==========

to

==========
Viewport apply scroll should be on the document element not scrollingElement.

In r387224 I moved all the viewport scrolling actions be happen from a special
apply scroll method installed on the scrollingElement. This turned out to be
my misunderstanding of how scrollingElement and viewport scrolling in regards
to the DOM tree worked. This caused a bug whenever a page explicitly made the
<body> element scrollable as the callback would try to scroll the viewport
rather than the <body>.

The solution is to install the apply scroll on the document element (<html>).
This means that the <body> element will still be considered for scrolling while
the scroll bubbles up the DOM. An <html> element never has any overflow of its
own, even if it's explicitly marked as overflow: scroll. In most cases, the
<body> won't have an overflow clip of its own and so the scroll will defer to
the viewport. This is in line with how scrolling worked prior to my change in
r387224.

Since we no longer query scrollingElement from layout, the failing test
expectation (track-word-breaking.html) is no longer tripped by this so I removed
the expectation.

BUG=604296

Committed: https://crrev.com/c6821f17fffc100363aab8ce04c22d6a59b57e33
Cr-Commit-Position: refs/heads/master@{#388320}
==========

commit-bot: I haz the power

4 years, 8 months ago (2016-04-22 19:16:47 UTC) #35

Message was sent while issue was closed.

Patchset 8 (id:??) landed as
https://crrev.com/c6821f17fffc100363aab8ce04c22d6a59b57e33
Cr-Commit-Position: refs/heads/master@{#388320}

Issue 1895323002: Viewport apply scroll should be on the document element not scrollingElement. (Closed)

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : Rebase #

Patch Set 6 : Moved attaching applyScroll to childrenChanged #

Patch Set 7 : Moved updateViewportApplyScroll into anonymous function #

Patch Set 8 : Rebase #

Messages