| Summary: | RTL web content should have left-hand scrollbar. | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jeremy Moskovich <playmobil> | ||||||||||||||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, aharon, cc-bugs, dglazkov, eae, eric, haraken, hbono, hyatt, jamesr, leviw, mitz, morrita, ojan, playmobil, progame+wk, rniwa, simon.fraser, tonikitoo, webkit.review.bot, xji, yael | ||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||
| Bug Blocks: | 50910 | ||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Jeremy Moskovich
2011-02-17 00:57:47 PST
Is there a browser other than IE with scrollbars on the left hand side? A scrollbar moving from side to side throughout the browsing session can be annoying... I see it as a disadvantage but I guess it's a matter of taste. (In reply to comment #1) > A scrollbar moving from side to side throughout the browsing session can be annoying... I see it as a disadvantage but I guess it's a matter of taste. We can use the same side per frame. It probably doesn't make sense to change the side per block element. There have been extended discussions about this - Aharon can provide the full details. I think the current thinking is to only do this for scrollbars in web content, leaving the directionality of the main window scrollbar up to the OS. Created attachment 87158 [details]
An initial change for starting technical discussion.
Greetings,
This is a change that moves the horizontal scrollbars and resizers of an RTL controls. (This change only moves the horizontal scrollbar of the RenderLayer class and it does not move the one of the RenderView class, though.) I notice this change causes layout-test failures for several tests because this change also moves the text in an RTL control right to prevent the text from being rendered on its horizontal scrollbar.
fast/block/float/026.html -> failed
fast/block/float/028.html -> failed
fast/events/offsetX-offsetY.html -> failed
fast/overflow/overflow-rtl-vertical.html -> failed
fast/overflow/unreachable-overflow-rtl-bug.html -> failed
Regards,
Hironori Bono
(In reply to comment #4) > Created an attachment (id=87158) [details] > An initial change for starting technical discussion. > > Greetings, > > This is a change that moves the horizontal scrollbars and resizers of an RTL controls. it is moving the vertical scrollbar, right? Greetings, (In reply to comment #5) > it is moving the vertical scrollbar, right? Right. This change also moves the resizer, though. Regards, Hironori Bono Greetings, (In reply to comment #4) > fast/overflow/unreachable-overflow-rtl-bug.html -> failed Oops, this seems to be a real regression. (I forgot moving the float objects right by the width of the horizontal scrollbar.) Regards, Hironori Bono Comment on attachment 87158 [details] An initial change for starting technical discussion. View in context: https://bugs.webkit.org/attachment.cgi?id=87158&action=review I think the approach seems fine (assuming its mirrors what we do when scrollbars are on the right). But the code needs further cleanup to abstract these functions nicely. > Source/WebCore/rendering/RenderBlock.cpp:1751 > + if (style()->isHorizontalWritingMode() && !style()->isLeftToRightDirection()) This should be a style()->showScrollbarOnLeft() check. > Source/WebCore/rendering/RenderBox.cpp:479 > +IntRect RenderBox::contentBoxRect() const { style. { on next line. > Source/WebCore/rendering/RenderBox.cpp:481 > + if (!style()->isLeftToRightDirection()) Same here. We may decide this is not related to writign direction at a later time. Whether we show a scrollbar on the left or right shoudl be an independent function check. > Source/WebCore/rendering/RenderBox.cpp:1130 > + clipX += layer()->verticalScrollbarWidth(relevancy); > clipWidth -= layer()->verticalScrollbarWidth(relevancy); Seems we should cache this value instead of asking it twice... Created attachment 87748 [details]
A trial change
Greetings,
I have updated this change to add a CSS property that moves the vertical scrollbar to the left side. I would like to upload this change and set review? just to check if this change can complie on all platforms. (I understand I need to polish this change.) Sorry for my noisy review request in advance.
Regards,
Hironori Bono
Attachment 87748 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 1 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Why is a new CSS property added? Who are you expecting to set that property? I do not believe making this a CSS property is the right way to go. This should just be a global policy decision and not controllable by authors. I also don't believe we want this change on Mac OS X, since scrollbars stay on the right hand side even with RTL content in list boxes for example. Being inconsistent with iframe scrollbars and top-level document scrollbars also seems bad to me. Note also that you'd have to deal with vertical text scrollbars as well. Comment on attachment 87748 [details] A trial change View in context: https://bugs.webkit.org/attachment.cgi?id=87748&action=review Minusing because of the use of a CSS property (and for the other reasons mentioned in the previous comment). Not all platforms are going to want this change, so I think this is better off as a Setting if you're going to do it. > Source/WebCore/rendering/RenderBlock.cpp:1415 > + if (style()->isShowScrollbarOnLeft()) This is bad terminology and bad English. Created attachment 88796 [details] A trial change v2 Greetings, Thank you all for your review and comments. Even though I have updated my change, I realize this change still needs a lot of cases to discuss about. (In reply to comment #12) > I do not believe making this a CSS property is the right way to go. This should just be a global policy decision and not controllable by authors. > I also don't believe we want this change on Mac OS X, since scrollbars stay on the right hand side even with RTL content in list boxes for example. Thanks for your comment. (My last change added a CSS property just because I badly misunderstood a comment from Eric.) I have updated this change to use a setting. (If this feature is not needed by Mac, we need to move these layout tests to 'LayoutTests/platform' and use an ENABLE() macro?) > Being inconsistent with iframe scrollbars and top-level document scrollbars also seems bad to me. If I can recall comments from a native Hebrew speaker (Aharon), he suggested not to move the vertical scrollbars of <iframe> or <body> even when their direction is RTL. (He suggested to move them when the UI language is an RTL one.) I would like to check it again. By the way, I'm personally wondering if I shuuld include the code that moves the vertical scrollbars of RenderView objects to this change because this change already becomes >35KB. It would be definitely helpful to give me your opinions about it. > Note also that you'd have to deal with vertical text scrollbars as well. Yeah. Even though I notice vertical text, this change did not deal with it because I would like to ask Aharon about it with this change. (Maybe showing to the top-side?) (In reply to comment #13) > Minusing because of the use of a CSS property (and for the other reasons mentioned in the previous comment). > Not all platforms are going to want this change, so I think this is better off as a Setting if you're going to do it. Thank you. I have updated my change to use it. > > Source/WebCore/rendering/RenderBlock.cpp:1415 > > + if (style()->isShowScrollbarOnLeft()) > > This is bad terminology and bad English. Indeed. (This clearly shows my poor English skill.) Regards, Hironori Bono Attachment 88796 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8347989 (In reply to comment #12) > Being inconsistent with iframe scrollbars and top-level document scrollbars also seems bad to me. In frames and iframes hosting HTML documents, the dir attribute (and CSS direction) of the frame/iframe element has no visible effect, since it is completely overridden by the direction of the HTML document in the frame. Thus, the vertical scrollbar position should be determined by the frame's HTML document too. Thus, the exception for frames and iframes makes sense. The rule is simple: for RTL content, the vertical scrollbar should be on the left side. The direction of the content in a frame is given by the document, not the frame element. As for top-level documents, this is indeed an exception to the rule. The reason for the exception is that it is even more important to have the *window*'s vertical scrollbar in a fixed position as one surfs from one page to another of the opposite direction, since it saves the user having to check the direction of the page or visually find the scrollbar before starting to move the mouse to it. Please note that this rationale does not apply to internal elements, since their scrollbar is in an arbitrary position in the window anyway. > I also don't believe we want this change on Mac OS X, since scrollbars stay on the right hand side even with RTL content in list boxes for example. WebKit does try to emulate native control behavior, so I guess this comment makes sense. Although as far as I am concerned, Mac OS X behavior in this case is not optimal, and I would not be surprised if it will change some day. Created attachment 89186 [details] A trial change v3 Greetings, (In reply to comment #15) > Attachment 88796 [details] did not build on chromium: > Build output: http://queues.webkit.org/results/8347989 Oops, I forgot adding Chromium-specific files to this change. I have updated the change to include some Chromium-specific files. Regards, Hironori Bono Created attachment 90162 [details]
A trial change v4
Greetings,
I have updated this change just for fixing a conflict.
Regards,
Hironori Bono
Hi Bono-san, what about iframe? Is it a different story from this patch? > Hi Bono-san, what about iframe? Is it a different story from this patch?
Oops. I missed aharon's comment... I'm sorry for disturbing.
David, Simon, Dan, could you kindly review the patch? Thanks and appreciated! I am not sure in what version they made the change, but as of version 5, Firefox displays scrollbars on elements below <body> on the end side, i.e. on the left for an RTL element. The Firefox bugs for this are https://bugzilla.mozilla.org/show_bug.cgi?id=556363 and https://bugzilla.mozilla.org/show_bug.cgi?id=619963. Created attachment 107460 [details]
Patch v6 (enabled only on Chromium)
Greetings,
Even though my previous patch has not got any interest of reviewers, I have updated my change because my previous patch does not compile on the latest WebKit any longer. Also, I have changed the layout tests to a script test and added an ENABLED_OVERFLOW_MIRRORING flag so we can enable/disable this feature at compilation time. (It seems Safari does not need it.)
Regards,
Hironori Bono
This bug, which lacks a workaround, is causing problems at several Google products. Is anyone available to review Hirinori's patch? Comment on attachment 107460 [details] Patch v6 (enabled only on Chromium) View in context: https://bugs.webkit.org/attachment.cgi?id=107460&action=review informal review since I'm not a WebKit reviewer. > Source/WebCore/ChangeLog:6 > + This change adds a new flag ENABLE_OVERFLOW_MIRRORING and render nit: I think ENABLE_OVERFLOW_MIRRORING is a little hard to understand. How about ENABLE_RTL_SCROLLBARS ? > Source/WebCore/rendering/RenderBlock.cpp:1468 > + if (positionedObject->style()->position() != FixedPosition) { nit: I think rewriting this as: if (positionedObject->style()->position() == FixedPosition) continue; #ifdef ... Would be a little cleaner since you wouldn' t need to repeat the if inside the #ifdef. > Source/WebCore/rendering/RenderBox.cpp:1194 > +#else Can you remove the #else and reuse the same call to contract() ? This may make things easier to follow... > Source/WebCore/rendering/style/RenderStyle.h:529 > + bool isRightToLeftDirection() const { return direction() == RTL; } I may be wrong but I seem to recall Mitz mentioning that he wanted everything phrased as being related to isLTR so you may want to remove this and just use a locale isRTL = !bla->isLeftToRightDirection() where you need htat. Comment on attachment 107460 [details] Patch v6 (enabled only on Chromium) Attachment 107460 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9685143 New failing tests: fast/block/float/028.html fast/overflow/overflow-rtl-vertical.html fast/events/offsetX-offsetY.html fast/overflow/unreachable-overflow-rtl-bug.html fast/block/float/026.html Comment on attachment 107460 [details] Patch v6 (enabled only on Chromium) View in context: https://bugs.webkit.org/attachment.cgi?id=107460&action=review > Source/WebCore/rendering/RenderBlock.cpp:1467 > +#if ENABLE(OVERFLOW_MIRRORING) Adam Barth is "fixing" many of the mis using of "ENABLE". I believe you would be introducing yet another one, but I would like him to comment. Adam, ENABLE x USE here? > Source/WebCore/rendering/RenderLayer.cpp:1769 > + const RenderStyle* style = layer->renderer()->style(); I would drop the 'const' here. Adam, see comment #27. Comment on attachment 107460 [details]
Patch v6 (enabled only on Chromium)
I would like to see some hit testing tests here especially when overflow contents are scrolled by a few pixels. r- due to lack of hit testing tests.
Comment on attachment 107460 [details] Patch v6 (enabled only on Chromium) View in context: https://bugs.webkit.org/attachment.cgi?id=107460&action=review > Source/WebCore/rendering/RenderLayer.cpp:1772 > + return LayoutRect(x, bounds.maxY() - verticalThickness - style->borderBottomWidth(), > + horizontalThickness, verticalThickness); I'd note that indentation here is wrong. horizontalThickness should be on the right of "return LayoutRect" by exactly 4 spaces (8 spaces in the total). Ditto for the rest of the patch. Created attachment 114018 [details]
Patch v7
Greetings,
Sorry for the lack of my updates.
I have renamed ENABLE(OVERFLOW_MIRRORING) to USE(RTL_SCROLLBAR), re-implemented its layout test that hits a scrollbar track, and fixed a conflict.
Regards,
Hironori Bono
Comment on attachment 114018 [details] Patch v7 Attachment 114018 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10370064 New failing tests: fast/block/float/028.html fast/overflow/overflow-rtl-vertical.html fast/events/offsetX-offsetY.html fast/overflow/unreachable-overflow-rtl-bug.html fast/block/float/026.html Do you know why those tests are failing? Comment on attachment 114018 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=114018&action=review Nits. > Source/WebCore/ChangeLog:10 > + Reviewed by NOBODY (OOPS!). This line should appear before the long description. > Source/WebCore/rendering/RenderLayer.cpp:2147 > + absBounds.y() + box->borderTop(), > + m_vBar->width(), > + absBounds.height() - (box->borderTop() + box->borderBottom()) - scrollCorner.height())); Wrong indentation. "absBounds.y() + box->borderTop()" should be exactly 4 spaces to the right of "m_vBar->". > Source/WebCore/rendering/RenderLayer.cpp:2157 > + m_hBar->height())); Ditto. > Source/WebCore/rendering/RenderLayer.cpp:2572 > + box->borderTop(), > + m_vBar->width(), > + box->height() - (box->borderTop() + box->borderBottom()) - (m_hBar ? m_hBar->height() : resizeControlSize)); Ditto. > Source/WebCore/rendering/RenderLayer.cpp:2594 > + box->height() - box->borderBottom() - m_hBar->height(), > + box->width() - (box->borderLeft() + box->borderRight()) - (m_vBar ? m_vBar->width() : resizeControlSize), > + m_hBar->height()); Ditto. > LayoutTests/ChangeLog:10 > + Reviewed by NOBODY (OOPS!). This line should appear before the long description. > LayoutTests/platform/chromium/fast/events/rtl-scrollbar.html:1 > +<html> No DOCTYPE? > LayoutTests/platform/chromium/fast/events/rtl-scrollbar.html:16 > +// Save the vertical-scroll offset of the above <div> element before sending a > +// click event. If we successfully scroll down the element, this offset should > +// become greather than this value. I don't think we need this comment. > LayoutTests/platform/chromium/fast/events/rtl-scrollbar.html:24 > + eventSender.mouseMoveTo(10, 300); You can do mouseMoveTo(overflow.offsetLeft + 5, overflow.offsetTop + overflow.offsetHeight - 50); instead of putting div at the right location. > LayoutTests/platform/chromium/fast/events/rtl-scrollbar.html:29 > + // Wait until we finish rendering the element. > + setTimeout(finished, 1000); One second seems like an excess amount of time to wait. Probably setTimeout(finished, 0) is sufficient. Also, I don't this comment is useful because the code is quite self-evident. > LayoutTests/platform/chromium/fast/events/rtl-scrollbar.html:34 > + // Verify the vertical-scroll offset becomes greater than the saved one. The code is self-evident. Comment on attachment 114018 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=114018&action=review This patch feels like it begs for some sort of abstraction. It's unclear to me what that abstraction should look like, but a series of ifdefs seems wrong. Can you clarify for me (and in the ChangeLog) which platforms would want this behavior vs. not? Could we have a rtlAwareVerticalScrollbarWidth() which returns 0 when this is disabled, and thus we only would need the #ifdef in one place? > Source/WebCore/rendering/RenderBlock.cpp:1475 > +#if USE(RTL_SCROLLBAR) What platforms would want this disabled? Normally we have defaults for these sorts of things in Platform.h. Comment on attachment 114018 [details]
Patch v7
This seems like a lot of copy-paste code.
I'm willing to believe that this *is* the right solution, but I need you to convince me of that.
Why is this the right abstraction layer? Can we make this change at a lower level? Could we add helper functions like "adjustRectForRTLAwareScrollBar" which would contain this ifdef and do nothing when this ifdef is disabled?
I'm nto sure what the right way to write this change is. You are *much* more familiar with this code than I am, and I trust your judgement!
I do, however need you to convince me, and future readers of this patch, (in the ChangeLog, ideally) that this is the right way to go.
If you can come up with a solution which involves less copy-paste code, that would be ideal.
Thanks again for the patch!
A gentle reminder: this bug is causing problems in various Google apps. There is no workaround. Created attachment 121427 [details] Patch v8 (refactored) Greetings, Many thanks for your review and comments. Also sorry for my slow update. This feature s enabled only on Chromium just because Dave told Safari did not need it as written in his comment #12. (Even though I'm not sure if other ports need it, I would like to disable it because it may surprise them to change the behavior of scrollbars without noticing them.) As you pointed out, my previous change has lots of copy-and-paste code. (This is just because I could not find good function names.) I have created three new functions (rtlAwareCornerX, rtlAwareVerticalScrollbarX, and rtlAwareHorizontalScrollbarX) to simplify this change. (Unfortunately, we need three functions: the first one is for moving a resizer, the second one is for moving a vertical scrollbar, and the third one is for moving a horizontal scrollbar, respectively.) Moving a vertical scrollbar needs to move a horizontal scrollbar and a resizer. Regards, Hironori Bono (In reply to comment #36) > (From update of attachment 114018 [details]) > This seems like a lot of copy-paste code. > > I'm willing to believe that this *is* the right solution, but I need you to convince me of that. > > Why is this the right abstraction layer? Can we make this change at a lower level? Could we add helper functions like "adjustRectForRTLAwareScrollBar" which would contain this ifdef and do nothing when this ifdef is disabled? > > I'm nto sure what the right way to write this change is. You are *much* more familiar with this code than I am, and I trust your judgement! > > I do, however need you to convince me, and future readers of this patch, (in the ChangeLog, ideally) that this is the right way to go. > > If you can come up with a solution which involves less copy-paste code, that would be ideal. > > Thanks again for the patch! Comment on attachment 121427 [details] Patch v8 (refactored) Attachment 121427 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11145364 New failing tests: fast/block/float/028.html fast/overflow/overflow-rtl-vertical.html fast/events/offsetX-offsetY.html fast/block/float/026.html Comment on attachment 121427 [details] Patch v8 (refactored) Attachment 121427 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11142397 New failing tests: fast/block/float/028.html fast/overflow/overflow-rtl-vertical.html fast/events/offsetX-offsetY.html fast/block/float/026.html Comment on attachment 121427 [details] Patch v8 (refactored) View in context: https://bugs.webkit.org/attachment.cgi?id=121427&action=review Great! New patch looks much cleaner. > Source/WebCore/rendering/RenderLayer.cpp:1952 > +LayoutUnit RenderLayer::rtlAwareVerticalScrollbarX(int minX, int maxX) const We normally use the word "start" to mean left for LTR and right for RTL so how about verticalScrollbarStart? > Source/WebCore/rendering/RenderLayer.cpp:1962 > +LayoutUnit RenderLayer::rtlAwareHorizontalScrollbarX(int minX) const horizontalScrollbarStart? Created attachment 122368 [details] Patch v9 (renamed function names) Greetings Niwa-san, Many thanks for your comments. I have renamed the functions added by this change. Also, I have disabled moving scrollbars when -webkit-writing-mode is vertical-lr or vertical rl. (I notice we also need to change Y positions to support them.) Should this change support them as well, or should I use another change to support them? By the way, this change ALWAYS moves horizontal scrollbars of RTL elements on Chromium. That is, this change needs rebaselines for some layout tests that use RTL elements: "fast/block/float/026.html", "fast/block/float/028.html", "fast/events/offsetX-offsetY.html". (Unfortunately, the first two tests are pixel ones and I cannot get rebaselined results for all platforms.) Regards, Hironori Bono (In reply to comment #41) > (From update of attachment 121427 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121427&action=review > > Great! New patch looks much cleaner. > > > Source/WebCore/rendering/RenderLayer.cpp:1952 > > +LayoutUnit RenderLayer::rtlAwareVerticalScrollbarX(int minX, int maxX) const > > We normally use the word "start" to mean left for LTR and right for RTL so how about verticalScrollbarStart? > > > Source/WebCore/rendering/RenderLayer.cpp:1962 > > +LayoutUnit RenderLayer::rtlAwareHorizontalScrollbarX(int minX) const > > horizontalScrollbarStart? Comment on attachment 122368 [details] Patch v9 (renamed function names) Attachment 122368 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11152456 New failing tests: fast/block/float/028.html fast/events/offsetX-offsetY.html fast/block/float/026.html Comment on attachment 122368 [details] Patch v9 (renamed function names) Attachment 122368 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11152465 New failing tests: fast/block/float/028.html fast/events/offsetX-offsetY.html fast/block/float/026.html Comment on attachment 122368 [details] Patch v9 (renamed function names) View in context: https://bugs.webkit.org/attachment.cgi?id=122368&action=review Does your patch work for a block with writing-mode: bt-rl? > Source/WebCore/rendering/RenderBlock.cpp:1497 > + if (!style()->isLeftToRightDirection() && style()->isHorizontalWritingMode()) Can we move !style()->isLeftToRightDirection() to the outer if-statement so that we don't make an extra function call to addOverflowFromChild? > Source/WebCore/rendering/RenderBlock.cpp:1898 > + if (!style()->isLeftToRightDirection() && style()->isHorizontalWritingMode()) Instead of duplicating this condition throughout the codebase, can we add an inline function to wrap it? e.g. style()->shouldPlaceVerticalScrollbarOnLeft() > Source/WebCore/rendering/RenderBox.cpp:1232 > // Subtract out scrollbars if we have them. You should add an early exit when !layer() to avoid duplicating the if-statement. Greetings Niwa-san, Many thanks for your comments. Is it possible to clarify a couple of your comments before applying them? (In reply to comment #45) > Can we move !style()->isLeftToRightDirection() to the outer if-statement so that we don't make an extra function call to addOverflowFromChild? Does this mean we just call addOverflowFromChild(positionedObject) (not addOverflowFromChild(positionedObject, IntSize(...))) when !style()->isLeftToRightDirection() is true? (We need to call addOverflowFromChild itself for non-fixed objects as the original code calls.) > > Source/WebCore/rendering/RenderBlock.cpp:1898 > > + if (!style()->isLeftToRightDirection() && style()->isHorizontalWritingMode()) > > Instead of duplicating this condition throughout the codebase, can we add an inline function to wrap it? > e.g. style()->shouldPlaceVerticalScrollbarOnLeft() In my personal opinion, I wonder if we can call this function 'vertical'. When -webkit-writing-mode is vertical-rl or vertical-lr, this scrollbar is used for scrolling text "horizontally". For example, when we open "LayoutTests/fast/overflow/overflow-rtl-vertical.html" on Safari, it shows a scrollbar above its text. When we apply my patch v8, it moves the scrollbar below its text. Regards, Hironori Bono (In reply to comment #46) > (In reply to comment #45) > > Can we move !style()->isLeftToRightDirection() to the outer if-statement so that we don't make an extra function call to addOverflowFromChild? > > Does this mean we just call addOverflowFromChild(positionedObject) (not addOverflowFromChild(positionedObject, IntSize(...))) when !style()->isLeftToRightDirection() is true? (We need to call addOverflowFromChild itself for non-fixed objects as the original code calls.) Sorry, that comment seems bogus. What I really meant is move "if (positionedObject->style()->position() != FixedPosition)" outside of if-defs to be shared. Then, you only need to wrap two lines inside if-defs (x -= verticalScrollbarWidth(); and if before that). > > > Source/WebCore/rendering/RenderBlock.cpp:1898 > > > + if (!style()->isLeftToRightDirection() && style()->isHorizontalWritingMode()) > > > > Instead of duplicating this condition throughout the codebase, can we add an inline function to wrap it? > > e.g. style()->shouldPlaceVerticalScrollbarOnLeft() > > In my personal opinion, I wonder if we can call this function 'vertical'. When -webkit-writing-mode is vertical-rl or vertical-lr, this scrollbar is used for scrolling text "horizontally". For example, when we open "LayoutTests/fast/overflow/overflow-rtl-vertical.html" on Safari, it shows a scrollbar above its text. When we apply my patch v8, it moves the scrollbar below its text. Hm... how about shouldPlaceBlockDirectionScrollbarOnLogicalLeft() then ? Created attachment 125303 [details] Patch v10 (Applied comments) Greetings Niwa-san, Many thanks for your comments. (In reply to comment #47) > Sorry, that comment seems bogus. What I really meant is move "if (positionedObject->style()->position() != FixedPosition)" outside of if-defs to be shared. Then, you only need to wrap two lines inside if-defs (x -= verticalScrollbarWidth(); and if before that). Done. Thanks for your correction. > Hm... how about shouldPlaceBlockDirectionScrollbarOnLogicalLeft() then ? Done. I should have describe this issue when I noticed it. Regards, Hironori Bono Comment on attachment 125303 [details] Patch v10 (Applied comments) Attachment 125303 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11418295 New failing tests: fast/block/float/028.html fast/events/offsetX-offsetY.html fast/block/float/026.html Comment on attachment 125303 [details] Patch v10 (Applied comments) Attachment 125303 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11422247 New failing tests: fast/block/float/028.html fast/events/offsetX-offsetY.html fast/block/float/026.html Comment on attachment 125303 [details] Patch v10 (Applied comments) View in context: https://bugs.webkit.org/attachment.cgi?id=125303&action=review This code looks right to me. I think you could have simplified the #defines by moving them as noted below, but I also think this i OK. Thank you for hte patch. > Source/WebCore/rendering/RenderLayer.cpp:1831 > + return IntRect(cornerStart(layer, bounds.x(), bounds.maxX(), horizontalThickness), I'm not sure what you mean by "corner start" here? > Source/WebCore/rendering/style/RenderStyle.h:966 > + bool shouldPlaceBlockDirectionScrollbarOnLogicalLeft() const { return !isLeftToRightDirection() && isHorizontalWritingMode(); } It seems it would have been simpler to put the #if around this function, and just define a version of thsi function which only returns false when RTL_SCROLLBAR is disabled. Created attachment 127350 [details]
Patch v11 (Applied comments from Eric)
Greetings Eric,
Thanks for your review and comments.
I have applied your comments and uploaded the updated one.
Regards,
Hironori Bono
Comment on attachment 127350 [details] Patch v11 (Applied comments from Eric) Attachment 127350 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11534808 New failing tests: fast/block/float/028.html fast/events/offsetX-offsetY.html fast/block/float/026.html Comment on attachment 127350 [details] Patch v11 (Applied comments from Eric) Where is the resize box rendered after your patch? Ojan pointed out a good point in https://bugs.webkit.org/show_bug.cgi?id=9223 that resize handles and scrollbar should appear on the same side. Does your patch move the resize box as well? Greetings Niwa-san, Thanks for your feedbakc. Yes, cornerStart() moves it. (This RenderLayer class uses 'cornerRect' probably because not all input boxes have resizers.) Regards, Hironori Bono (In reply to comment #54) > (From update of attachment 127350 [details]) > Where is the resize box rendered after your patch? Ojan pointed out a good point in https://bugs.webkit.org/show_bug.cgi?id=9223 that resize handles and scrollbar should appear on the same side. > > Does your patch move the resize box as well? (In reply to comment #55) > Greetings Niwa-san, > > Thanks for your feedbakc. > Yes, cornerStart() moves it. (This RenderLayer class uses 'cornerRect' probably because not all input boxes have resizers.) Fantastic! Comment on attachment 127350 [details] Patch v11 (Applied comments from Eric) r=me trusting eseidel's previous review. Please add a test for resize handle (could be a followup patch on the bug 9223). Created attachment 129623 [details]
Patch v12 (Updated to ToT)
Greetings,
Somehow webkit-patch land-from-bug cannot land my previous change. I would like to upload the new one to see it works.
Regards,
Hironori Bono
Comment on attachment 129623 [details] Patch v12 (Updated to ToT) View in context: https://bugs.webkit.org/attachment.cgi?id=129623&action=review Again, please add a test for resize handle as a follow up. > Source/WebCore/ChangeLog:10 > + Reviewed by NOBODY (OOPS!). Reviewed by line should appear above the description but below the bug url. > Source/WebKit/chromium/ChangeLog:10 > + Reviewed by NOBODY (OOPS!). Ditto. > LayoutTests/ChangeLog:10 > + Reviewed by NOBODY (OOPS!). Ditto. Greetings Niwa-san, Thanks for your comment. (In reply to comment #59) > Again, please add a test for resize handle as a follow up. This change just moves a resizer to the left wide. In brief, I a little wonder if it is totally the expected behavior of Bug 9223. (Does it expect mirroring a resizer bitmap as well as moving it to the left side?) Regards, Hironori Bono Comment on attachment 129623 [details] Patch v12 (Updated to ToT) Attachment 129623 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11779380 New failing tests: fast/block/float/028.html fast/events/offsetX-offsetY.html fast/block/float/026.html Comment on attachment 129623 [details] Patch v12 (Updated to ToT) Clearing flags on attachment: 129623 Committed r109512: <http://trac.webkit.org/changeset/109512> All reviewed patches have been landed. Closing bug. Based on a discussion with morrita@ and hbono@, I marked the following tests as FAIL: fast/block/float/026.html fast/block/float/028.html fast/events/offsetX-offsetY.html fast/overflow/unreachable-overflow-rtl-bug.html http://trac.webkit.org/changeset/109537 Greetings Hara-san, Thanks a lot for noticing these failures. I will rebaseline them. (It may be better to rebaseline them after fixing Bug 9223? Fixing Bug 9223 needs to change a resizer image, i.e. it probably changes the output images of these tests.) Regards, Hironori Bono (In reply to comment #64) > Based on a discussion with morrita@ and hbono@, I marked the following tests as FAIL: > > fast/block/float/026.html > fast/block/float/028.html > fast/events/offsetX-offsetY.html > fast/overflow/unreachable-overflow-rtl-bug.html > > http://trac.webkit.org/changeset/109537 I think adamk has already rebaselined them. I am reopening because iframes are not being handled properly. Their scrollbar is currently always on the right, even if both the <iframe> and the document inside it have direction:rtl. For an <iframe>, the scrollbar should be on the start side of the document inside it (even though when displayed in a separate tab, not an <iframe>, the document's direction should not determine the scrollbar side). If the thing inside the <iframe> does not have a direction, e.g. it is an image, it should be on the start side of the <iframe>'s direction. Attaching a test case. Please note that this is the same requirement I originally stated in comment 16 (quoted below), although perhaps not clearly enough. (In reply to comment #16) > In frames and iframes hosting HTML documents, the dir attribute (and CSS direction) of the frame/iframe element has no visible effect, since it is completely overridden by the direction of the HTML document in the frame. Thus, the vertical scrollbar position should be determined by the frame's HTML document too. Thus, the exception for frames and iframes makes sense. > > The rule is simple: for RTL content, the vertical scrollbar should be on the left side. The direction of the content in a frame is given by the document, not the frame element. > > As for top-level documents, this is indeed an exception to the rule. The reason for the exception is that it is even more important to have the *window*'s vertical scrollbar in a fixed position as one surfs from one page to another of the opposite direction, since it saves the user having to check the direction of the page or visually find the scrollbar before starting to move the mouse to it. > > Please note that this rationale does not apply to internal elements, since their scrollbar is in an arbitrary position in the window anyway. Probably better to open a new bug than to re-open this one. This one has served us well, but trying to work here further is likely just to make reviews harder as the number of patches/comments is somewhat intimidating. :) You can of course reference this bug for "further information", or mark it as a dependent when you file the new one with your test case. Created attachment 137316 [details]
Test case. Extract the files in the zip and open iframe-scollbar-outer.html.
(In reply to comment #68) > Probably better to open a new bug than to re-open this one. This one has served us well, but trying to work here further is likely just to make reviews harder as the number of patches/comments is somewhat intimidating. :) > > You can of course reference this bug for "further information", or mark it as a dependent when you file the new one with your test case. Ok, opened bug 84026 for the <iframe> issue; marking this as resolved again. |