Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2512033002:
RTL overlay scrollbar on Mac is left aligned. (Closed)
DescriptionRTL overlay scrollbar on Mac is left aligned.
BUG=626919
Committed: https://crrev.com/94a2767135371369605eda754487ea11ff7213b2
Cr-Commit-Position: refs/heads/master@{#434239}
Patch Set 1 #Patch Set 2 : Layouttest draft deleted. #
Total comments: 2
Messages
Total messages: 22 (12 generated)
Description was changed from ========== RTL overlay scrollbar on Mac is left aligned. WIP draft BUG=626919 ========== to ========== RTL overlay scrollbar on Mac is left aligned. WIP draft BUG=626919 ==========
sahel@chromium.org changed reviewers: + bokan@chromium.org
Please review the WIP cl, the layout test is the issue.
On 2016/11/18 15:17:19, sahel wrote: > Please review the WIP cl, the layout test is the issue. Thanks, I don't have a Mac with me at the moment but I'll give it a try on Monday.
So the scrollbar will show up with this code:
<!DOCTYPE html>
<script>
window.jsTestIsAsync = true;
window.enablePixelTesting = true;
</script>
<script src="../../resources/js-test.js"></script>
<script>
if (window.testRunner) {
testRunner.waitUntilDone();
}
if (window.internals) {
internals.settings.setOverlayScrollbarsEnabled(true);
}
</script>
<style>
html, body { margin: 0; padding: 0; }
#green_div {
direction: rtl;
width: 400px;
height: 300px;
overflow-y: scroll;
border: 1px solid green;
}
.strut { height: 500px; }
</style>
<div id="green_div" style="transform: translateZ(0)">
<div class="strut">
Test<br>Test<br>3
</div>
</div>
<script>
window.onload = function() {
const green_div = document.getElementById('green_div');
if (window.internals) {
internals.setScrollbarVisibilityInScrollableArea(green_div, true);
setTimeout(function() {
if (window.testRunner)
testRunner.notifyDone();
}, 300);
}
}
</script>
The issue is that it takes a while for the scrollbar paint to show up and that
timeout will be very flaky. While looking around, I did find that
NSScrollerImpPair has a lockOverlayScrollbarState which seems like it'd be
useful
(https://github.com/pombredanne/osx_headers/blob/master/Frameworks/AppKit/NSSc...)
but there was no way I could get it to work. (Side note: I also found this
https://github.com/WebKit/webkit/commit/6d7a5db8448c70678aae4fd3c4d44cbfad47ef84
:)
So in summary, adding any kind of testing here will be flaky until we can figure
out the scrollbar situation on Mac for testing better. Adding a test would
result in more followup work than it'll save so I'm fine with no test. Please
just add a screenshot to the bug showing before and after.
bokan@chromium.org changed reviewers: + jbroman@chromium.org
Ok, this is lgtm from me. +jbroman@ for platform OWNER. See my comment above as to why no test.
https://codereview.chromium.org/2512033002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollbarThemeMac.mm (right): https://codereview.chromium.org/2512033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollbarThemeMac.mm:298: // of scrollbar width, to avoid the gap on the left side of the thumb. One question here: what happens if the Mac system language is set to an RTL language? It'd be nice to be sure that |drawKnob| actually draws on the right, as opposed to "right on LTR, left on RTL" (in the latter case, there would also be the reverse version of this bug, for LTR content on an RTL system).
https://codereview.chromium.org/2512033002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollbarThemeMac.mm (right): https://codereview.chromium.org/2512033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollbarThemeMac.mm:298: // of scrollbar width, to avoid the gap on the left side of the thumb. On 2016/11/22 23:21:54, jbroman wrote: > One question here: what happens if the Mac system language is set to an RTL > language? > > It'd be nice to be sure that |drawKnob| actually draws on the right, as opposed > to "right on LTR, left on RTL" (in the latter case, there would also be the > reverse version of this bug, for LTR content on an RTL system). I set the OS language to Farsi. The window scrollbars are still on the right. This is also true for main frame scrollbars on Safari. Scrollbar of a div with RTL direction doesn't go to the left either (regardless of the system language).
Thanks, lgtm.
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== RTL overlay scrollbar on Mac is left aligned. WIP draft BUG=626919 ========== to ========== RTL overlay scrollbar on Mac is left aligned. BUG=626919 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sahel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479931322898790,
"parent_rev": "0eb01699b040af3247a87dc5d6914647d47deca6", "commit_rev":
"d28fc88a6f0b911ba335ad62182e0c164a2128c1"}
Message was sent while issue was closed.
Description was changed from ========== RTL overlay scrollbar on Mac is left aligned. BUG=626919 ========== to ========== RTL overlay scrollbar on Mac is left aligned. BUG=626919 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== RTL overlay scrollbar on Mac is left aligned. BUG=626919 ========== to ========== RTL overlay scrollbar on Mac is left aligned. BUG=626919 Committed: https://crrev.com/94a2767135371369605eda754487ea11ff7213b2 Cr-Commit-Position: refs/heads/master@{#434239} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/94a2767135371369605eda754487ea11ff7213b2 Cr-Commit-Position: refs/heads/master@{#434239} |
