Bug 85856

Summary:

[Chromium] Cannot show the left side of an RTL element with a vertical scrollbar

Product:

WebKit

Reporter:

Hironori Bono <hbono>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED WONTFIX

Severity:

Normal

CC:

aharon, cmarcelo, dglazkov, eric, hyatt, jchaffraix, leviw, pkasting, playmobil, progame+wk, rniwa, schenney, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
A test case	none
Patch v1	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-04	none
Patch v2 (used reftests)	tony: review-, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02	none
Patch v3 (replaced clientLeft with verticalScrollbarWidth())	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02	none
Patch v4 (added another test)	tony: review-, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-04	none
Patch v5 (applied a comment and fixed scrollWidth)	none
Patch v9 (updated to the top of trunk)	tony: review-
Patch v10 (Updated description)	none
Patch v11 (for landing)	none
Patch v12 (fixed regressions, but dirty)	none
An inconsistency between localtoAbsolute and the rendering position.	none

Hironori Bono

Reported 2012-05-07 21:24:55 PDT

Greetings, Unfortunately, my r109512 has a mapping problem from scrollLeft to the view point. In fact, Chrome cannot show the leftmost character as listed in the following reproduction step. What steps will reproduce the problem? 1. Open the attached file 'rtl-div.html'. What is the expected output? Chrome shows all characters '0123456789'. What do you see instead? Chrome cannot show the leftmost character '0'. Regards, Hironori Bono

Attachments
A test case (404 bytes, text/html) 2012-05-07 21:25 PDT, Hironori Bono	no flags	Details
Patch v1 (21.38 KB, patch) 2012-05-21 23:26 PDT, Hironori Bono	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-04 (585.93 KB, application/zip) 2012-05-22 01:27 PDT, WebKit Review Bot	no flags	Details
Patch v2 (used reftests) (5.82 KB, patch) 2012-05-23 03:16 PDT, Hironori Bono	tony: review- webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.01 MB, application/zip) 2012-05-23 04:13 PDT, WebKit Review Bot	no flags	Details
Patch v3 (replaced clientLeft with verticalScrollbarWidth()) (5.82 KB, patch) 2012-05-25 03:07 PDT, Hironori Bono	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-02 (597.13 KB, application/zip) 2012-05-25 04:19 PDT, WebKit Review Bot	no flags	Details
Patch v4 (added another test) (8.60 KB, patch) 2012-05-30 02:45 PDT, Hironori Bono	tony: review- webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-04 (766.85 KB, application/zip) 2012-05-30 07:07 PDT, WebKit Review Bot	no flags	Details
Patch v5 (applied a comment and fixed scrollWidth) (12.92 KB, patch) 2012-07-04 03:10 PDT, Hironori Bono	no flags	Details Formatted Diff Diff
Patch v9 (updated to the top of trunk) (13.18 KB, patch) 2012-07-04 18:58 PDT, Hironori Bono	tony: review-	Details Formatted Diff Diff
Patch v10 (Updated description) (13.10 KB, patch) 2012-07-11 01:23 PDT, Hironori Bono	no flags	Details Formatted Diff Diff
Patch v11 (for landing) (13.05 KB, patch) 2012-07-17 23:05 PDT, Hironori Bono	no flags	Details Formatted Diff Diff
Patch v12 (fixed regressions, but dirty) (16.97 KB, patch) 2012-08-14 00:14 PDT, Hironori Bono	no flags	Details Formatted Diff Diff
An inconsistency between localtoAbsolute and the rendering position. (108.44 KB, image/jpeg) 2012-08-15 23:32 PDT, Hironori Bono	no flags	Details
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.

Hironori Bono

Comment 1 2012-05-07 21:25:34 PDT

Created attachment 140664 [details] A test case Oops, I forgot attaching the file.

Hironori Bono

Comment 2 2012-05-21 23:26:09 PDT

Created attachment 143198 [details] Patch v1 Greetings, Even though I have quickly written a fix and a layout test, I'm not sure if this layout test succeeds on EWS bots. (Expected images created on my PCs do not work on EWS bots, somehow.) I would like to set r? to run this change on EWS bots so I can see their results. Regards, Hironori Bono

WebKit Review Bot

Comment 3 2012-05-22 01:27:33 PDT

Comment on attachment 143198 [details] Patch v1 Attachment 143198 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12739743 New failing tests: fast/block/float/028.html fast/block/float/026.html scrollbars/rtl/rtl-div.html

WebKit Review Bot

Comment 4 2012-05-22 01:27:38 PDT

Created attachment 143221 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Tony Chang

Comment 5 2012-05-22 10:22:57 PDT

Comment on attachment 143198 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=143198&action=review > LayoutTests/scrollbars/rtl/rtl-div.html:1 > +<!DOCTYPE html> Please use a ref test. We don't want to check in pixel results if we can easily avoid it.

Hironori Bono

Comment 6 2012-05-23 03:16:29 PDT

Created attachment 143519 [details] Patch v2 (used reftests) Greetings Tony, Thanks for your review and comments. After uploading my previous change, I noticed it might be better to fix the clientLeft attribute as written in <http://crbug.com/128500> and use it instead of using scroll-bar width directly. I have also changed its layout test to a reftest. (This test should pass both whether USE(RTL_SCROLLBAR) is enabled.) Regards, Hironori Bono

WebKit Review Bot

Comment 7 2012-05-23 04:13:09 PDT

Comment on attachment 143519 [details] Patch v2 (used reftests) Attachment 143519 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12768176 New failing tests: http/tests/cookies/simple-cookies-expired.html fast/text/international/rtl-white-space-pre-wrap.html fast/css/text-overflow-ellipsis.html fast/overflow/overflow-rtl-inline-scrollbar.html fast/regions/overflow-size-change-with-stacking-context-rtl.html fast/overflow/overflow-rtl.html fast/css/text-overflow-ellipsis-strict.html fast/block/float/026.html editing/spelling/inline_spelling_markers.html fast/text/international/unicode-bidi-plaintext-in-textarea.html fast/events/offsetX-offsetY.html fast/overflow/unreachable-overflow-rtl-bug.html fast/block/float/028.html

WebKit Review Bot

Comment 8 2012-05-23 04:13:19 PDT

Created attachment 143526 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Tony Chang

Comment 9 2012-05-23 09:32:45 PDT

Comment on attachment 143519 [details] Patch v2 (used reftests) View in context: https://bugs.webkit.org/attachment.cgi?id=143519&action=review > Source/WebCore/rendering/RenderBlock.cpp:2869 > + if (hasOverflowClip()) { > scrolledOffset.move(-scrolledContentOffset()); > + if (style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft()) > + scrolledOffset.move(clientLeft(), 0); It looks like if we move by clientLeft, we also move by the borderLeft. Can you add a test with a left border and make sure it works? > Source/WebCore/rendering/RenderBox.h:200 > + LayoutUnit clientLeft() const { return borderLeft() + (style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ? verticalScrollbarWidth() : 0); } Is this correct in vertical writing mode? The style parameter you check is for logical left, but clientLift is not logical left. Can you add a test for vertical writing mode? You might want to add the method clientLogicalStart to RenderBox.

Hironori Bono

Comment 10 2012-05-25 03:03:14 PDT

Comment on attachment 143519 [details] Patch v2 (used reftests) View in context: https://bugs.webkit.org/attachment.cgi?id=143519&action=review >> Source/WebCore/rendering/RenderBlock.cpp:2869 >> + scrolledOffset.move(clientLeft(), 0); > > It looks like if we move by clientLeft, we also move by the borderLeft. Can you add a test with a left border and make sure it works? D'oh. This is totally my mistake. Thanks for pointing it out. >> Source/WebCore/rendering/RenderBox.h:200 >> + LayoutUnit clientLeft() const { return borderLeft() + (style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ? verticalScrollbarWidth() : 0); } > > Is this correct in vertical writing mode? The style parameter you check is for logical left, but clientLift is not logical left. Can you add a test for vertical writing mode? You might want to add the method clientLogicalStart to RenderBox. Yes, shouldPlaceBlockDirectionScrollbarOnLogicalLeft() becomes false in the vertical-writing mode: <http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/style/RenderStyle.h&exact_package=chromium&l=987>. (Nevertheless, it is totally my mistake to use clientLeft() in RenderBlock::paintObject(), though.)

Hironori Bono

Comment 11 2012-05-25 03:07:25 PDT

Created attachment 144025 [details] Patch v3 (replaced clientLeft with verticalScrollbarWidth()) Greetings Tony, Thanks for your review and comments. Even though I have replaced clientLeft() with verticalScrollbarWidth(), I would like to run the updated one on EWS bots to figure out why my previous change broke several tests. (I will cancel this review request after EWS bots finish running tests.) Regards, Hironori Bono

WebKit Review Bot

Comment 12 2012-05-25 04:19:54 PDT

Comment on attachment 144025 [details] Patch v3 (replaced clientLeft with verticalScrollbarWidth()) Attachment 144025 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12791508 New failing tests: fast/block/float/028.html fast/events/offsetX-offsetY.html fast/overflow/unreachable-overflow-rtl-bug.html fast/block/float/026.html

WebKit Review Bot

Comment 13 2012-05-25 04:19:58 PDT

Created attachment 144043 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Tony Chang

Comment 14 2012-05-25 16:45:50 PDT

(In reply to comment #10) > (From update of attachment 143519 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143519&action=review > > Is this correct in vertical writing mode? The style parameter you check is for logical left, but clientLift is not logical left. Can you add a test for vertical writing mode? You might want to add the method clientLogicalStart to RenderBox. > > Yes, shouldPlaceBlockDirectionScrollbarOnLogicalLeft() becomes false in the vertical-writing mode: <http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/style/RenderStyle.h&exact_package=chromium&l=987>. (Nevertheless, it is totally my mistake to use clientLeft() in RenderBlock::paintObject(), though.) Can we add a test case for vertical writing mode?

Hironori Bono

Comment 15 2012-05-25 17:01:11 PDT

Greetings Tony, Thanks for your comment. Yes, I will upload another test for the vertical-writing mode. (I would like to investigate the test failures before uploading the next change, though.) Regards, Hironori Bono (In reply to comment #14) > Can we add a test case for vertical writing mode?

Hironori Bono

Comment 16 2012-05-30 02:45:02 PDT

Created attachment 144772 [details] Patch v4 (added another test) Greetings, I have added a layout test to test this change works with the vertical-writing mode. By the way, to investigate failing tests, the following tests (028.html, unreachable-overflow-rtl.html, 026.html) cannot show the left sides of RTL elements exactly due to this issue. (That is, this change fixed rendering problems of these tests and needs to rebaseline them.) fast/block/float/028.html fast/overflow/unreachable-overflow-rtl-bug.html fast/block/float/026.html On the other hand, 'offsetX-offsetY.html' sends mouse events to scrollbars and it fails when a vertical scrollbar is shown at the left side. (That is, we need to change the test itself.) fast/events/offsetX-offsetY.html Does this change need to include rebaselined results and test fixes? Regards, Hironori Bono

WebKit Review Bot

Comment 17 2012-05-30 07:07:50 PDT

Comment on attachment 144772 [details] Patch v4 (added another test) Attachment 144772 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12853593 New failing tests: fast/block/float/028.html fast/events/offsetX-offsetY.html fast/overflow/unreachable-overflow-rtl-bug.html fast/block/float/026.html

WebKit Review Bot

Comment 18 2012-05-30 07:07:54 PDT

Created attachment 144804 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Ryosuke Niwa

Comment 19 2012-06-01 00:24:26 PDT

Comment on attachment 144772 [details] Patch v4 (added another test) View in context: https://bugs.webkit.org/attachment.cgi?id=144772&action=review Could you figure out why tests are failing? Do they just need rebaselines? > Source/WebCore/ChangeLog:16 > + (WebCore::RenderBlock::paintObject): Move contents left when a vertical scrollbar is shown at the left side. You mean "move contents to the right"?

Ryosuke Niwa

Comment 20 2012-06-01 00:25:02 PDT

Levi is a WebKit reviewer now. Woohoo!

Tony Chang

Comment 21 2012-06-01 10:53:12 PDT

(In reply to comment #16) > Created an attachment (id=144772) [details] > Patch v4 (added another test) > > Greetings, > > I have added a layout test to test this change works with the vertical-writing mode. > By the way, to investigate failing tests, the following tests (028.html, unreachable-overflow-rtl.html, 026.html) cannot show the left sides of RTL elements exactly due to this issue. (That is, this change fixed rendering problems of these tests and needs to rebaseline them.) > fast/block/float/028.html > fast/overflow/unreachable-overflow-rtl-bug.html > fast/block/float/026.html > On the other hand, 'offsetX-offsetY.html' sends mouse events to scrollbars and it fails when a vertical scrollbar is shown at the left side. (That is, we need to change the test itself.) > fast/events/offsetX-offsetY.html > Does this change need to include rebaselined results and test fixes? Sorry about missing this earlier. Yes, please include the test fix for offsetX-offsetY.html and for the other tests, mark them in test_expectations.txt. For the other ports that need baselines, you'll have to watch the tree when the patch lands and grab the results off the bots.

Tony Chang

Comment 22 2012-06-01 10:54:11 PDT

Comment on attachment 144772 [details] Patch v4 (added another test) We should avoid making the chromium bots red via test_expectations.txt.

Hironori Bono

Comment 23 2012-06-27 23:59:27 PDT

Greetings Tony, Sorry for my slow response. I notice a fix for offsetX-offsetY.html becomes bigger than I thought and I have uploaded is fix as a separate change <http://webkit.org/b/73640>. (It takes long time to decrypt all the constants used by the test.) Regards, Hironori Bono

Hironori Bono

Comment 24 2012-07-04 03:10:14 PDT

Created attachment 150756 [details] Patch v5 (applied a comment and fixed scrollWidth) Greetings, While fixing offsetX-offsetY.html, I notice Chrome sets a wrong value to scrollWidth when it renders a vertical scrollbar at the left side of an RTL element. (It becomes smaller than the expected value by the width of a scrollbar.) This is the root cause of this issue. I have fixed this scrollWidth issue and replaced ref tests with script tests. Regards, Hironori Bono

Hironori Bono

Comment 25 2012-07-04 18:58:30 PDT

Created attachment 150857 [details] Patch v9 (updated to the top of trunk) Greetings, I have rebaselined my change to solve a conflict in TestExpectations.. Regards, Hironori Bono

Tony Chang

Comment 26 2012-07-09 10:05:53 PDT

Comment on attachment 150857 [details] Patch v9 (updated to the top of trunk) View in context: https://bugs.webkit.org/attachment.cgi?id=150857&action=review It looks like in IE, clientLeft when there is a scrollbar in a direction:rtl div is outside the scrollbar. clientWidth does not include the scrollbar. I think this is different from IE's behavior. Here's a test case: http://jsfiddle.net/QJGgS/ > Source/WebCore/ChangeLog:3 > + Move contents left when a vertical scrollbar is shown at the left side of an RTL element. The contents move to the right, no? > Source/WebCore/ChangeLog:11 > + This change also increases the clientLeft value by this scrollbar width and move > + contents left to improve compliance with CSSOM <http://www.w3.org/TR/cssom-view>. move contents left -> move contents right.

Hironori Bono

Comment 27 2012-07-09 21:12:13 PDT

Comment on attachment 150857 [details] Patch v9 (updated to the top of trunk) View in context: https://bugs.webkit.org/attachment.cgi?id=150857&action=review >> Source/WebCore/ChangeLog:3 >> + Move contents left when a vertical scrollbar is shown at the left side of an RTL element. > > The contents move to the right, no? Right. I have pasted the original (and wrong) description here. >> Source/WebCore/ChangeLog:11 >> + contents left to improve compliance with CSSOM <http://www.w3.org/TR/cssom-view>. > > move contents left -> move contents right. Apologies for this mistake again. (Even though I thought I fixed this typo, I pasted the original (and wrong) sentence when updating this description.)

Hironori Bono

Comment 28 2012-07-11 01:23:48 PDT

Created attachment 151640 [details] Patch v10 (Updated description) Greetings Tony, Thanks for your review and comments. As you notice, this change does not emulate IE9 because it emulates Firefox as suggested by Chromium Bug 128500 <http://crbug.com/128500>. (To read <http://www.w3.org/TR/cssom-view/#client-attributes> straightforwardly, this seems to be the expected behavior.) Regards, Hironori Bono (In reply to comment #26) > It looks like in IE, clientLeft when there is a scrollbar in a direction:rtl div is outside the scrollbar. clientWidth does not include the scrollbar. I think this is different from IE's behavior.

Tony Chang

Comment 29 2012-07-11 10:26:25 PDT

Comment on attachment 151640 [details] Patch v10 (Updated description) View in context: https://bugs.webkit.org/attachment.cgi?id=151640&action=review Thanks for finding the spec for this. Since IE is the only one with a different behavior and since having clientLeft be to the right of the scrollbar makes more sense, this looks correct to me. Can you add some explicit tests for clientLeft (could be a separate patch)? > LayoutTests/scrollbars/rtl/div-horizontal.html:36 > +var scrollbarWidthRTL = outerRTL.clientLeft > 0 ? outerRTL.clientLeft : outerRTL.offsetWidth - outerRTL.clientWidth; Why do we need the outerRTL.clientLeft > 0 check? Won't outerRTL.offsetWidth - outerRTL.clientWidth always give the width of the scrollbar? > LayoutTests/scrollbars/rtl/div-vertical.html:36 > +var scrollbarWidthRTL = outerRTL.clientLeft > 0 ? outerRTL.clientLeft : outerRTL.offsetWidth - outerRTL.clientWidth; Same question about the clientLeft check.

Hironori Bono

Comment 30 2012-07-17 22:55:07 PDT

Comment on attachment 151640 [details] Patch v10 (Updated description) View in context: https://bugs.webkit.org/attachment.cgi?id=151640&action=review >> LayoutTests/scrollbars/rtl/div-horizontal.html:36 >> +var scrollbarWidthRTL = outerRTL.clientLeft > 0 ? outerRTL.clientLeft : outerRTL.offsetWidth - outerRTL.clientWidth; > > Why do we need the outerRTL.clientLeft > 0 check? Won't outerRTL.offsetWidth - outerRTL.clientWidth always give the width of the scrollbar? Yes, you are totally right and this is caused by my misunderstanding. (I too much focused on using clientLeft when its scrollbar is at the left side. In fact, offsetWidth - clientWidth always represents the scrollbar width.)

Hironori Bono

Comment 31 2012-07-17 23:05:55 PDT

Created attachment 152935 [details] Patch v11 (for landing)

Hajime Morrita

Comment 32 2012-07-18 19:08:32 PDT

Comment on attachment 152935 [details] Patch v11 (for landing) on behalf of tony..

WebKit Review Bot

Comment 33 2012-07-18 19:51:51 PDT

Comment on attachment 152935 [details] Patch v11 (for landing) Clearing flags on attachment: 152935 Committed r123067: <http://trac.webkit.org/changeset/123067>

Peter Kasting

Comment 34 2012-07-28 09:47:00 PDT

Comment on attachment 151640 [details] Patch v10 (Updated description) Clearing flags on this obsolete patch.

Peter Kasting

Comment 35 2012-07-28 09:47:19 PDT

Closing as all reviewed patches have landed.

Hironori Bono

Comment 36 2012-08-06 22:16:23 PDT

Greetings, I would like to reopen this issue because I rolled out my r123067 manually. (Due to conflicts, I could not use the sheriff bot to roll it out.) Regards, Hironori Bono

Hironori Bono

Comment 37 2012-08-14 00:14:30 PDT

Created attachment 158240 [details] Patch v12 (fixed regressions, but dirty) Greetings, I have re-implemented this change so it does not cause known regressions (http://webkit.org/b/91756 and http://crbug.com/139486). Unfortunately, this change becomes a little complicated and I would like to hear ideas that simplify this change. Regards, Hironori Bono

Tony Chang

Comment 38 2012-08-14 20:41:37 PDT

Comment on attachment 158240 [details] Patch v12 (fixed regressions, but dirty) View in context: https://bugs.webkit.org/attachment.cgi?id=158240&action=review > Source/WebCore/ChangeLog:27 > + (WebCore::RenderBlock::paintChildren): Move the paint rectangles of RTL children right. It feels wrong that we're moving the paint rectangles. Why is it better to move the paint offset rather than changing the RenderObject's offset? > Source/WebCore/dom/Element.cpp:420 > + if (RenderBox* renderer = renderBox()) { > + LayoutUnit clientLeft = renderer->clientLeft(); Nit: I would change this to early return if renderer is 0.

Hironori Bono

Comment 39 2012-08-15 23:31:55 PDT

Greetings Tony, Thank you for your comment. Yes, this is exactly the dirty part of this fix. To describe the background of this code, RenderBlock::paintChildren() cannot render to the place referred by the offset of the RanderObject class somehow when we move the top-left origin. (This function assumes static-positioned children always starts from (borderLeft + paddingLeft, borderTop + paddingTop)?) (That is, this change sets the offset correctly.) On the other hand, localToAbsolute() returns the expected position without this code. So, without this code, RenderBlock::paintChildren() paints RTL children to the different places from the ones returned by localToAbsolute() and it causes the rendering problem <http://crbug.com/139486>. (It is easy to show this inconsistency with DevTool and select an element.) Regards, Hironori Bono (In reply to comment #38) > It feels wrong that we're moving the paint rectangles. Why is it better to move the paint offset rather than changing the RenderObject's offset?

Hironori Bono

Comment 40 2012-08-15 23:32:52 PDT

Created attachment 158719 [details] An inconsistency between localtoAbsolute and the rendering position.

Tony Chang

Comment 41 2012-08-16 00:21:00 PDT

Hmm, there must be some other way to fix this. Maybe Hyatt or Eric or someone else who understands scrollbars better than me can provide a suggestion.

Tony Chang

Comment 42 2012-08-16 00:21:56 PDT

Julien may also have some suggestions.

Hironori Bono

Comment 43 2012-08-16 00:24:25 PDT

Greetings Tony, I agree. There should be a better way and this is the reason why I uploaded this change. Regards, Hironori Bono (In reply to comment #41) > Hmm, there must be some other way to fix this. Maybe Hyatt or Eric or someone else who understands scrollbars better than me can provide a suggestion.

Julien Chaffraix

Comment 44 2012-10-22 14:28:23 PDT

Comment on attachment 158240 [details] Patch v12 (fixed regressions, but dirty) View in context: https://bugs.webkit.org/attachment.cgi?id=158240&action=review > Source/WebCore/ChangeLog:12 > + layout offsets. 'right' is used 5 times and the way it's written is confusing: I had a hard time saying if you are talking about the direction and just make them 'right'. >> Source/WebCore/ChangeLog:27 >> + (WebCore::RenderBlock::paintChildren): Move the paint rectangles of RTL children right. > > It feels wrong that we're moving the paint rectangles. Why is it better to move the paint offset rather than changing the RenderObject's offset? I agree with Tony here: we should probably patch the offsets at layout time and store them updated instead of requiring this code. Your approach only invites bugs in our code as we can easily forget a code path. I couldn't find any in the repaint code, but hit-testing looks suspicious as only RenderBlock was updated (but neither RenderReplaced nor RenderLayer was). > Source/WebCore/dom/Element.cpp:421 > + if (renderer->style() && renderer->style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft()) Can we really have renderer->style() be NULL? Do you know when this occurs? > Source/WebCore/rendering/RenderBlock.cpp:1681 > + addOverflowFromChild(positionedObject, IntSize(positionedObject->x(), positionedObject->y())); This should be LayoutSize. > Source/WebCore/rendering/RenderBox.cpp:1576 > + if (o->style() && o->style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() && o->isBox()) Again why do you NULL check style() here when none of the surrounding code does this check? Also it should be |styleToUse|, we cache style() (IIRC it is because region may have made this call more expensive).

Aharon (Vladimir) Lanin

Comment 45 2013-06-16 02:42:15 PDT

This is a real bug and should be fixed.

Levi Weintraub

Comment 46 2013-07-15 16:52:39 PDT

(In reply to comment #45) > This is a real bug and should be fixed. Yes, but it's Chromium-specific. We no longer use the WebKit bug tracker. We need a crbug.com bug to track this, instead.

Note You need to log in before you can comment on or make changes to this bug.