| 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: | P2 | ||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
|
Description
Hironori Bono
2012-05-07 21:24:55 PDT
Created attachment 140664 [details]
A test case
Oops, I forgot attaching the file.
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
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 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
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. 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 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 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
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. 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.) 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
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 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
(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? 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? 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
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 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
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"? Levi is a WebKit reviewer now. Woohoo! (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. Comment on attachment 144772 [details]
Patch v4 (added another test)
We should avoid making the chromium bots red via test_expectations.txt.
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 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
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
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. 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.) 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. 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. 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.) Created attachment 152935 [details]
Patch v11 (for landing)
Comment on attachment 152935 [details]
Patch v11 (for landing)
on behalf of tony..
Comment on attachment 152935 [details] Patch v11 (for landing) Clearing flags on attachment: 152935 Committed r123067: <http://trac.webkit.org/changeset/123067> Comment on attachment 151640 [details]
Patch v10 (Updated description)
Clearing flags on this obsolete patch.
Closing as all reviewed patches have landed. 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 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 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. 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? Created attachment 158719 [details]
An inconsistency between localtoAbsolute and the rendering position.
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 may also have some suggestions. 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. 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). This is a real bug and should be fixed. (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. |