Bug 157666 - Align window.scroll() / scrollTo() / scrollBy() with the CSSOM specification
Summary: Align window.scroll() / scrollTo() / scrollBy() with the CSSOM specification
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://drafts.csswg.org/cssom-view/#...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-12 23:03 PDT by Eunmi Lee
Modified: 2016-09-05 18:31 PDT (History)
9 users (show)

See Also:


Attachments
WIP patch (19.67 KB, patch)
2016-05-13 10:58 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (30.09 KB, patch)
2016-05-13 11:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eunmi Lee 2016-05-12 23:03:08 PDT
Safari can not scroll to the bottom area in the https://members.chosun.com/cms_subscribe/application/index.jsp which has <body onScroll="scroll()">,
because scroll() with no arguments treats as scroll(0, 0) in the WebKit.

It is regression of r100163, and IE 11, Firefox 46.0.1 and Chrome 50 can scroll this web page.
(Chrome can scroll after this patch: https://codereview.chromium.org/59863003/)
Comment 1 Chris Dumez 2016-05-13 09:27:23 PDT
The bug report, as is stated, does not seem correct as per the spec:
https://drafts.csswg.org/cssom-view/#extensions-to-the-window-interface

The spec define 2 scroll() functions:
void scroll(optional ScrollToOptions options);
void scroll(unrestricted double x, unrestricted double y);

So if you call scroll() function without parameter, it should call the first one.
However, our behavior is incorrect in this case I believe. It looks like we are supposed to use the current's viewport position as x/y but we use 0/0 instead. So I think we should not scroll instead of scrolling to the top-left.
Comment 2 Chris Dumez 2016-05-13 10:58:11 PDT
Created attachment 278850 [details]
WIP patch

Fixes the bug on https://members.chosun.com/cms_subscribe/application/index.jsp and aligns us with the spec but needs more testing.
It is leveraging Darin's recent support for dictionaries although I had to make it work for floating point typed members.
Comment 3 Chris Dumez 2016-05-13 11:27:12 PDT
Created attachment 278855 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2016-05-13 12:04:20 PDT
<rdar://problem/26272835>
Comment 5 Darin Adler 2016-05-13 19:00:47 PDT
Comment on attachment 278855 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278855&action=review

> Source/WebCore/bindings/js/JSDOMConvert.h:35
> +template<typename T, typename Enable = void> struct Converter;

I’m not familiar with this technique.

> Source/WebCore/page/DOMWindow.cpp:1522
> +    RefPtr<FrameView> view = m_frame->view();

Why is this RefPtr? Why not auto*? I guess this was copied from the function below, but I don’t think it needs to be.
Comment 6 Chris Dumez 2016-05-13 19:05:30 PDT
Comment on attachment 278855 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278855&action=review

>> Source/WebCore/bindings/js/JSDOMConvert.h:35
>> +template<typename T, typename Enable = void> struct Converter;
> 
> I’m not familiar with this technique.

Found on http://en.cppreference.com/w/cpp/types/enable_if

I did not want to have to specialize for both float and double.
Comment 7 WebKit Commit Bot 2016-05-13 19:09:36 PDT
Comment on attachment 278855 [details]
Patch

Clearing flags on attachment: 278855

Committed r200907: <http://trac.webkit.org/changeset/200907>
Comment 8 WebKit Commit Bot 2016-05-13 19:09:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Eunmi Lee 2016-05-14 01:26:42 PDT
I've surprised at quick patch. Thank you very much Chris! :)
Comment 10 Simon Fraser (smfr) 2016-09-05 18:31:57 PDT
Also need to match the spec for element scrolling: bug 161610.