(Mentioned this to smfr on IRC, he didn't know what to make of it either and suggested filing this bug) At the bottom of RenderLayer::updateScrollInfoAfterLayout() is some code that resets the horizontal and vertical scrollbars. The blocks are identical, except that the horizontal scrollbar gets a setValue() call and the vertical scrollbar doesn't. Is this a bug? If not can we comment about why? This dates from 2005, when darin checked in r11521 to fix bug 5826. Before this change, neither block had a setValue(); the change added the setValue() call to just the horizontal scrollbar block.
If we can find a symptom, then we can fix the bug. I don’t think we should change any code based on just seeing it’s different. I’ll look at the patch and see if I can remember anything from the distant past.
Yes, I wish I knew what this code did so I could figure out how to test this codepath. Of course, if I knew that much, I could just say whether the code was right :P
(In reply to comment #0) > This dates from 2005, when darin checked in r11521 to fix bug 5826. Note that the change r11521 was done by Mitz Pettel. I was just the committer. Back then I was the commit bot.
This code ensures that when a right-to-left scrollable area’s width (or content width) changes, the top right corner of the content doesn’t shift with respect to the top right corner of the area. Conceptually, right-to-left areas have their origin at the top-right, but RenderLayer is top-left oriented, so this is needed to keep everything working (see how scrollXOffset() differs from srollYOffsset() to get an idea of why the horizontal and vertical scrollbars need to be treated differently).
I’m pretty sure there are regression tests that cover this already, but here’s the simplest one: <div style="direction: rtl; width: 100px; overflow: auto;"> <div id="target" style="width: 150px; height: 100px;"></div> </div>
Setting a resolution based on Mitz’s comments. Or we could add a comment explaining this.
It'd be awesome to add a comment in the code. All my checkouts are in use at the moment, but if no one gets to it before me I'll add something like comment 4.
Created attachment 45213 [details] Add comment This patch basically adds mitz' comment verbatim into the code. I think this is really useful when trying to figure out why the code does what it does.
Attachment 45213 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/rendering/RenderLayer.cpp:1895: Line contains invalid UTF-8 (or Unicode replacement character). [readability/utf8] [5] Total errors found: 1
Created attachment 45214 [details] Add comment, v2 Fix apostrophe
style-queue ran check-webkit-style on attachment 45214 [details] without any errors.
Comment on attachment 45214 [details] Add comment, v2 Clearing flags on attachment: 45214 Committed r52379: <http://trac.webkit.org/changeset/52379>
All reviewed patches have been landed. Closing bug.