Issue 782793002: Update Element API for CSSOM smooth scrolling to match the spec

Issue 782793002: Update Element API for CSSOM smooth scrolling to match the spec (Closed)

Created:
6 years ago by ajuma

Modified:
6 years ago

Reviewers:
Ian Vollick, Jens Widell

CC:
blink-reviews, blink-reviews-html_chromium.org, arv+blink, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews-bindings_chromium.org, Inactive, rwlbuis

Base URL:
svn://svn.chromium.org/blink/trunk

Project:
blink

Visibility:
Public.

More Reviews

Description

Update Element API for CSSOM smooth scrolling to match the spec This updates the Element API for smooth scrolling to match changes to the CSSOM View spec. Previously, the spec overloaded the setters for scrollTop and scrollLeft, allowing these to take a ScrollOptions dictionary. Now, the spec instead adds scroll, scrollTo, and scrollBy methods that each take either a pair of doubles or a single ScrollToOptions dictionary (just like the corresponding methods on Window). Smooth scrolling is behing a runtime flag. Spec: http://dev.w3.org/csswg/cssom-view/#extensions-to-the-element-interface BUG=243871 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186725

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : Rebased #

Total comments: 10

Patch Set 5 : Address review comments #

Created: 6 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1890 lines, -681 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2 3	1 chunk	+0 lines, -2 lines	0 comments	Download
D	LayoutTests/fast/dom/Element/scrollTop-scrollLeft-setters.html	View		1 chunk	+0 lines, -76 lines	0 comments	Download
D	LayoutTests/fast/dom/Element/scrollTop-scrollLeft-setters-expected.txt	View		1 chunk	+0 lines, -55 lines	0 comments	Download
M	LayoutTests/fast/dom/non-numeric-values-numeric-parameters-expected.txt	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/fast/dom/script-tests/non-numeric-values-numeric-parameters.js	View		1 chunk	+2 lines, -2 lines	0 comments	Download
D	LayoutTests/fast/scroll-behavior/listbox-interrupted-scroll.html	View		1 chunk	+0 lines, -69 lines	0 comments	Download
D	LayoutTests/fast/scroll-behavior/listbox-interrupted-scroll-expected.txt	View		1 chunk	+0 lines, -10 lines	0 comments	Download
D	LayoutTests/fast/scroll-behavior/listbox-scrollTop.html	View		1 chunk	+0 lines, -79 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/main-frame-element-scroll.html	View	1 2	1 chunk	+86 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/main-frame-element-scroll-expected.txt	View	1 2	1 chunk	+22 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/main-frame-element-scrollBy.html	View	1 2	1 chunk	+85 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/main-frame-element-scrollBy-expected.txt	View	1 2	1 chunk	+22 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/main-frame-element-scrollTo.html	View	1 2	1 chunk	+86 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/main-frame-element-scrollTo-expected.txt	View	1 2	1 chunk	+22 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/main-frame-scroll-in-quirks-mode.html	View	1	1 chunk	+68 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/main-frame-scroll-in-quirks-mode-expected.txt	View		1 chunk	+44 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/main-frame-scroll-in-standards-mode.html	View	1	1 chunk	+69 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/main-frame-scroll-in-standards-mode-expected.txt	View		1 chunk	+44 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/main-frame-scrollLeft.html	View		1 chunk	+7 lines, -17 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/main-frame-scrollLeft-expected.txt	View		1 chunk	+4 lines, -10 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/main-frame-scrollTop.html	View		1 chunk	+7 lines, -17 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/main-frame-scrollTop-expected.txt	View		1 chunk	+4 lines, -10 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-hidden-scroll.html	View	1 2	1 chunk	+95 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-hidden-scroll-expected.txt	View		1 chunk	+22 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-hidden-scrollBy.html	View	1 2	1 chunk	+94 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-hidden-scrollBy-expected.txt	View		1 chunk	+22 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/overflow-hidden-scrollLeft.html	View		1 chunk	+7 lines, -17 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/overflow-hidden-scrollLeft-expected.txt	View		1 chunk	+3 lines, -9 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-hidden-scrollTo.html	View	1 2	1 chunk	+95 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-hidden-scrollTo-expected.txt	View		1 chunk	+22 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/overflow-hidden-scrollTop.html	View		1 chunk	+7 lines, -17 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/overflow-hidden-scrollTop-expected.txt	View		1 chunk	+2 lines, -8 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/overflow-interrupted-scroll.html	View		1 chunk	+1 line, -1 line	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-scroll-scroll.html	View	1 2	1 chunk	+95 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-scroll-scroll-expected.txt	View		1 chunk	+22 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-scroll-scrollBy.html	View	1 2	1 chunk	+94 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-scroll-scrollBy-expected.txt	View		1 chunk	+22 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/overflow-scroll-scrollLeft.html	View		1 chunk	+7 lines, -17 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/overflow-scroll-scrollLeft-expected.txt	View		1 chunk	+3 lines, -9 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-scroll-scrollTo.html	View	1 2	1 chunk	+95 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/overflow-scroll-scrollTo-expected.txt	View		1 chunk	+22 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/overflow-scroll-scrollTop.html	View		1 chunk	+7 lines, -17 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/overflow-scroll-scrollTop-expected.txt	View		1 chunk	+2 lines, -8 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/resources/scroll-behavior-test.js	View		1 chunk	+1 line, -2 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/subframe-element-scroll.html	View	1 2	1 chunk	+87 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/subframe-element-scroll-expected.txt	View	1 2	1 chunk	+23 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/subframe-element-scrollBy.html	View	1 2	1 chunk	+86 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/subframe-element-scrollBy-expected.txt	View	1 2	1 chunk	+23 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/subframe-element-scrollTo.html	View	1 2	1 chunk	+87 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/scroll-behavior/subframe-element-scrollTo-expected.txt	View	1 2	1 chunk	+23 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/subframe-scrollLeft.html	View		1 chunk	+7 lines, -17 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/subframe-scrollLeft-expected.txt	View		1 chunk	+4 lines, -10 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/subframe-scrollTop.html	View		1 chunk	+7 lines, -17 lines	0 comments	Download
M	LayoutTests/fast/scroll-behavior/subframe-scrollTop-expected.txt	View		1 chunk	+4 lines, -10 lines	0 comments	Download
D	LayoutTests/platform/linux/fast/scroll-behavior/listbox-scrollTop-expected.txt	View		1 chunk	+0 lines, -18 lines	0 comments	Download
D	LayoutTests/platform/mac/fast/scroll-behavior/listbox-interrupted-scroll-expected.txt	View		1 chunk	+0 lines, -10 lines	0 comments	Download
D	LayoutTests/platform/mac/fast/scroll-behavior/listbox-scrollTop-expected.txt	View		1 chunk	+0 lines, -18 lines	0 comments	Download
D	LayoutTests/platform/win-xp/fast/scroll-behavior/listbox-scrollTop-expected.txt	View		1 chunk	+0 lines, -18 lines	0 comments	Download
D	LayoutTests/platform/win/fast/scroll-behavior/listbox-scrollTop-expected.txt	View		1 chunk	+0 lines, -18 lines	0 comments	Download
M	LayoutTests/webexposed/global-interface-listing-expected.txt	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	Source/bindings/core/v8/custom/V8ElementCustom.cpp	View		1 chunk	+0 lines, -32 lines	0 comments	Download
M	Source/core/dom/Element.h	View	1 2 3 4	3 chunks	+13 lines, -2 lines	0 comments	Download
M	Source/core/dom/Element.cpp	View	1 2 3 4	4 chunks	+143 lines, -44 lines	0 comments	Download
M	Source/core/dom/Element.idl	View	1 2 3 4	1 chunk	+9 lines, -6 lines	0 comments	Download
M	Source/core/html/HTMLBodyElement.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	Source/core/html/HTMLBodyElement.cpp	View	1 2 3 4	4 chunks	+49 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLSelectElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLSelectElement.cpp	View		3 chunks	+4 lines, -4 lines	0 comments	Download

Messages

Total messages: 10 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

ajuma

https://codereview.chromium.org/782793002/diff/60001/LayoutTests/fast/scroll-behavior/listbox-scrollTop.html File LayoutTests/fast/scroll-behavior/listbox-scrollTop.html (left): https://codereview.chromium.org/782793002/diff/60001/LayoutTests/fast/scroll-behavior/listbox-scrollTop.html#oldcode1 LayoutTests/fast/scroll-behavior/listbox-scrollTop.html:1: <!DOCTYPE html> Now that RenderListBox is no longer a ...

6 years ago (2014-12-08 15:14:14 UTC) #2

Jens Widell

bindings/ LGTM https://codereview.chromium.org/782793002/diff/60001/Source/core/dom/Element.idl File Source/core/dom/Element.idl (right): https://codereview.chromium.org/782793002/diff/60001/Source/core/dom/Element.idl#newcode78 Source/core/dom/Element.idl:78: [RuntimeEnabled=CSSOMSmoothScroll] void scroll([Default=Undefined] optional ScrollToOptions scrollToOptions); [Default=Undefined] ...

6 years ago (2014-12-08 16:19:07 UTC) #4

Ian Vollick

lgtm with some nits and questions. https://codereview.chromium.org/782793002/diff/60001/LayoutTests/fast/scroll-behavior/overflow-hidden-scroll-expected.txt File LayoutTests/fast/scroll-behavior/overflow-hidden-scroll-expected.txt (right): https://codereview.chromium.org/782793002/diff/60001/LayoutTests/fast/scroll-behavior/overflow-hidden-scroll-expected.txt#newcode11 LayoutTests/fast/scroll-behavior/overflow-hidden-scroll-expected.txt:11: FAIL Scroll x:10, ...

6 years ago (2014-12-08 16:35:23 UTC) #5

ajuma

Ian, PTAL at the helpers added to Element (specifically, scrollFrameBy, scrollFrameTo, scrollRenderBoxBy, and scrollRenderBoxTo). https://codereview.chromium.org/782793002/diff/60001/LayoutTests/fast/scroll-behavior/overflow-hidden-scroll-expected.txt ...

6 years ago (2014-12-08 18:34:13 UTC) #6

Ian, PTAL at the helpers added to Element (specifically, scrollFrameBy,
scrollFrameTo, scrollRenderBoxBy, and scrollRenderBoxTo).

https://codereview.chromium.org/782793002/diff/60001/LayoutTests/fast/scroll-...
File LayoutTests/fast/scroll-behavior/overflow-hidden-scroll-expected.txt
(right):

https://codereview.chromium.org/782793002/diff/60001/LayoutTests/fast/scroll-...
LayoutTests/fast/scroll-behavior/overflow-hidden-scroll-expected.txt:11: FAIL
Scroll x:10, y:15, smooth:true assert_equals: expected "3, 4" but got "10, 15"
On 2014/12/08 16:35:23, vollick wrote:
> Can you please remind me why you're adding these FAIL lines to the
expected.txt
> files? I assume that it's because the implementation isn't complete, so even a
> virtual test suite or adding an InternalSetting to enable smooth scrolling
> wouldn't get these tests passing? 

Yes, these are here only because the implementation isn't complete.

https://codereview.chromium.org/782793002/diff/60001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/782793002/diff/60001/Source/core/dom/Element....
Source/core/dom/Element.cpp:752:
document().updateLayoutIgnorePendingStylesheets();
On 2014/12/08 16:35:23, vollick wrote:
> Eventually, we'll want to process scrolls after compositing update. Will this
> still be required when that's the case?

No, there's nothing smooth-scroll specific that requires this. Once this can be
removed for 'regular' scrolls (e.g. in setScrollTop/setScrollLeft) it can be
removed here too.

> If not, can you add comment referencing
> the related bug (https://code.google.com/p/chromium/issues/detail?id=420741)?
> (Here and in ::scrollTo)

Done.

https://codereview.chromium.org/782793002/diff/60001/Source/core/dom/Element.idl
File Source/core/dom/Element.idl (right):

https://codereview.chromium.org/782793002/diff/60001/Source/core/dom/Element....
Source/core/dom/Element.idl:78: [RuntimeEnabled=CSSOMSmoothScroll] void
scroll([Default=Undefined] optional ScrollToOptions scrollToOptions);
On 2014/12/08 16:19:07, Jens Widell wrote:
> [Default=Undefined] has no effect on optional dictionary type arguments. (The
> effect it has on other types of arguments is implied for dictionary type
> arguments.)
> 
> So, since it's just noise, remove it?

Done.

https://codereview.chromium.org/782793002/diff/60001/Source/core/html/HTMLBod...
File Source/core/html/HTMLBodyElement.cpp (right):

https://codereview.chromium.org/782793002/diff/60001/Source/core/html/HTMLBod...
Source/core/html/HTMLBodyElement.cpp:340: void HTMLBodyElement::scrollBy(const
ScrollToOptions& scrollToOptions)
On 2014/12/08 16:35:23, vollick wrote:
> nit: The similarity of these methods and those in Element is a little sad. Is
it
> possible to share code?

Added helpers (scrollFrameBy, scrollFrameTo, scrollRenderBoxBy,
scrollRenderBoxTo) to Element so that code can be shared.

Ian Vollick

On 2014/12/08 18:34:13, ajuma wrote: > Ian, PTAL at the helpers added to Element (specifically, ...

6 years ago (2014-12-08 18:48:59 UTC) #7

On 2014/12/08 18:34:13, ajuma wrote:
> Ian, PTAL at the helpers added to Element (specifically, scrollFrameBy,
> scrollFrameTo, scrollRenderBoxBy, and scrollRenderBoxTo).
> 
>
https://codereview.chromium.org/782793002/diff/60001/LayoutTests/fast/scroll-...
> File LayoutTests/fast/scroll-behavior/overflow-hidden-scroll-expected.txt
> (right):
> 
>
https://codereview.chromium.org/782793002/diff/60001/LayoutTests/fast/scroll-...
> LayoutTests/fast/scroll-behavior/overflow-hidden-scroll-expected.txt:11: FAIL
> Scroll x:10, y:15, smooth:true assert_equals: expected "3, 4" but got "10, 15"
> On 2014/12/08 16:35:23, vollick wrote:
> > Can you please remind me why you're adding these FAIL lines to the
> expected.txt
> > files? I assume that it's because the implementation isn't complete, so even
a
> > virtual test suite or adding an InternalSetting to enable smooth scrolling
> > wouldn't get these tests passing? 
> 
> Yes, these are here only because the implementation isn't complete.
> 
>
https://codereview.chromium.org/782793002/diff/60001/Source/core/dom/Element.cpp
> File Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/782793002/diff/60001/Source/core/dom/Element....
> Source/core/dom/Element.cpp:752:
> document().updateLayoutIgnorePendingStylesheets();
> On 2014/12/08 16:35:23, vollick wrote:
> > Eventually, we'll want to process scrolls after compositing update. Will
this
> > still be required when that's the case?
> 
> No, there's nothing smooth-scroll specific that requires this. Once this can
be
> removed for 'regular' scrolls (e.g. in setScrollTop/setScrollLeft) it can be
> removed here too.
> 
> > If not, can you add comment referencing
> > the related bug
(https://code.google.com/p/chromium/issues/detail?id=420741)?
> > (Here and in ::scrollTo)
> 
> Done.
> 
>
https://codereview.chromium.org/782793002/diff/60001/Source/core/dom/Element.idl
> File Source/core/dom/Element.idl (right):
> 
>
https://codereview.chromium.org/782793002/diff/60001/Source/core/dom/Element....
> Source/core/dom/Element.idl:78: [RuntimeEnabled=CSSOMSmoothScroll] void
> scroll([Default=Undefined] optional ScrollToOptions scrollToOptions);
> On 2014/12/08 16:19:07, Jens Widell wrote:
> > [Default=Undefined] has no effect on optional dictionary type arguments.
(The
> > effect it has on other types of arguments is implied for dictionary type
> > arguments.)
> > 
> > So, since it's just noise, remove it?
> 
> Done.
> 
>
https://codereview.chromium.org/782793002/diff/60001/Source/core/html/HTMLBod...
> File Source/core/html/HTMLBodyElement.cpp (right):
> 
>
https://codereview.chromium.org/782793002/diff/60001/Source/core/html/HTMLBod...
> Source/core/html/HTMLBodyElement.cpp:340: void HTMLBodyElement::scrollBy(const
> ScrollToOptions& scrollToOptions)
> On 2014/12/08 16:35:23, vollick wrote:
> > nit: The similarity of these methods and those in Element is a little sad.
Is
> it
> > possible to share code?
> 
> Added helpers (scrollFrameBy, scrollFrameTo, scrollRenderBoxBy,
> scrollRenderBoxTo) to Element so that code can be shared.

Thanks! Still lgtm.

commit-bot: I haz the power

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

6 years ago (2014-12-08 18:50:24 UTC) #9

commit-bot: I haz the power

6 years ago (2014-12-08 20:02:08 UTC) #10

Message was sent while issue was closed.

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

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages