Summary: | Support Autoscrolling in contenteditable for WK2 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||
Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aestes, simon.fraser, thorton, webkit-bug-importer, webkit, wenson_hsieh | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Megan Gardner
2017-12-13 18:07:03 PST
Created attachment 329517 [details]
Patch
Created attachment 329527 [details]
Patch
Comment on attachment 329527 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329527&action=review Looks good but r- for cleanup and test improvement. > Source/WebCore/page/EventHandler.h:341 > + IntPoint m_targetAutoscrollPosition; In what coordinate system relative to which element? > Source/WebCore/page/EventHandler.h:342 > + bool m_isAutoscrollActive { false }; "active" is ambiguous; it could be "is able to autoscroll" or "is autoscrolling now". I would call this m_isAutoscrolling. Or it seems to correspond to the state of the autoscroll timer. Maybe you don't need this. > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:695 > +- (void)cancelAutoscroll blank line between methods please. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1389 > + m_page->mainFrame().eventHandler().startTextAutoscroll(m_assistedNode->renderer(), position); What if m_assistedNode->renderer() is null? > LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:23 > + var tapPointX = targetRect.x+targetRect.width / 2; > + var tapPointY = targetRect.y+targetRect.height / 2; Spaces around + > LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:30 > + setTimeout(function(){ // wait a spell while the keyboard comes up We have functions to detect when the keyboard animation is finished. Created attachment 329542 [details]
Patch
Comment on attachment 329542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329542&action=review > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:690 > +// @protocol UITextAutoscrolling Nit - we should usually indicate this using `#pragma mark - UITextAutoscrolling` > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:703 > + // used to scroll selection on keyboard up; we already scroll to visible Nit - inline comments like these should begin with a capital letter and end with a period. > LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:22 > + var tapPointX = targetRect.x+targetRect.width / 2; Nit - spaces around the + > LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:23 > + var tapPointY = targetRect.y+targetRect.height / 2; Ditto. > LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:34 > + testRunner.runUIScript(continueTouchAndDragFromPointToPoint(dragX, tapPointY, dragX+5, tapPointY), function() { Ditto (spaces around +) > LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:35 > + if (scrollBox.scrollLeft > 0 ) Nit - extra space after the 0. > LayoutTests/fast/events/touch/ios/resources/basic-gestures.js:173 > + uiController.uiScriptComplete(); Nit - looks like there's an extra indent here? Comment on attachment 329542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329542&action=review > Source/WebCore/page/EventHandler.h:339 > + WEBCORE_EXPORT void startTextAutoscroll(RenderObject* renderer, const FloatPoint& position); position -> positionInWindow > Source/WebCore/page/EventHandler.h:342 > + bool m_isAutoscrolling { false }; My comment about not needing this and using the timer state still stands. > Source/WebCore/page/ios/EventHandlerIOS.mm:563 > +void EventHandler::startTextAutoscroll(RenderObject* renderer, const FloatPoint& position) position -> positionInWindow > Source/WebKit/UIProcess/WebPageProxy.h:573 > + void startAutoscrollAtPosition(const WebCore::FloatPoint&); Name the parameter positionInWindow > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:721 > +void WebPageProxy::startAutoscrollAtPosition(const WebCore::FloatPoint& position) Name the parameter positionInWindow > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:102 > + StartAutoscrollAtPosition(WebCore::FloatPoint position) Name the parameter positionInWindow > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1386 > +void WebPage::startAutoscrollAtPosition(const WebCore::FloatPoint& position) Name the parameter positionInWindow The m_isAutoscrolling is used in a call that the timer uses to determine if it should still be updating, so using the timer state instead would be problematic. *** Bug 148061 has been marked as a duplicate of this bug. *** Comment on attachment 329542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329542&action=review >> Source/WebCore/page/EventHandler.h:342 >> + bool m_isAutoscrolling { false }; > > My comment about not needing this and using the timer state still stands. ... also, these are in the public section :) they should be down with the other members |