Bug 74907 - Add support for scrollLineDown: and scrollLineUp: NSResponder selectors
Summary: Add support for scrollLineDown: and scrollLineUp: NSResponder selectors
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-19 20:30 PST by Sam Weinig
Modified: 2011-12-21 10:51 PST (History)
3 users (show)

See Also:


Attachments
Patch (13.68 KB, patch)
2011-12-19 20:34 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (13.63 KB, patch)
2011-12-19 20:45 PST, Sam Weinig
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2011-12-19 20:30:57 PST
Add support for scrollLineDown: and scrollLineUp: NSResponder selectors
Comment 1 Sam Weinig 2011-12-19 20:34:29 PST
Created attachment 119980 [details]
Patch
Comment 2 Sam Weinig 2011-12-19 20:45:04 PST
Created attachment 119983 [details]
Patch
Comment 3 Alexey Proskuryakov 2011-12-20 15:47:34 PST
Comment on attachment 119983 [details]
Patch

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

Do we need this in WebKit1? Without a rationale, I can't tell.

> Tools/ChangeLog:8
> +        Add ScrollByLineCommands API test.

Why an API test? We have plenty of tests for commands that use layoutTestController.execCommand (and can thus see all commands, not just the one that are available to JS).
Comment 4 mitz 2011-12-20 15:51:46 PST
(In reply to comment #3)
> (From update of attachment 119983 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119983&action=review
> 
> Do we need this in WebKit1?

WebFrameView implements this.
Comment 5 Sam Weinig 2011-12-20 18:01:19 PST
(In reply to comment #3)
> Do we need this in WebKit1? Without a rationale, I can't tell.

This is implemented without WebCore involvement in WebKit1 (in WebFrameView as Dan noted), but this seemed like a more sound approach for WebKit2.

> > Tools/ChangeLog:8
> > +        Add ScrollByLineCommands API test.
> 
> Why an API test? We have plenty of tests for commands that use layoutTestController.execCommand (and can thus see all commands, not just the one that are available to JS).

I didn't remember we have that.  I can certainly convert the test/add one to LayoutTests.
Comment 6 Alexey Proskuryakov 2011-12-20 21:52:48 PST
In WK1, most NSResponder selectors go through WEBCORE_COMMAND in WebHTMLView.mm. I'm not sure how we chose what to implement there, and what in WebFrameView. Obviously, commands that need to work in non-HTML content go to WebFrameView, but why is the implementation so different?

For example, scrollPageUp: is in WebFrameView, while pageUp: is in WebHTMLView.

This patch does feel weird in that it uses WebHTMLView machinery to implement something that's not implemented in WebHTMLView. But I don't know if there is a deep reason there, or any effect on observable behavior.

Please test that the methods added here don't change selection in editable HTML content.

> I didn't remember we have that.  I can certainly convert the test/add one to LayoutTests.

Having a cross platform test would be useful. Maybe keeping an API test makes sense too, as execCommand won't test it for us. But then again, we don't test for any other selectors in API.
Comment 7 Darin Adler 2011-12-21 10:51:00 PST
(In reply to comment #6)
> In WK1, most NSResponder selectors go through WEBCORE_COMMAND in WebHTMLView.mm. I'm not sure how we chose what to implement there, and what in WebFrameView. Obviously, commands that need to work in non-HTML content go to WebFrameView, but why is the implementation so different?
> 
> For example, scrollPageUp: is in WebFrameView, while pageUp: is in WebHTMLView.

It’s just historical.

When I moved much code from WebHTMLView.mm into WebCore to make it cross-platform instead of Mac-specific and introduced WEBCORE_COMMAND I did not convert everything; Alice Liu and some others worked with me on it as well and it took a long time. Many pure scrolling functions were already implemented in WebFrameView and I did not attempt to make a 100% conversion to a new model for every last function. There were some scrolling subtleties in WebFrameView that I was not sure would be easy to translate, and I also slightly feared some incompatibility with apps that were going at specific WebKit views and depending on which handled a given selector.