50928 – RTL: Select elements with a size attribute are always left aligned

RESOLVED FIXED Bug 50928

RTL: Select elements with a size attribute are always left aligned

https://bugs.webkit.org/show_bug.cgi?id=50928

Summary RTL: Select elements with a size attribute are always left aligned

Jeremy Moskovich

Reported 2010-12-13 09:35:26 PST

Created attachment 76394 [details] Testcase Chromium bug: crbug.com/66709 1. Open attached test case. What is the expected output? Contents of select element should be right aligned. Actual: Contents of select element are always left aligned. The items in a select elements whose size attribute is set should be right aligned if dir=rtl is set on the select element, instead they are left aligned. Browsers tested: Safari nightly r73886: FAIL Chrome 9.0.597.16 : FAIL Firefox 3.6.12: OK Opera 10.61.8429: OK

Attachments
Testcase (325 bytes, text/html) 2010-12-13 09:35 PST, Jeremy Moskovich	no flags	Details
Chrome vs FF (25.17 KB, image/jpeg) 2010-12-13 09:38 PST, Jeremy Moskovich	no flags	Details
Patch (14.64 KB, patch) 2011-01-13 05:54 PST, Ofri Wolfus	no flags	Details Formatted Diff Diff
Patch (114.06 KB, patch) 2011-02-10 06:48 PST, Ofri Wolfus	no flags	Details Formatted Diff Diff
Patch (114.42 KB, patch) 2011-03-09 04:56 PST, Ofri Wolfus	no flags	Details Formatted Diff Diff
Patch (114.40 KB, patch) 2011-03-15 08:25 PDT, Ofri Wolfus	no flags	Details Formatted Diff Diff
Patch (114.37 KB, patch) 2011-03-22 01:11 PDT, Jeremy Moskovich	no flags	Details Formatted Diff Diff
Patch (211.65 KB, patch) 2011-03-28 04:47 PDT, Ofri Wolfus	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Jeremy Moskovich

Comment 1 2010-12-13 09:38:47 PST

Created attachment 76396 [details] Chrome vs FF

Ofri Wolfus

Comment 2 2011-01-13 05:54:29 PST

Created attachment 78801 [details] Patch

Dimitri Glazkov (Google)

Comment 3 2011-01-13 08:47:04 PST

Comment on attachment 78801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78801&action=review Thanks for the patch, Ofri! I took a first pass at the quality. You need to reach out to mitz or hyatt to check the meat. > Source/WebCore/ChangeLog:8 > + Added support for alignment in RenderListBox. I am surprised there's no "Test: fast/forms/listbox-bidi-align.html" in here. Did you prepare this ChangeLog before writing the test? > Source/WebCore/ChangeLog:11 > + * rendering/RenderListBox.cpp: > + (WebCore::RenderListBox::paintItemForeground): Please use these to provide description of changes in each method and file. It both helps reviewers and archeologists to understand mechanics of the change. > Source/WebCore/rendering/RenderListBox.cpp:355 > + ETextAlign actualAlignment = element->renderStyle() ? element->renderStyle()->textAlign() : style()->textAlign(); > + // FIXME: Firefox doesn't respect JUSTIFY. Should we? > + if (actualAlignment == TAAUTO || actualAlignment == JUSTIFY) { > + if (isLTR) > + actualAlignment = LEFT; > + else > + actualAlignment = RIGHT; > + } > + > + // Determine where the item text should be placed > + IntRect r = itemBoundingBoxRect(tx, ty, listIndex); > + if (actualAlignment == RIGHT || actualAlignment == WEBKIT_RIGHT) { > + float textWidth = itemFont.floatWidth(textRun); > + r.move(r.width() - textWidth - optionsSpacingHorizontal, itemFont.ascent()); > + r.setWidth(textWidth); > + } else if (actualAlignment == CENTER) { > + float textWidth = itemFont.floatWidth(textRun); > + r.move((r.width() - textWidth) / 2, itemFont.ascent()); > + r.setWidth(textWidth); > + } else { > + r.move(optionsSpacingHorizontal, itemFont.ascent()); > + } This is something the RTL folks should check.

Ryosuke Niwa

Comment 4 2011-01-13 09:09:11 PST

Comment on attachment 78801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78801&action=review > LayoutTests/fast/forms/listbox-bidi-align.html:1 > +<!doctype html> Nit: <!DOCTYPE html> > LayoutTests/platform/mac/fast/forms/listbox-bidi-align-expected.txt:6 > +layer at (0,0) size 800x600 > + RenderView at (0,0) size 800x600 > +layer at (0,0) size 800x419 > + RenderBlock {HTML} at (0,0) size 800x419 > + RenderBody {BODY} at (8,8) size 784x403 > + RenderBlock (anonymous) at (0,0) size 784x36 This test should also have .png image, which can be generated by passing using --pixel option to run-webkit-tests

Xiaomei Ji

Comment 5 2011-01-13 12:32:39 PST

Comment on attachment 78801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78801&action=review > Source/WebCore/rendering/RenderListBox.cpp:306 > + Thanks for the patch! Seems that renderStyle() returns the <select>'s style even if the style is set in <option>. So, alignment style in <option> is ignored and <option> always aligned according to <select>'s alignment (or implied from direction). Is this expected behavior? and is it defined in standard? > Source/WebCore/rendering/RenderListBox.cpp:334 ETextAlign actualAlignment = itemStyle->textAlign() > Source/WebCore/rendering/RenderListBox.cpp:335 > + // FIXME: Firefox doesn't respect JUSTIFY. Should we? I think we should. then, remove this line? > Source/WebCore/rendering/RenderListBox.cpp:349 > + r.setWidth(textWidth); > + } else if (actualAlignment == CENTER) { else if (actualAlignment == CENTER || actualAlignment == WEBKIT_CENTER) > Source/WebCore/rendering/RenderListBox.cpp:352 > + r.setWidth(textWidth); why need setWidth here (and above)? >> Source/WebCore/rendering/RenderListBox.cpp:355 > > This is something the RTL folks should check. Seems <select>/<option> never truncate, so above logic is correct. This code is not shared by <input> or other element that truncation could happen, right?

Ofri Wolfus

Comment 6 2011-02-10 06:48:13 PST

Created attachment 81972 [details] Patch

Ofri Wolfus

Comment 7 2011-02-10 06:48:59 PST

Ready for another look. Thanks!

Ofri Wolfus

Comment 8 2011-02-10 06:56:02 PST

Thanks for the useful comments! Regarding the justify issue, unless I'm mistaking it'll be a lot of work to support it. The drawing code used doesn't appear to have any support for this feature. Ofri (In reply to comment #5) > (From update of attachment 78801 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78801&action=review > > > Source/WebCore/rendering/RenderListBox.cpp:306 > > + > > Thanks for the patch! > Seems that renderStyle() returns the <select>'s style even if the style is set in <option>. So, alignment style in <option> is ignored and <option> always aligned according to <select>'s alignment (or implied from direction). > Is this expected behavior? and is it defined in standard? > > > Source/WebCore/rendering/RenderListBox.cpp:334 > > > ETextAlign actualAlignment = itemStyle->textAlign() > > > Source/WebCore/rendering/RenderListBox.cpp:335 > > + // FIXME: Firefox doesn't respect JUSTIFY. Should we? > > I think we should. then, remove this line? > > > Source/WebCore/rendering/RenderListBox.cpp:349 > > + r.setWidth(textWidth); > > + } else if (actualAlignment == CENTER) { > > else if (actualAlignment == CENTER || actualAlignment == WEBKIT_CENTER) > > > Source/WebCore/rendering/RenderListBox.cpp:352 > > + r.setWidth(textWidth); > > why need setWidth here (and above)? > > >> Source/WebCore/rendering/RenderListBox.cpp:355 > > > > > This is something the RTL folks should check. > > Seems <select>/<option> never truncate, so above logic is correct. > This code is not shared by <input> or other element that truncation could happen, right?

Xiaomei Ji

Comment 9 2011-02-10 12:18:40 PST

(In reply to comment #6) > Created an attachment (id=81972) [details] > Patch Should the elements' rendering here be consistent with elements' rendering in button text and drop-down when there is no size attribute? If so, should we differentiate using selectItemWritingDirectionIsNatural() and selectItemAlignmentFollowsMenuWritingDirection() as in RenderMenuList::adjustInnerStyle() although the behavior of selectItemAlignmentFollowsMenuWritingDirection() is adopted in HTML5 standard? And in the case of selectItemAlignmentFollowsMenuWritingDirection(), item's alignment is based on <select>'s writing direction (not <select>'s alignment, neither <option>'s alignment). So, there wont be mixed-alignment. There is discussion about alignment issue in chromium's bug: http://code.google.com/p/chromium/issues/detail?id=72000 If we need another alignment behavior, we could define selectItemAlignmentFollowsOptionAlignment(). But it might look weird to support some alignments without supporting the others.

Jeremy Moskovich

Comment 10 2011-02-13 02:51:01 PST

(In reply to comment #9) > Should the elements' rendering here be consistent with elements' rendering in button text and drop-down when there is no size attribute? I don't think that needs to be part of this bug. Select elements without a size attribute use native controls and are thus subject to various limitations of the OS controls. This change makes us match FF/IE for select multiple which seems like a good thing. The only case we don't match is IE which doesn't support text-align:center or per-option alignment.

Aharon (Vladimir) Lanin

Comment 11 2011-02-15 18:49:43 PST

A couple of thoughts: 1. when the select with size has dir=rtl, its scrollbar should be on the left. It is currently on the right. This is completely analogous to the drop-down button when there is no size, which on a select with dir=rtl comes out on the left. I know that Firefox doesn't get this right either, but I don't care - it's a bug. IE8 gets it right. 2. If you are going to make text-align work for options in a select with size, I think it is critical that you put the following in your default stylesheet: select {text-align:start} option {text-align:match-parent} The first will prevent options in a select with size whose ancestor has text-align:center come out centered by default. The second will make options with different dir in a select with size come out aligned to different sides.

Aharon (Vladimir) Lanin

Comment 12 2011-02-16 09:32:54 PST

(In reply to comment #11) > select {text-align:start} I should mention that this is what Gecko does. > option {text-align:match-parent} > [...] > [This] will make options with different dir in a select with size come out aligned to different sides. Typo: I meant to say "will prevent options with different dir in a select with size coming out aligned to different sides." BTW: 1. I also suggested this to Gecko. 2. Of course, for this to work, text-align:match-parent has to get implemented. I consider match-parent to be a very important addition, and it should be really easy to implement.

Jeremy Moskovich

Comment 13 2011-02-17 01:06:42 PST

Thanks for your comments, you raise some valuable points but I think they're orthogonal, can we please review this patch as is and track separate issues in their own bugs: * Left-side scrollbars - issue 54623 * Implementing text-align:match-parent: issue 50951 * Changing default stylesheet for select/option - could you file a separate bug for this?

Xiaomei Ji

Comment 14 2011-02-23 10:11:59 PST

The patch itself looks good to me, but I am not a reviewer.

Eric Seidel (no email)

Comment 15 2011-02-28 10:34:35 PST

Comment on attachment 81972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81972&action=review Honestly this all looks fine. I'm sorry I didn't review it sooner. Please try pulling out some of your new code into a helper function and re-post. Ping me right away if you don't see an immediate review and I'll be happy to look again. > Source/WebCore/rendering/RenderListBox.cpp:337 > + ETextAlign actualAlignment = itemStyle->textAlign(); > + // FIXME: Firefox doesn't respect JUSTIFY. Should we? > + if (actualAlignment == TAAUTO || actualAlignment == JUSTIFY) { > + if (isLTR) > + actualAlignment = LEFT; > + else > + actualAlignment = RIGHT; > + } Since this is part of the later if-cascade, I would just include it in the helper function as well. > Source/WebCore/rendering/RenderListBox.cpp:341 > + // Determine where the item text should be placed > + IntRect r = itemBoundingBoxRect(tx, ty, listIndex); > + if (actualAlignment == RIGHT || actualAlignment == WEBKIT_RIGHT) { I would have made this whole if-cascade a helper function with a nice name which returned an IntSize (since move() takes an intsize). Something like this: r.move(itemOffsetForAlignment(itemStyle, itemFont, textRun)) Depending on how you wanted to break it up, you could even push teh whole bounds collection into a helper function. Since it doesn't look like it needs any member variables, it could just be a static inline. > Source/WebCore/rendering/RenderListBox.cpp:349 > + } else { > + r.move(optionsSpacingHorizontal, itemFont.fontMetrics().ascent()); > + } Style.

Ofri Wolfus

Comment 16 2011-03-09 04:56:29 PST

Created attachment 85162 [details] Patch

Eric Seidel (no email)

Comment 17 2011-03-09 10:24:24 PST

Comment on attachment 85162 [details] Patch Seems reasonable to me.

WebKit Commit Bot

Comment 18 2011-03-10 16:34:09 PST

Comment on attachment 85162 [details] Patch Rejecting attachment 85162 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'ap..." exit_code: 2 Last 500 characters of output: file Source/WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebCore/rendering/RenderListBox.cpp Hunk #1 succeeded at 316 (offset 28 lines). Hunk #2 succeeded at 342 (offset 28 lines). Hunk #3 succeeded at 367 (offset 28 lines). Hunk #4 FAILED at 381. 1 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/rendering/RenderListBox.cpp.rej Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/8133090

Ofri Wolfus

Comment 19 2011-03-15 08:25:36 PDT

Created attachment 85808 [details] Patch

Jeremy Moskovich

Comment 20 2011-03-22 01:11:51 PDT

Created attachment 86438 [details] Patch

Jeremy Moskovich

Comment 21 2011-03-22 01:12:42 PDT

Comment on attachment 86438 [details] Patch Already r+'d by Eric, marking for commit.

WebKit Commit Bot

Comment 22 2011-03-22 03:14:50 PDT

Comment on attachment 86438 [details] Patch Clearing flags on attachment: 86438 Committed r81653: <http://trac.webkit.org/changeset/81653>

WebKit Commit Bot

Comment 23 2011-03-22 03:14:57 PDT

All reviewed patches have been landed. Closing bug.

Eric Seidel (no email)

Comment 24 2011-03-22 10:11:39 PDT

Was rolled out.

Ofri Wolfus

Comment 25 2011-03-28 04:47:17 PDT

Created attachment 87124 [details] Patch

Eric Seidel (no email)

Comment 26 2011-03-28 04:49:18 PDT

Comment on attachment 87124 [details] Patch Looks good! Thanks.

WebKit Commit Bot

Comment 27 2011-03-28 23:42:48 PDT

Comment on attachment 87124 [details] Patch Clearing flags on attachment: 87124 Committed r82200: <http://trac.webkit.org/changeset/82200>

WebKit Commit Bot

Comment 28 2011-03-28 23:42:57 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS OS X 10.5

Product WebKit

Component Layout and Rendering

Assignee

Nobody

Reported

2010-12-13 09:35 PST

Modified

2011-03-28 23:42 PDT History

CC List

8 users Show

URL

Keywords

Depends on

56839

Blocks

Dependancies

tree graph