Implementing document.setRootScroller API for main thread scrolling.
This patch implements the setRootScroller API, also know as the non-document
root scroller proposal. See this expaliner for details:
https://github.com/bokand/NonDocumentRootScroller
This patch implements most of the initial functionality but only for scrolling
on the main thread. All new API surfaces are behind a RuntimeEnabledFeature.
BUG=505516
Committed: https://crrev.com/861d774435b04448856e99462d4f4e3d734f72f4
Cr-Commit-Position: refs/heads/master@{#390200}
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913843004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913843004/20001
4 years, 7 months ago
(2016-04-26 15:20:57 UTC)
#2
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/218904)
4 years, 7 months ago
(2016-04-26 16:36:53 UTC)
#4
Description was changed from ========== Implementing document.setRootScroller API for main thread scrolling. This patch implements ...
4 years, 7 months ago
(2016-04-26 17:48:29 UTC)
#5
Description was changed from
==========
Implementing document.setRootScroller API for main thread scrolling.
This patch implements the setRootScroller API, also know as the non-document
root scroller proposal. See this expaliner for details:
https://github.com/bokand/NonDocumentRootScroller
This patch implements most of the initial functionality but only for scrolling
on the main thread. All added API surfaces are behind a RuntimeEnabledFeature.
BUG=505516
==========
to
==========
Implementing document.setRootScroller API for main thread scrolling.
This patch implements the setRootScroller API, also know as the non-document
root scroller proposal. See this expaliner for details:
https://github.com/bokand/NonDocumentRootScroller
This patch implements most of the initial functionality but only for scrolling
on the main thread. All new API surfaces are behind a RuntimeEnabledFeature.
BUG=505516
==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913843004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913843004/60001
4 years, 7 months ago
(2016-04-26 23:06:51 UTC)
#11
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ninja/builds/207426) mac_chromium_rel_ng on ...
4 years, 7 months ago
(2016-04-26 23:09:09 UTC)
#13
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913843004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913843004/80001
4 years, 7 months ago
(2016-04-27 00:56:40 UTC)
#15
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/219351)
4 years, 7 months ago
(2016-04-27 02:10:50 UTC)
#17
Found two failing tests that looked unrelated but are in fact caused by this patch. ...
4 years, 7 months ago
(2016-04-27 17:53:56 UTC)
#19
Found two failing tests that looked unrelated but are in fact caused by this
patch. Looking into it now.
https://codereview.chromium.org/1913843004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/ViewportScrollCallback.cpp
(right):
https://codereview.chromium.org/1913843004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/ViewportScrollCallback.cpp:92: //
TODO: This isn't consuming everything
On 2016/04/27 15:37:08, tdresser wrote:
> Should this TODO have a bug associated with it?
Done.
https://codereview.chromium.org/1913843004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/RootScrollerTest.cpp (right):
https://codereview.chromium.org/1913843004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:228: ASSERT_FLOAT_EQ(0,
mainFrameView()->scrollPositionDouble().y());
On 2016/04/27 15:37:08, tdresser wrote:
> Because of scroll latching, we wouldn't scroll the FrameView either way,
right?
> I think we need an additional gesture sequence.
Yah, this comment is misleading so I changed it. We do try a new gesture
sequence below for just this reason.
https://codereview.chromium.org/1913843004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:364: // Tests that
setting the root scroller on an element within a iframe fails.
On 2016/04/27 15:37:08, tdresser wrote:
> Do you set the root scroller on an element, or do you set the root scroller to
> the element?
>
> It's a bit weird because "the root scroller" refers to an element, but being
the
> root scroller is a property of the element, and we sort of use them
> interchangeably.
>
> When you talk about "setting the root scroller" I think of the element which
is
> the root scroller, not the property which is whether or not an element is a
root
> scroller.
Good point. The way to think about it is that the "Root Scroller" is a property
of the document whose type is an Element. I've updated all the comments here to
be consistent with this interpretation.
https://codereview.chromium.org/1913843004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:414: {
On 2016/04/27 15:37:08, tdresser wrote:
> It might be worth splitting up these two cases into two tests.
Done.
https://codereview.chromium.org/1913843004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:438: // Trying to set
an element from outside the iframe should fail.
On 2016/04/27 15:37:08, tdresser wrote:
> I initially misinterpreted this comment, thinking that it meant that the
action
> "set an element" took place outside the iframe, not that the element being
> referred to was outside of the iframe.
>
> That's a pretty silly interpretation, but it wouldn't hurt to elaborate a bit
on
> this (and the comment above).
Done.
https://codereview.chromium.org/1913843004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:449:
TrackExceptionState exceptionState1;
On 2016/04/27 15:37:08, tdresser wrote:
> Is there any reason to not reuse the exceptionState object?
Didn't think to just clear the exception state. Done.
bokan
:/ Test failures were cause of a bad rebase conflict resolve in PatchSet 5. I've ...
4 years, 7 months ago
(2016-04-27 19:02:03 UTC)
#20
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913843004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913843004/180001
4 years, 7 months ago
(2016-04-27 20:27:14 UTC)
#27
Description was changed from ========== Implementing document.setRootScroller API for main thread scrolling. This patch implements ...
4 years, 7 months ago
(2016-04-27 21:44:43 UTC)
#28
Message was sent while issue was closed.
Description was changed from
==========
Implementing document.setRootScroller API for main thread scrolling.
This patch implements the setRootScroller API, also know as the non-document
root scroller proposal. See this expaliner for details:
https://github.com/bokand/NonDocumentRootScroller
This patch implements most of the initial functionality but only for scrolling
on the main thread. All new API surfaces are behind a RuntimeEnabledFeature.
BUG=505516
==========
to
==========
Implementing document.setRootScroller API for main thread scrolling.
This patch implements the setRootScroller API, also know as the non-document
root scroller proposal. See this expaliner for details:
https://github.com/bokand/NonDocumentRootScroller
This patch implements most of the initial functionality but only for scrolling
on the main thread. All new API surfaces are behind a RuntimeEnabledFeature.
BUG=505516
==========
commit-bot: I haz the power
Committed patchset #10 (id:180001)
4 years, 7 months ago
(2016-04-27 21:44:44 UTC)
#29
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/861d774435b04448856e99462d4f4e3d734f72f4 Cr-Commit-Position: refs/heads/master@{#390200}
4 years, 7 months ago
(2016-04-30 17:13:32 UTC)
#30
Description was changed from ========== Implementing document.setRootScroller API for main thread scrolling. This patch implements ...
4 years, 7 months ago
(2016-04-30 17:52:11 UTC)
#31
Message was sent while issue was closed.
Description was changed from
==========
Implementing document.setRootScroller API for main thread scrolling.
This patch implements the setRootScroller API, also know as the non-document
root scroller proposal. See this expaliner for details:
https://github.com/bokand/NonDocumentRootScroller
This patch implements most of the initial functionality but only for scrolling
on the main thread. All new API surfaces are behind a RuntimeEnabledFeature.
BUG=505516
==========
to
==========
Implementing document.setRootScroller API for main thread scrolling.
This patch implements the setRootScroller API, also know as the non-document
root scroller proposal. See this expaliner for details:
https://github.com/bokand/NonDocumentRootScroller
This patch implements most of the initial functionality but only for scrolling
on the main thread. All new API surfaces are behind a RuntimeEnabledFeature.
BUG=505516
Committed: https://crrev.com/861d774435b04448856e99462d4f4e3d734f72f4
Cr-Commit-Position: refs/heads/master@{#390200}
==========
https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Source/core/dom/Document.cpp#newcode611 third_party/WebKit/Source/core/dom/Document.cpp:611: "Element isn't in this document."); This doesn't really make ...
4 years, 7 months ago
(2016-05-05 17:29:47 UTC)
#34
Thanks for the help Elliott, I'll put up a new patch soon that fixes your ...
4 years, 7 months ago
(2016-05-05 21:19:49 UTC)
#35
Message was sent while issue was closed.
Thanks for the help Elliott, I'll put up a new patch soon that fixes your
concerns.
https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.idl (right):
https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.idl:77:
[RuntimeEnabled=SetRootScroller, RaisesException] void setRootScroller (Element
element);
On 2016/05/05 17:20:19, esprehn wrote:
> This doesn't match web platform conventions. The web doesn't use setFoo
methods,
> it uses setter properties. This API needs a review.
Acknowledged.
https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp (right):
https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:26:
DCHECK(frameHost->page().mainFrame()->isLocalFrame());
On 2016/05/05 17:29:46, esprehn wrote:
> There's no reason to go through the FrameHost like this all the time, just use
> the localRoot()
Acknowledged.
https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:37: if
(box->isDocumentElement() && element.document().view())
On 2016/05/05 17:29:46, esprehn wrote:
> it's impossible for view() to be null here if the LayoutObject is not null
Acknowledged.
https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:40: return
static_cast<PaintInvalidationCapableScrollableArea*>(
On 2016/05/05 17:29:46, esprehn wrote:
> this needs a toPaintInvalidationCapableScrollableArea() casting function and a
> virtual check.
This is an upcast though. LayoutBoxModelObject::getScrollableArea()
(confusingly) returns a PaintLayerScrollableArea which derives from
PaintInvalidationCapableScrollableArea.
https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:47: :
m_frameHost(&frameHost)
On 2016/05/05 17:29:46, esprehn wrote:
> use a Frame, not the FrameHost.
Yah, I hadn't thought this through and had a 1 RootScroller per Page. I think
having 1:1 RootScroller:Frame is a better idea, with only the main frame's
RootScroller applying viewport actions.
https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:132: if
(element.document() != topDocument(m_frameHost))
On 2016/05/05 17:29:46, esprehn wrote:
> you want to hook Element::removedFrom not layout for this
There's other cases that make an Element invalid to be the root scroller. I.e.
if it's not a box.
I didn't add it in this patch, but our initial version of this proposal also has
that only viewport-filling elements (i.e. width:100%, height:100%) can be the
root scroller. This is to prevent strange effects as we increase/decrease the
clip height dynamically on the root scroller as we move the top controls and to
make overscroll glow simpler to implement. For those cases, I think we need to
be hooked into layout, right?
The way this is implemented right now, if the currently set root scroller
becomes invalid after being set, the root scroller will reset to the document
element. It sounds like maybe making it "sticky" would be more natural. i.e.
While its invalid, use the document element, but if it becomes valid again it
will be used. Does that sound better?
https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:135: if
(!element.isInTreeScope())
On 2016/05/05 17:29:46, esprehn wrote:
> the layoutObject check below is a proxy for this, there's no reason to check
> both
Acknowledged.
https://codereview.chromium.org/1913843004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:138: if
(!element.layoutObject())
On 2016/05/05 17:29:46, esprehn wrote:
> this check doesn't make sense, it means if you display: none the root
scroller,
> force a layout, and then display: block it again the scroller is reset. I
don't
> think this feature should work that way.
Yah, that was the intention, though I can see that it might be surprising. I'll
do what I mentioned above.
Issue 1913843004: Implementing document.setRootScroller API for main thread scrolling.
(Closed)
Created 4 years, 7 months ago by bokan
Modified 4 years, 7 months ago
Reviewers: esprehn, jbroman, tdresser
Base URL: https://chromium.googlesource.com/chromium/src.git@overscrollController
Comments: 62
Chromium Code Reviews
Issue
1913843004:
Implementing document.setRootScroller API for main thread scrolling. (Closed)
