Bug 46645 – Make scrolling work properly with writing modes

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

We'll have to decide how we want the scrollbars and such to look for various block-flow directions.

[Adele Peterson]

<rdar://problem/8612047>

[Dave Hyatt]

Created attachment 75303 [details]
Part 1: Generalize the scrollOrigin document concept to work with writing modes

[Dave Hyatt]

Created attachment 75305 [details]
Part 1: Generalize the scrollOrigin concept to work with both horizontal and vertical directions

[Dave Hyatt]

Created attachment 75306 [details]
Part 1: Generalize the scrollOrigin concept to work with both horizontal and vertical directions

Clean up the ChangeLogs.

[Darin Adler]

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.

[Dave Hyatt]

Created attachment 75308 [details]
Generalize the scrollOrigin concept to work with both horizontal and vertical directions 

Cleaned up some comments that no longer apply.

[Dave Hyatt]

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

[Dave Hyatt]

Created attachment 75309 [details]
Part 1: Generalize scrollOrigin concept to work with both horizontal and vertical text.

[Dave Hyatt]

Ok new patch posted that addresses Darin's concern and fixes the cross-platform code to be like the Mac WebDynamicScrollbarsView.

[Darin Adler]

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.

[WebKit Review Bot]

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

[Dave Hyatt]

Created attachment 75330 [details]
Part 2: Generalize scrollOrigin to work with both horizontal and vertical overflow renderers

[Dave Hyatt]

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.

[Eric Seidel (no email)]

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.