Bug 74907 – Add support for scrollLineDown: and scrollLineUp: NSResponder selectors

• Status: NEW
• Component: HTML Editing

[Sam Weinig]

Add support for scrollLineDown: and scrollLineUp: NSResponder selectors

[Sam Weinig]

Created attachment 119980 [details]
Patch

[Sam Weinig]

Created attachment 119983 [details]
Patch

[Alexey Proskuryakov]

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).

(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.

[Sam Weinig]

(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.

[Alexey Proskuryakov]

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.

[Darin Adler]

(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.