Bug 157666

Summary: Align window.scroll() / scrollTo() / scrollBy() with the CSSOM specification
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, commit-queue, darin, esprehn+autocc, kondapallykalyan, rniwa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://drafts.csswg.org/cssom-view/#extensions-to-the-window-interface
Attachments:
Description Flags
WIP patch
none
Patch none

Eunmi Lee
Reported 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/)
Attachments
WIP patch (19.67 KB, patch)
2016-05-13 10:58 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (30.09 KB, patch)
2016-05-13 11:27 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 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.
Chris Dumez
Comment 2 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.
Chris Dumez
Comment 3 2016-05-13 11:27:12 PDT
Created attachment 278855 [details] Patch
Radar WebKit Bug Importer
Comment 4 2016-05-13 12:04:20 PDT
<rdar://problem/26272835>
Darin Adler
Comment 5 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.
Chris Dumez
Comment 6 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.
WebKit Commit Bot
Comment 7 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>
WebKit Commit Bot
Comment 8 2016-05-13 19:09:43 PDT
All reviewed patches have been landed. Closing bug.
Eunmi Lee
Comment 9 2016-05-14 01:26:42 PDT
I've surprised at quick patch. Thank you very much Chris! :)
Simon Fraser (smfr)
Comment 10 2016-09-05 18:31:57 PDT
Also need to match the spec for element scrolling: bug 161610.
Note You need to log in before you can comment on or make changes to this bug.