Summary: | Make scrolling work properly with writing modes | ||
---|---|---|---|
Product: | WebKit | Reporter: | Dave Hyatt <hyatt> |
Component: | Layout and Rendering | Assignee: | 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
Dave Hyatt
2010-09-27 12:48:31 PDT
Created attachment 75303 [details]
Part 1: Generalize the scrollOrigin document concept to work with writing modes
Created attachment 75305 [details]
Part 1: Generalize the scrollOrigin concept to work with both horizontal and vertical directions
Created attachment 75306 [details]
Part 1: Generalize the scrollOrigin concept to work with both horizontal and vertical directions
Clean up the ChangeLogs.
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. Created attachment 75308 [details]
Generalize the scrollOrigin concept to work with both horizontal and vertical directions
Cleaned up some comments that no longer apply.
(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. Created attachment 75309 [details]
Part 1: Generalize scrollOrigin concept to work with both horizontal and vertical text.
Ok new patch posted that addresses Darin's concern and fixes the cross-platform code to be like the Mac WebDynamicScrollbarsView. 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. 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 Created attachment 75330 [details]
Part 2: Generalize scrollOrigin to work with both horizontal and vertical overflow renderers
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 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. |