Hi, gecko has window.scrollByLines, and window.scrollByPages methods, which are nice to use. Currently, webkit only has window.scrollBy. I wonder if that's a good idea to add those methods to webkit.
Created attachment 22686 [details] window.scrollByLines + window.scrollByPages Here is a patch proposal. It's possibly naive, as I don't know webkit well. I'm not aware of difficulties with all platforms. Anyway, it seems to work fine on my system (webkit gtk). To get line height, I used lineSpacing defaulting to 12pt in case it was impossible to compute. From other parts of webkit, I had the feeling that using was not possible with qt, (see WebCore/platform/graphics/Font.h) but I don't know why. For page height, I used mozilla algorithm: the size of the page minus the smaller of 10% of the size or 2 lines. I'd be happy to have feedback on my first proposal to work again on it if needed.
Comment on attachment 22686 [details] window.scrollByLines + window.scrollByPages Thanks, this patch looks really good! +2008-08-06 arno <arenevier@fdn.fr> We'd prefer to have your full name here if that's OK. + Tests for window.scrollByLines and window.scrollByPages + implements window.scrollByPages and window.scrollByLines DOM level 0 + methods These lines have tabs in them. We can't commit patches with tab characters (Subversion is set to not allow it), so it's best if you give use changes that don't have them. #include "SecurityOrigin.h" +#include "RenderView.h" +#include "SimpleFontData.h" The lists of included files should be kept sorted, so RenderView should be moved up a few lines. The DOMWindow class is supposed to simply be the DOM interface -- the actual scrolling function should be in the FrameView class and DOMWindow should simply forward the calls there. +#if !PLATFORM(QT) + RenderView *renderer = m_frame->contentRenderer(); Why is Qt different here? + int linespacing = style->font().primaryFont()->lineSpacing(); + scrollBy(0, linespacing * y); You should capitalize your local variable like the function: lineSpacing. Should we really base the definition of a line on the default font size? Is there a better technique? Is this what other browsers do? How does this definition of a page relate to what other browsers do? Can we make this share code with the code that scrolls a line or a page when you hit the arrow keys and page up and page down keys? There are enough issues that I'm going to say review- for now, but this seems about ready to go.
Sorry, I marked this bug as "security sensitive" by accident. I was actually just trying to "cc" myself on the bug!
Created attachment 23055 [details] patch v2 Thanks for you useful comments. Now, main code is in FrameView, and DOMWindow just forwards it. > Why is Qt different here? because in WebCore/platform/graphics/Font.h, primaryFont method does not seem to be defined for Qt, so I fear it won't compile on qt. May be I misunderstood something, and primaryFont is defined otherwise, but I did not found where. > Should we really base the definition of a line on the default font size? Is > there a better technique? Is this what other browsers do? How does this > definition of a page relate to what other browsers do? Mozilla seems to be the only implementor of scrollByLines/scrollByPages; opera and ie do not define those methods. I tried to follow mozilla's implementation: line height is given by platform, defaults is 12pt. > Can we make this share code with the code that scrolls a line or a page when > you hit the arrow keys and page up and page down keys? I checked gtk backend, and it calls scrollBy with a fixed (40) number of pixels when hitting arrow key; it forwards keypress event to underlying gtk widget when hitting page down key. Windows code looks quite similar (number of pixels is 15). So, I don't see how to share code. Problem with current patch is that scrollByLines is not coherent with hitting arrow key (ie: it does not scroll same number of pixels); also, scrollByPages is not coherent with hitting page down. If that's important, may be, DOMWindow could forward directly to platform ScrollView, and each platform would handle it their own way.
(In reply to comment #4) > Problem with current patch is that scrollByLines is not coherent with hitting > arrow key (ie: it does not scroll same number of pixels); also, scrollByPages > is not coherent with hitting page down. If that's important, may be, DOMWindow > could forward directly to platform ScrollView, and each platform would handle > it their own way. I think that is important. But I think calling out to the platform is not the right design going forward. The right way to do it is to put the line and page scrolling into WebCore so it's shared cross-platform. I think the reason it's currently separate for each platform is mostly historical -- it was in WebKit on Mac OS X and it was one of the things we didn't move into WebCore. If there are important platform differences we can give platforms control, but I suspect the current differences are mostly arbitrary and not worth preserving. However, it's not necessary to do all these things at once. For now, forwarding to the same code that handles keyboard scrolling on all the platforms would be OK. I don't like the idea, though, of introducing duplicate code now, so I'd prefer that we do one of the things I mention above.
Comment on attachment 23055 [details] patch v2 822 Document* doc = m_frame->document(); 823 ASSERT(doc); 824 if (doc) 825 doc->updateLayoutIgnorePendingStylesheets(); This part of the code also belongs inside FrameView. I'm going to say review- so you can move that code at a minimum. But also, please investigate making the platforms share the same logic moving by line and page by keyboard as for these functions. For example, the current definition of how far to scroll by a line or page on Mac OS X is in the WebFrameView class in the methods _verticalKeyboardScrollDistance and _verticalPageScrollDistance. That's the same distance scrollByLines and scrollByPages should use. Maybe we do want to delegate this question to the client, because it does seem that the Mac OS X methods are doing things that are specific to the platform.
Created attachment 23085 [details] updated patch > This part of the code also belongs inside FrameView. Ok, I've updated my patch. To share code between scrollByLines, and keypress handlers, I've though about two ways: * with current patch, platform could call scrollByLines or scrollByPages methods. I don't understand mac specific code, so I don't known if that would be possible, but it seems at least possible for gtk, qt and win platforms. A cast would be needed to use those method from scrollview implementations (in ScrollView::wheelEvent for example). For those methods to be useful, scrollByLines could take two arguments (x and y) to handle left and right arrow. Therefore, I'll attach an alternative patch to add an second argument to scrollByLines. * or, scrollByLines/scrollByPages could be dispatched to platform specific scrollView. Then each platform would implement those method, and refactor some code if they need. So, I'll attach another alternative patch that dispatch scrollByLines/scrollByPages to currently notImplemented scrollView methods. Is one of those methods right, or is there a better way to do ?
Created attachment 23086 [details] same patch with a second argument for scrollByLines
Created attachment 23087 [details] dispatch methods to currently notImplemented platform specific methods
Were these patches intended for review? Please mark them as such by clicking the Edit link (right to each patch) and setting the flag.
Comment on attachment 23085 [details] updated patch +using std::min; Normally we'd just do "using namespace std". + // Line increment is given line height as reported by platform. Fallbacks + // to 12pt This should be "Falls back to 12 pt." rather than "Fallbacks to 12pt." I also think that there's no good reason to quite this figure in "pt". We should say "falls back to 16 pixels". I'd like to say a FIXME that says "this should share the code with the keyboard bindings that do the same thing". +#if !PLATFORM(QT) This #if is in the wrong place. It should be around the entire function except for the call to scrollBy. It's also horrible that we need this! + doc->updateLayoutIgnorePendingStylesheets(); Why are we doing updateLayout in scrollByLines? Is it that we are trying to force the renderer be created? If so then we need to get the renderer *after* calling updateLayoutIgnorePendingStylesheets. + int lineSpacing = 0; +#if !PLATFORM(QT) + RenderView *renderer = m_frame->contentRenderer(); + if (renderer) { + RenderStyle* style = renderer->style(); + if (style) { + lineSpacing = style->font().primaryFont()->lineSpacing(); + } + } + if (lineSpacing == 0) +#endif + lineSpacing = 16; Why not just initialize lineSpacing to 16? I don't see the point in setting it to 0 and then 16. + void scrollByLines(int y); + void scrollByPages(int y); I think these arguments would be better off either without names or with clearer names. I'm going to say r=me despite these problems. It seems OK to land this.
Created attachment 23490 [details] updated patch Why are we doing updateLayout in scrollByLines? Is it that we are trying to force the renderer be created? Layout is not updated if I don't call updateLayout (ie: scrollbar does not move, and window view does not change). Here is a patch addressing your comments.
(In reply to comment #12) > Layout is not updated if I don't call updateLayout (ie: scrollbar does not > move, and window view does not change). Huh? That doesn't make sense. That's not what the updateLayout function is for, so there must be some misunderstanding here.
I'm confused by this patch. Why would you not just do what "spacebar" and what hitting the "down arrow / up arrow" do? We already have the concept of the correct scroll granularity for this operation.
(In reply to comment #14) > I'm confused by this patch. Why would you not just do what "spacebar" and what > hitting the "down arrow / up arrow" do? We already have the concept of the > correct scroll granularity for this operation. We have that separately in the platform-specific code for each platform. What we should do is move it into WebCore and have it share code with these functions.
Isn't the scroll() method on ScrollView usable for this? That method is cross-platform. bool scroll(ScrollDirection, ScrollGranularity);
(In reply to comment #16) > Isn't the scroll() method on ScrollView usable for this? That method is > cross-platform. > > bool scroll(ScrollDirection, ScrollGranularity); Hey, that looks good! We should use that if it works.
(In reply to comment #17) > (In reply to comment #16) > > Isn't the scroll() method on ScrollView usable for this? That method is > > cross-platform. > > > > bool scroll(ScrollDirection, ScrollGranularity); > > Hey, that looks good! We should use that if it works. It's cross-platform in theory, but there's no Mac implementation.
> Huh? That doesn't make sense. That's not what the updateLayout function is for, > so there must be some misunderstanding here. At first, I used this function because it used in DOMWindow::scrollBy. Then I noticed that it's necessary to call it; otherwise, view is not scrolled. > Isn't the scroll() method on ScrollView usable for this? That method is > cross-platform. I had tried to use it, but did not succeed. It seems that it can only be used when scrolling within a region with scrollbars, but not to scroll whole document. For example, in gtk port. bool ScrollView::scroll(ScrollDirection direction, ScrollGranularity granularity) { ... if (m_data->vBar) return m_data->vBar->scroll(direction, granularity); m_data->vBar is null if calling from FrameView::scrollByLines I think window port behaves the same way. I've made some attempts toward code sharing. See comment 7, and attachment 23086 [details] and attachment 23087 [details]
Comment on attachment 23490 [details] updated patch Hyatt just re-wrote large hunks of the scrollbar code, it may be possible to write this differently now. A couple nits: +void DOMWindow::scrollByLines(int y) const +void DOMWindow::scrollByPages(int y) const Really should have better argument names, like: +void DOMWindow::scrollByLines(int lines) const +void DOMWindow::scrollByPages(int pages) const Same here: + void scrollByLines(int y) const; + void scrollByPages(int y) const; And here: + [RequiresAllArguments] void scrollByLines(in long y); + [RequiresAllArguments] void scrollByPages(in long y); This differs from WebKit style: + RenderView *renderer = m_frame->contentRenderer(); This needs a comment (IMO): +#if !PLATFORM(QT) to explain that we can't turn this code on for Qt due to lack of primaryFont(). That said, the Qt guys were just in teh middle of re-writing the qt font code to be more like all the other platforms. I'm not sure if that's landed or not yet. If it has, those #ifs can go away. :)
Comment on attachment 23490 [details] updated patch With Hyatt doing so much work in this area right now, I think he has to be the one to review this.
Comment on attachment 23490 [details] updated patch This should use the scroll() method on ScrollView with the appropriate granularity/direction. That method should be implemented on Mac.
If you can't implement the Mac version, I think it would be ok to do a temporary hack for !PLATFORM(MAC) to work around that platform's lack of a scroll() method implementation, but every other platform can just use scroll() instead. Just remember to account for the fact that our line multiple is 3.
Can you elaborate on this? "> Isn't the scroll() method on ScrollView usable for this? That method is > cross-platform. I had tried to use it, but did not succeed. It seems that it can only be used when scrolling within a region with scrollbars, but not to scroll whole document. For example, in gtk port." Maybe I am not understanding something here.
scroll can be amended to take an additional float multiplier argument (defaulting to 1) that would allow you to scroll by a multiple of lines/pages. Then scrollByLines should pass either n or n / 3 as the multiplier (should compare to Gecko to see what they do... I suspect they may consider a "line" to be about 40px like we do, in which case n would be fine rather than n / 3). scrollByPages could just pass n. Your other option if you want to avoid using scroll() is to just do scrollBy and use the line/page constants from Scrollbar.h. const int cScrollbarPixelsPerLineStep = 40; const float cMouseWheelPixelsPerLineStep = 40.0f / 3.0f; const int cAmountToKeepWhenPaging = 40; A "line" is always either 40 (if we use the single line idea of hitting the down arrow on the scrollbar for example), or 40 / 3 (if we want the literal definition of a single line... used only by wheeling currently). A "page" is always the visibleHeight() of the ScrollView minus cAmountToKeepWhenPaging.
>> I had tried to use it, but did not succeed. It seems that it can only be used >> when scrolling within a region with scrollbars, but not to scroll whole >> document. > Maybe I am not understanding something here. When trying to implement DOMWindow::scrollByPages as follow (it's for testing purpose, I known for a real implementation, I should add a parameter to scroll { if (!m_frame) return; FrameView* view = m_frame->view(); if (!view) return; int i = 0; for (i = 0; i < lines; i++) { view->scroll(ScrollUp, ScrollByLine); } } it does nothing. That's because m_verticalScrollbar is null in ScrollView::scroll. Do I call scroll in a wrong way ? Is that a bug in gtk backend ? Is that intended behaviour ?
Comment on attachment 23085 [details] updated patch Clearing review flag on this obsoleted patch so this doesn't show up in the "reviewed, need to be committed" queue.
https://developer.mozilla.org/en-US/docs/Web/API/Window/scrollByPages - Considered as Non-Standard. Similarly - https://developer.mozilla.org/en-US/docs/Web/API/Window/scrollByLines only Firefox implemented them, not even Internet Explorer. https://caniuse.com/?search=scrollby > Are there any plan as part of InterOp to standardise them or this bug can be marked as "RESOLVED WONTFIX" due to superior solutions being available in newer web-specs? Thanks!
Let's close this as Later. We can always consider adding them if they come up in any compatibility testing.