[Webkit-unassigned] [Bug 50928] RTL: Select elements with a size attribute are always left aligned

[bugzilla-daemon at webkit.org]

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


Dimitri Glazkov (Google) <dglazkov at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78801|review?                     |review-
               Flag|                            |




--- Comment #3 from Dimitri Glazkov (Google) <dglazkov at chromium.org>  2011-01-13 08:47:04 PST ---
(From update of attachment 78801)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.