Bug 50928

Summary:

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

Product:

WebKit

Reporter:

Jeremy Moskovich <playmobil>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aharon, commit-queue, dglazkov, eric, mitz, ofri, rniwa, xji

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

OS X 10.5

Bug Depends on:

56839

Bug Blocks:

Attachments:

Description	Flags
Testcase	none
Chrome vs FF	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

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.