Bug 50969 - <option> should support the dir attribute and be displayed accordingly both in the dropdown and after being chosen
Summary: <option> should support the dir attribute and be displayed accordingly both i...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://dev.w3.org/html5/spec/Overview...
Keywords:
Depends on: 19785
Blocks: 50910
  Show dependency treegraph
 
Reported: 2010-12-13 14:03 PST by Xiaomei Ji
Modified: 2011-12-30 11:07 PST (History)
12 users (show)

See Also:


Attachments
patch for Chromium port (8.17 KB, patch)
2011-02-03 10:00 PST, Xiaomei Ji
no flags Details | Formatted Diff | Diff
Patch for Chromium on Mac (7.31 KB, patch)
2011-02-07 13:10 PST, Avi Drissman
fishd: review-
fishd: commit-queue-
Details | Formatted Diff | Diff
Patch for Chromium on Mac, v2 (7.34 KB, patch)
2011-02-07 14:20 PST, Avi Drissman
no flags Details | Formatted Diff | Diff
Forgot to initialize variables (1.11 KB, patch)
2011-03-10 18:06 PST, Avi Drissman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2010-12-13 14:03:43 PST
Following is from the spec:

10.6.4 Text rendered in native user interfaces

User agents are expected to honor the Unicode semantics of text that is exposed in user interfaces, for example supporting the bidirectional algorithm in text shown in dialogs, title bars, pop-up menus, and tooltips. Text from elements (either attribute values or the contents of elements) is expected to be rendered in a manner that honors the directionality of the element from which the text was obtained.

Consider the following markup, which has Hebrew text asking for a programming language, the languages being text for which a left-to-right direction is important given the punctuation in some of their names:

<p dir="rtl" lang="he">
 <label>
  בחר שפת תכנות:
  <select>
   <option dir="ltr">C++</option>
   <option dir="ltr">C#</option>
   <option dir="ltr">FreePascal</option>
   <option dir="ltr">F#</option>
  </select>
 </label>
</p>
If the select element was rendered as a drop down box, a correct rendering would ensure that the punctuation was the same both in the drop down, and in the box showing the current selection.

Note: with image showing the item showing in drop-down and selection are both in LTR directionality and right alignment.
Comment 1 Levi Weintraub 2011-01-18 10:30:56 PST
Safari 5.0.3 handles this correctly, but Chromium with TOT WebKit is still broken. Haven't nailed down why precisely yet.
Comment 2 Jeremy Moskovich 2011-01-18 10:34:59 PST
Levi: perhaps what you're seeing is Cocoa aligning Hebrew text to the right and English text to the left?  What about the following case:

  <select>
   <option dir="rtl">Right aligned</option>
   <option dir="ltr">left aligned</option>
   <option dir="ltr">מיושר לשמאל</option>
   <option dir="rtl">מיושר לימין</option>
  </select
Comment 3 Levi Weintraub 2011-01-18 10:37:42 PST
That'd be it, thanks! Glancing at the interface I didn't see how that was getting communicated... :)
Comment 4 Xiaomei Ji 2011-01-28 13:50:08 PST
Refer to http://trac.webkit.org/changeset/76983 for detail.
Other ports need to 
1. define Chrome::selectItemAlignmentFollowsMenuWritingDirection() as true.
2. implement rendering of <option> in dropdown using <option>'s dir as what mitz did in webkit2.
Comment 5 Xiaomei Ji 2011-02-03 10:00:52 PST
Created attachment 81072 [details]
patch for Chromium port
Comment 6 David Levin 2011-02-04 11:29:25 PST
Comment on attachment 81072 [details]
patch for Chromium port

View in context: https://bugs.webkit.org/attachment.cgi?id=81072&action=review

I think you can address my comments above and commit this. Thanks!

> Source/WebCore/ChangeLog:5
> +        Implement "<option> should support the dir attribute and be displayed accordingly both in the drop-down and after being chosen" for chromium port after r76983.

This is pretty verbose.

"<option> should implement the dir attribute."

sums it up. (Just exposing the attribute isn't implementing it. The code needs to display the text correctly, etc.)

> Source/WebCore/ChangeLog:12
> +        Turn on it for <select>.

This is really long.

I think you can leave it out entirely. You explained it well in the file/function level comments.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:84
> +    false // restrictWidthOfListBox

For future reference, misc formatting fixes mixed in a fix on lines where you aren't doing changes is frowned on because it obscures the change being done, but ok.

> Source/WebKit/chromium/ChangeLog:13
> +

Same comments as before.
Comment 7 Xiaomei Ji 2011-02-04 12:31:48 PST
Committed r77654: <http://trac.webkit.org/changeset/77654> for chromium port.
Comment 8 David Levin 2011-02-04 14:24:01 PST
Please resolve bugs after landing them :)
Comment 9 Levi Weintraub 2011-02-04 14:25:22 PST
(In reply to comment #8)
> Please resolve bugs after landing them :)

I think it's still open because it isn't fixed for all ports.
Comment 10 Xiaomei Ji 2011-02-04 14:55:51 PST
Comment on attachment 81072 [details]
patch for Chromium port

clear flags for landed patch.
Comment 11 Xiaomei Ji 2011-02-04 14:56:21 PST
keep it open for other ports.
Comment 12 Avi Drissman 2011-02-07 13:10:09 PST
Created attachment 81512 [details]
Patch for Chromium on Mac
Comment 13 David Levin 2011-02-07 13:20:56 PST
Adding Darin due to modification in Source/WebKit/chromium/public/WebMenuItemInfo.h.
Comment 14 Darin Fisher (:fishd, Google) 2011-02-07 13:36:36 PST
Comment on attachment 81512 [details]
Patch for Chromium on Mac

View in context: https://bugs.webkit.org/attachment.cgi?id=81512&action=review

> Source/WebKit/chromium/public/WebMenuItemInfo.h:61
> +    bool directionalOverride;

it is not clear what directionalOverride means.  would it perhaps be
better named hasTextDirectionOverride as that is what WebCore seems
to be using for this field already?

if i'm the embedder, can i'm trying to render a menu based on this
information, what am i supposed to do with "directionalOverride" and
"textDirection"?  I think the other fields are fairly self-explanatory,
but these fields are not similarly obvious.
Comment 15 Avi Drissman 2011-02-07 14:20:28 PST
Created attachment 81517 [details]
Patch for Chromium on Mac, v2

Re directionalOverride, agreed; changed.

Re textDirection. This is of enum type, which IMO makes it clear. platform/PopupMenuStyle.h has textDirection() returning the same enum. "Direction" always means LTR/RTL when it comes to text. See editing/WritingDirection.h, editing/EditingStyle.h's textDirection(), WK/chromium/public/WebTextDirection.h, etc.
Comment 16 Xiaomei Ji 2011-02-08 16:16:24 PST
A summary of the changes in case I made Csaba and Martin confused in IRC.

1. This bug is about in what directionality <select>/<option> should be rendered. It is standardized in HTML5. 
Basically, the items should be displayed according to its directionality defined in <option> in both button text (when chosen) and in drop-down menu.

As to how <select>/<option> should be aligned, it seems not defined in standard. Webkit currently alignes item in drop-down box and in button text (when chosen) according to <select>'s directionality. It disregards text-align attribute in <select>. Please refer to mitz's comment in  https://bugs.webkit.org/show_bug.cgi?id=19785#c19.

2. Mitz implemented rendering button text according to its directionality defined in <option> and aligning button text based on <select>'s directionality in r76983.
The implementation is exposed through ChromeClient::selectItemAlignmentFollowsMenuWritingDirection().

In the same changset, Mitz also implemented rending the items in drop-down box using its directionality defined in <option> and aligning items in drop-down box using <select>'s directionality for Mac OS X Snow leopard.

3. Other ports need to do the following to achieve the same behavior:
3.1 override selectItemAlignmentFollowsMenuWritingDirection() as true  and override selectItemWritingDirectionIsNatural() as false so that button text is rendered correctly on both directionality and alignment.
3.2  implement rendering of items in drop-down box using <option>'s directionality and aligning by <select>'s writing direction as what mitz did in webkit2 or platform/mac/PopupMenuMac.mm.
Chromium already implemented aligning items in drop-down box using <select>'s writing direction in WebCore/platform/chromium/PopupMenuChromium.cpp a while back. It also has implementation ready to render items in drop-down using item's directionality, and it just needs to turn on the switch. So, the chromium's patch for Windows and Linux committed here basically do not have any implementation of 3.2.

4. I added fast/text/international/pop-up-button-text-alignment-and-direction, which *only* test the rendering of button text.
It's skipped in gtk and qt for now. ossy and mrobinson will take care of qt and gtk ports.
To check the correctness, the button text should be rendered the same as the text below it per directionality and alignment as specified in the 1st line in the test.

5. We should add automatic test for drop-down box as well.
Comment 17 Avi Drissman 2011-02-09 08:09:30 PST
(In reply to comment #16)
> 3.2  implement rendering of items in drop-down box using <option>'s directionality and aligning by <select>'s writing direction as what mitz did in webkit2 or platform/mac/PopupMenuMac.mm.

The patch waiting for review here implements half of this for Chromium Mac. I have the other half (the Chromium side) ready for commit (http://codereview.chromium.org/6410125/). When landed, this will give Chromium Mac the exact same behavior as what mitz did for PopupMenuMac.mm.

(Ping; please re-review my patch attached here.)
Comment 18 Darin Fisher (:fishd, Google) 2011-02-09 13:29:17 PST
(In reply to comment #15)
> Created an attachment (id=81517) [details]
> Patch for Chromium on Mac, v2
> 
> Re directionalOverride, agreed; changed.
> 
> Re textDirection. This is of enum type, which IMO makes it clear. platform/PopupMenuStyle.h has textDirection() returning the same enum. "Direction" always means LTR/RTL when it comes to text. See editing/WritingDirection.h, editing/EditingStyle.h's textDirection(), WK/chromium/public/WebTextDirection.h, etc.

Can you use WebTextDirectionDefault in place of hasTextDirectionOverride?  

Unlike WebCore::TextDirection, WebTextDirection has the "unspecified" state.  It seems like hasTextDirectionOverride is therefore unnecessary / redundant.
Comment 19 Avi Drissman 2011-02-09 13:32:20 PST
(In reply to comment #18)
> Can you use WebTextDirectionDefault in place of hasTextDirectionOverride?  
> 
> Unlike WebCore::TextDirection, WebTextDirection has the "unspecified" state.  It seems like hasTextDirectionOverride is therefore unnecessary / redundant.

No. An override is orthogonal to the directionality. Please look at http://trac.webkit.org/browser/trunk/Source/WebCore/manual-tests/pop-up-alignment-and-direction.html , particularly options #3 and #4 in the popup. The fact there is an override is a separate fact from LTR/RTL.
Comment 20 WebKit Commit Bot 2011-02-10 04:07:28 PST
Comment on attachment 81517 [details]
Patch for Chromium on Mac, v2

Clearing flags on attachment: 81517

Committed r78193: <http://trac.webkit.org/changeset/78193>
Comment 21 Avi Drissman 2011-02-11 09:45:53 PST
(In reply to comment #16)
> 3. Other ports need to do the following to achieve the same behavior:
> 3.1 override selectItemAlignmentFollowsMenuWritingDirection() as true [...]
> 3.2  implement rendering of items in drop-down box [...]

I just landed r74622 in Chromium which takes advantage of the plumbed data to properly render items in the drop-down box. Chromium Mac is done.

Leaving this bug open; there are some more platforms to fix.
Comment 22 Stuart Morgan 2011-03-10 15:33:28 PST
This added two new POD members, but didn't add initializations to the constructor. Please fix that, since it causes clients to introduce uninitialized memory in normal use.
Comment 23 Stuart Morgan 2011-03-10 15:34:29 PST
(Added to WebMenuItemInfo, sorry)
Comment 24 Avi Drissman 2011-03-10 18:06:02 PST
Created attachment 85416 [details]
Forgot to initialize variables
Comment 25 WebKit Commit Bot 2011-03-11 10:48:09 PST
Comment on attachment 85416 [details]
Forgot to initialize variables

Clearing flags on attachment: 85416

Committed r80862: <http://trac.webkit.org/changeset/80862>
Comment 26 Aharon (Vladimir) Lanin 2011-12-29 22:16:09 PST
I have verified that it is fixed on Windows and Linux. Yay!
Comment 27 Ryosuke Niwa 2011-12-30 11:03:52 PST
Was this bug specific to Chromium port or no?
Comment 28 Avi Drissman 2011-12-30 11:07:36 PST
(In reply to comment #27)
> Was this bug specific to Chromium port or no?

According to comments 9 and 11, no, this is an issue that all ports have to handle.