Summary: | REGRESSION(r88757): input[type=range] renders incorrectly in RTL context | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rachel Blum <groby> | ||||||||
Component: | Layout and Rendering | Assignee: | Kent Tamura <tkent> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dglazkov, mitz, playmobil, progame+wk, rniwa, tkent, webkit.review.bot, xji | ||||||||
Priority: | P1 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Rachel Blum
2011-07-20 16:50:09 PDT
Created attachment 101527 [details]
repro case
Regressions are P1. groby: Can you bisect to find where this broke? i'll have one in a few minutes yes, i got as far as WebKit 88697:88789. (and r88757 seems likely) It breaks RTL DOMUI-Options Fonts pane in Chromium ( http://crbug.com/87690 ) Created attachment 101562 [details]
Patch
Comment on attachment 101562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101562&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:243 > + if (isVertical || !renderBox()->style()->isLeftToRightDirection()) Would this work if isVertical was true and isLeftToRightDirection() was false? Comment on attachment 101562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101562&action=review >> Source/WebCore/html/shadow/SliderThumbElement.cpp:243 >> + if (isVertical || !renderBox()->style()->isLeftToRightDirection()) > > Would this work if isVertical was true and isLeftToRightDirection() was false? It works. If isVertical is true, isLeftToRightDirection() doesn't matter. We have no LTR/RTL differences in vertical sliders. Comment on attachment 101562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101562&action=review ok, looks sane to me. >>> Source/WebCore/html/shadow/SliderThumbElement.cpp:243 >>> + if (isVertical || !renderBox()->style()->isLeftToRightDirection()) >> >> Would this work if isVertical was true and isLeftToRightDirection() was false? > > It works. > If isVertical is true, isLeftToRightDirection() doesn't matter. We have no LTR/RTL differences in vertical sliders. Should we at least add a test case for it? Created attachment 101564 [details]
Patch for landing
Add a vertical case
Comment on attachment 101564 [details] Patch for landing Rejecting attachment 101564 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2 Last 500 characters of output: Unexpected flakiness: text diff mismatch (1) fast/forms/implicit-submission.html = TEXT PASS Regressions: Unexpected image mismatch : (6) fast/forms/input-appearance-range-rtl.html = IMAGE fast/text/atsui-multiple-renderers.html = IMAGE fast/text/international/danda-space.html = IMAGE fast/text/international/thai-baht-space.html = IMAGE fast/text/international/thai-line-breaks.html = IMAGE platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE Full output: http://queues.webkit.org/results/9191618 (In reply to comment #12) > Regressions: Unexpected image mismatch : (6) > fast/forms/input-appearance-range-rtl.html = IMAGE I have no idea what's wrong on Chromium-linux. Anyway, it must be a test problem, not a C++ code problem. I'll add the test to test_expectations.txt, land the patch, and see results on Chromium buildbots. Committed r91460: <http://trac.webkit.org/changeset/91460> (In reply to comment #13) > (In reply to comment #12) > > Regressions: Unexpected image mismatch : (6) > > fast/forms/input-appearance-range-rtl.html = IMAGE > > I have no idea what's wrong on Chromium-linux. Anyway, it must be a test problem, not a C++ code problem. I'll add the test to test_expectations.txt, land the patch, and see results on Chromium buildbots. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showLargeExpectations=true&showExpectations=true&tests=range-rtl It was due to a Chromium bug: http://code.google.com/p/chromium/issues/detail?id=89616 |