Bug 80640 - RTL textarea resize handle looks bad on OS X
: RTL textarea resize handle looks bad on OS X
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-08 14:10 PST by Jeremy Moskovich
Modified: 2012-05-08 10:50 PDT (History)
5 users (show)

See Also:


Attachments
Screenshot (22.15 KB, image/jpeg)
2012-03-08 14:11 PST, Jeremy Moskovich
no flags Details
testcase (505 bytes, text/html)
2012-03-08 14:12 PST, Jeremy Moskovich
no flags Details
Patch v1 (9.67 KB, patch)
2012-04-22 22:53 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Moskovich 2012-03-08 14:10:13 PST
The fix for issue 9223 moved the resize handle to the left side for RTL textareas, in addition the resize handle image is mirrored.

On OS X the resize handle image for RTL texteareas overlaps the textarea border (see attached screenshot), this looks funny.

It needs to be inset by a pixel or 2 to match the LTR case.
Comment 1 Jeremy Moskovich 2012-03-08 14:11:57 PST
Created attachment 130901 [details]
Screenshot
Comment 2 Jeremy Moskovich 2012-03-08 14:12:29 PST
Created attachment 130902 [details]
testcase
Comment 3 Hironori Bono 2012-03-08 20:23:58 PST
Greetings Jeremy,

Thanks for your bug report.
Somehow, this issue happens only on Mac Chrome and Linux Chrome, i.e. it does not happen on Windows Chrome. (When I defined WTF_USE_RTL_SCROLLBAR on Safari, this issue also happens on Mac Safari.) I will investigate more deeply to find why my r110073 causes such platform differences.  

Regards,

Hironori Bono
Comment 4 Jeremy Moskovich 2012-04-15 23:02:57 PDT
Hi Bono-san, did you get a chance to look into this?
Comment 5 Hironori Bono 2012-04-15 23:17:00 PDT
Greetings Jeremy,

Unfortunately, not so much. As written in my previous comment, this issue does not happen on Windows and I assume this issue is caused by the platform-specific code of WebKit. (I would note my r110073 does not have any platform-specific code.) But, I do not have so much knowledge about it and I have a problem  understanding it.

Regards,

Hironori Bono

(In reply to comment #4)
> Hi Bono-san, did you get a chance to look into this?
Comment 6 Hironori Bono 2012-04-22 21:27:13 PDT
Greetings Jeremy,

I have looked at this issue today and noticed it was caused by a bonehead mistake of my r110073 <http://trac.webkit.org/changeset/110073>. (Its translation formula is wrong and it works only on some special situations.) I'm writing a fix for this issue now.

Regards,

Hironori Bono
Comment 7 Hironori Bono 2012-04-22 22:53:25 PDT
Created attachment 138293 [details]
Patch v1

Greetings,

I have quickly written a fix and its layout test.
Unfortunately, this test is a pixel test and I'm not sure if it works on all platforms. (This test finishes without errors when I tested it on my Win7 PC, Mac, and a Linux PC, respectively.) I would like to set r? to run it on EWS bots and see its results.

Regards,

Hironori Bono
Comment 8 Hironori Bono 2012-04-23 19:11:12 PDT
Greetings Jeremy,

It seems my layout test works well on EWS bots. Would it be possible to check if the attached image is your expected result?

Regards,

Hironori Bono
Comment 9 Jeremy Moskovich 2012-04-24 00:21:54 PDT
rtl-resizer-position-expected.png lgtm, thanks Bono-san!
Comment 10 Hironori Bono 2012-04-25 01:11:31 PDT
Greetings Tony,

Would it be possible to review this quick fix?

Regards,

Hironori Bono

(In reply to comment #9)
> rtl-resizer-position-expected.png lgtm, thanks Bono-san!
Comment 11 Tony Chang 2012-04-27 10:46:47 PDT
Comment on attachment 138293 [details]
Patch v1

Why is the layout test for Chromium only? Is it because this feature is currently only enabled on Chromium?  It might be better to move tests for left side scrollbars and resizers into a subdir and skip that dir on platforms that don't enable this feature.  That could be done in a follow up patch.
Comment 12 Hironori Bono 2012-05-07 21:04:11 PDT
Greetings Tony,

Thanks for your review and comment.
Yes, I made this test Chromium-only one because this feature was enabled only on Chromium. Since I have landed another Chromium-only test (r110073) to "LayoutTests/platform/chromium", I would like to create another change that moves all of them to "LayoutTests/scrollbars/rtl". Is it OK for you?

Regards,

Hironori Bono

(In reply to comment #11)
> (From update of attachment 138293 [details])
> Why is the layout test for Chromium only? Is it because this feature is currently only enabled on Chromium?  It might be better to move tests for left side scrollbars and resizers into a subdir and skip that dir on platforms that don't enable this feature.  That could be done in a follow up patch.
Comment 13 Tony Chang 2012-05-08 10:02:17 PDT
(In reply to comment #12)
> Greetings Tony,
> 
> Thanks for your review and comment.
> Yes, I made this test Chromium-only one because this feature was enabled only on Chromium. Since I have landed another Chromium-only test (r110073) to "LayoutTests/platform/chromium", I would like to create another change that moves all of them to "LayoutTests/scrollbars/rtl". Is it OK for you?

A separate change to move the tests is fine with me.

Keeping the tests out of platform/chromium makes it easier for other ports that want to enable the feature.
Comment 14 Tony Chang 2012-05-08 10:04:31 PDT
Also, I wonder how hard it would be to make this a ref test. You would probably have to include a copy of the reversed gripper with the test.
Comment 15 WebKit Review Bot 2012-05-08 10:50:47 PDT
Comment on attachment 138293 [details]
Patch v1

Clearing flags on attachment: 138293

Committed r116435: <http://trac.webkit.org/changeset/116435>
Comment 16 WebKit Review Bot 2012-05-08 10:50:53 PDT
All reviewed patches have been landed.  Closing bug.