Bug 46645

Summary: Make scrolling work properly with writing modes
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, eric, sam, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 46123    
Attachments:
Description Flags
Part 1: Generalize the scrollOrigin document concept to work with writing modes
none
Part 1: Generalize the scrollOrigin concept to work with both horizontal and vertical directions
none
Part 1: Generalize the scrollOrigin concept to work with both horizontal and vertical directions
none
Generalize the scrollOrigin concept to work with both horizontal and vertical directions
none
Part 1: Generalize scrollOrigin concept to work with both horizontal and vertical text.
none
Part 2: Generalize scrollOrigin to work with both horizontal and vertical overflow renderers mitz: review+

Description Dave Hyatt 2010-09-27 12:48:31 PDT
We'll have to decide how we want the scrollbars and such to look for various block-flow directions.
Comment 1 Adele Peterson 2010-10-29 15:13:34 PDT
<rdar://problem/8612047>
Comment 2 Dave Hyatt 2010-12-01 12:24:31 PST
Created attachment 75303 [details]
Part 1: Generalize the scrollOrigin document concept to work with writing modes
Comment 3 Dave Hyatt 2010-12-01 12:32:53 PST
Created attachment 75305 [details]
Part 1: Generalize the scrollOrigin concept to work with both horizontal and vertical directions
Comment 4 Dave Hyatt 2010-12-01 12:35:56 PST
Created attachment 75306 [details]
Part 1: Generalize the scrollOrigin concept to work with both horizontal and vertical directions

Clean up the ChangeLogs.
Comment 5 Darin Adler 2010-12-01 12:40:14 PST
Comment on attachment 75306 [details]
Part 1: Generalize the scrollOrigin concept to work with both horizontal and vertical directions

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

> WebCore/platform/ScrollView.cpp:1046
> +    // Check to see if either the direction or writing-mode has changed such that our scroll origin has gone from
> +    // zero to non-zero (or vice versa) in a particular axis.  If so, just reset the scroll position to the scroll
> +    // origin.
> +    if ((!prevScrollOrigin.x() && m_scrollOrigin.x()) || (!m_scrollOrigin.x() && prevScrollOrigin.x()) ||
> +        (!prevScrollOrigin.y() && m_scrollOrigin.y()) || (!m_scrollOrigin.y() && prevScrollOrigin.y()))
>          updateScrollbars(scrollOffset());

I don’t understand why this is right.
Comment 6 Dave Hyatt 2010-12-01 12:42:34 PST
Created attachment 75308 [details]
Generalize the scrollOrigin concept to work with both horizontal and vertical directions 

Cleaned up some comments that no longer apply.
Comment 7 Dave Hyatt 2010-12-01 12:48:45 PST
(In reply to comment #5)
> (From update of attachment 75306 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75306&action=review
> 
> > WebCore/platform/ScrollView.cpp:1046
> > +    // Check to see if either the direction or writing-mode has changed such that our scroll origin has gone from
> > +    // zero to non-zero (or vice versa) in a particular axis.  If so, just reset the scroll position to the scroll
> > +    // origin.
> > +    if ((!prevScrollOrigin.x() && m_scrollOrigin.x()) || (!m_scrollOrigin.x() && prevScrollOrigin.x()) ||
> > +        (!prevScrollOrigin.y() && m_scrollOrigin.y()) || (!m_scrollOrigin.y() && prevScrollOrigin.y()))
> >          updateScrollbars(scrollOffset());
> 
> I don’t understand why this is right.

Yeah that code is no longer needed.  I fixed the Mac-specific code to not need to do that, and the cross-platform code doesn't need it either.  Let me fix and post an update.
Comment 8 Dave Hyatt 2010-12-01 12:52:17 PST
Created attachment 75309 [details]
Part 1: Generalize scrollOrigin concept to work with both horizontal and vertical text.
Comment 9 Dave Hyatt 2010-12-01 12:52:56 PST
Ok new patch posted that addresses Darin's concern and fixes the cross-platform code to be like the Mac WebDynamicScrollbarsView.
Comment 10 Darin Adler 2010-12-01 12:58:43 PST
Comment on attachment 75309 [details]
Part 1: Generalize scrollOrigin concept to work with both horizontal and vertical text.

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

> WebCore/platform/ScrollView.cpp:1043
> +    IntPoint prevScrollOrigin = m_scrollOrigin;
>  
> -void ScrollView::updateScrollbars()
> -{
> -    if (!platformWidget())
> +    m_scrollOrigin = origin;
> +    
> +    if (platformWidget() || !updatePosition)
> +        return;
> +
> +    // Update if the scroll origin changes, since our position will be different if the content size did not change.
> +    if (prevScrollOrigin != m_scrollOrigin)
>          updateScrollbars(scrollOffset());

Seems like a cleaner way to write this is:

    if (origin == m_scrollOrigin)
        return;

    m_scrollOrigin = origin;

    ...

No need to have the check specifically wrapped around the updateScrollbars call.

> WebKit/mac/WebView/WebDynamicScrollBarsView.mm:68
> +    // scrollOrigin is set for various combinations of writing mode and direction.  See the comment next to the
> +    // corresponding member in ScrollView.h.
> +    NSPoint scrollOrigin;

We normally use one space after a period. I also suggest moving that second sentence to its own second line.

> WebKit/mac/WebView/WebDynamicScrollBarsView.mm:580
> +    NSPoint docOrigin = [docView bounds].origin;
> +    if (docOrigin.x != -scrollOrigin.x || docOrigin.y != -scrollOrigin.y) {

Early return would make this function read better, I think.

> WebKit/mac/WebView/WebDynamicScrollBarsView.mm:582
> +         

Looks like there was an extra space added here.
Comment 11 WebKit Review Bot 2010-12-01 14:02:09 PST
http://trac.webkit.org/changeset/73063 might have broken Qt Linux Release
The following tests are not passing:
fast/block/basic/truncation-rtl.html
fast/dom/horizontal-scrollbar-in-rtl.html
fast/dom/vertical-scrollbar-in-rtl.html
Comment 12 Dave Hyatt 2010-12-01 15:51:46 PST
Created attachment 75330 [details]
Part 2: Generalize scrollOrigin to work with both horizontal and vertical overflow renderers
Comment 13 Dave Hyatt 2010-12-01 16:14:55 PST
For 

fast/dom/vertical-scrollbar-in-rtl.html

platforms will need to just add their own specific results (just as was done with horizontal-scrollbar-in-rtl), since the HOME/END behavior is not the same on every platform.
Comment 14 Eric Seidel (no email) 2010-12-14 01:25:43 PST
Comment on attachment 75309 [details]
Part 1: Generalize scrollOrigin concept to work with both horizontal and vertical text.

Cleared Darin Adler's review+ from obsolete attachment 75309 [details] so that this bug does not appear in http://webkit.org/pending-commit.