Bug 55077 - Regression: Content not drawn when scrolling horizontally in an RTL page
Summary: Regression: Content not drawn when scrolling horizontally in an RTL page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P1 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar, Regression
: 55889 (view as bug list)
Depends on: 53249
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-23 13:10 PST by Yair Yogev
Modified: 2011-03-10 14:29 PST (History)
7 users (show)

See Also:


Attachments
test page (230 bytes, text/html)
2011-02-23 13:10 PST, Yair Yogev
no flags Details
patch w/ layout test (74.95 KB, patch)
2011-03-07 18:09 PST, Xiaomei Ji
hyatt: review+
Details | Formatted Diff | Diff
Updated Patch (11.95 KB, patch)
2011-03-09 21:10 PST, Sam Weinig
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yair Yogev 2011-02-23 13:10:17 PST
Created attachment 83532 [details]
test page

When scrolling to the left in an RTL page, the newly revealed content is not drawn.

Reproducible using WebKit r79303 (only tested under Windows though)

I attached a dir=rtl page with a long line of text, scrolling horizontally reveals white space instead of the rest of the text. (the text is bounded by the words BEGIN and END)


the regression window is r76810-r76915
http://trac.webkit.org/log/?rev=76915&stop_rev=76810&verbose=on

maybe http://trac.webkit.org/changeset/76831/ is to blame?
Comment 1 mitz 2011-02-23 13:14:52 PST
<rdar://problem/9043791>
Comment 2 Yair Yogev 2011-02-23 13:19:31 PST
The horizontal scrollbar for RTL pages was first implemented in Bug 23556 .

Apparently there is a flaw in that implementation: links are not click-able in the newly revealed area.

The interesting part is that the recent regression that prevented the content from being drawn, made those (now invisible) links click-able.

Originally reported in http://crbug.com/73724
Comment 3 Yair Yogev 2011-03-03 15:10:52 PST
(i think)
Comment 4 Xiaomei Ji 2011-03-07 14:24:43 PST
The following seems fix the problem and the one reported in 55889.

Index: ScrollView.cpp
===================================================================
--- ScrollView.cpp      (revision 80210)
+++ ScrollView.cpp      (working copy)
@@ -980,7 +980,7 @@
     if (scrollX() < 0) {
         verticalOverhangRect.setWidth(-scrollX());
         verticalOverhangRect.setHeight(frameRect().height() - horizontalOverhan
gRect.height());
-        verticalOverhangRect.setX(frameRect().x());
+        verticalOverhangRect.setX(frameRect().x() + scrollX());
         if (horizontalOverhangRect.y() == frameRect().y())
             verticalOverhangRect.setY(frameRect().y() + horizontalOverhangRect.
height());
         else
Comment 5 Xiaomei Ji 2011-03-07 18:09:44 PST
Created attachment 85005 [details]
patch w/ layout test

I am not sure whether the position is set correctly in other 2 conditions (when scroll position is greater than the overflow value) inside ScrollView::calculateOverhangAreasForPainting().

And I do not know how to write a non-pixel test to test it out.
Comment 6 Dave Hyatt 2011-03-08 15:21:17 PST
Comment on attachment 85005 [details]
patch w/ layout test

r=me
Comment 7 Xiaomei Ji 2011-03-08 22:24:33 PST
Committed r80622: <http://trac.webkit.org/changeset/80622>
Comment 8 Xiaomei Ji 2011-03-08 22:54:10 PST
*** Bug 55889 has been marked as a duplicate of this bug. ***
Comment 9 Simon Fraser (smfr) 2011-03-09 14:15:44 PST
*** Bug 55889 has been marked as a duplicate of this bug. ***
Comment 10 Sam Weinig 2011-03-09 21:10:00 PST
Created attachment 85281 [details]
Updated Patch

I don't think the fix that went in was quite right, we really should be doing these checks taking the scroll origin into account.
Comment 11 WebKit Review Bot 2011-03-09 21:10:47 PST
Attachment 85281 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/ScrollableArea.h:130:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/ScrollableArea.h:131:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Dave Hyatt 2011-03-10 11:19:11 PST
Comment on attachment 85281 [details]
Updated Patch

r=me.  Fix the style queue issues.
Comment 13 Sam Weinig 2011-03-10 13:44:01 PST
Updated fix in 80757
Comment 14 Xiaomei Ji 2011-03-10 14:29:33 PST
Thanks for landing the right one!