Patch for Bidi Support

13 views
Skip to first unread message

Rajeev Dayal

unread,
Feb 26, 2008, 7:06:12 PM2/26/08
to Joel Webber, Google Web Toolkit Contributors, Kelly Norton, Shanjian Li, Adil Allawi
Hello Joel,

I'd like you to do a code review. To apply this patch, navigate to the trunk, and perform the following steps:

svn update -r1910
patch -p0 < bidilayout-r1910.patch


Description:

This patch adds support for RTL layout in GWT. Here are the specific changes:

-adds the dir="rtl" attribute to the <html> element on the first call to RootPanel.get(id) or RootPanel.get(), provided that the application's current locale is one with an RTL language
-Removes default horizontal alignment of "left" from HorizontalPanel, DockPanel, and VerticalPanel - the horizontal alignment is left blank, and is chosen by the browser based on the document's layout
-adds some documentation to DockPanel to clarify the meaning of adding a widget to the WEST and EAST docks in RTL layout (WEST refers to the right side of the page, and EAST refers to the left side of the page)
-changes the layout of the Tree based on the directionality of the document - branches expand to the left when the layout is RTL. Also changed keyboard navigation - in an RTL layout, the left key expands branches, and the right key collapses them
-changes the layout of Menus so that submenus pop out to the left in an RTL layout
-adds support for HorizontalSplitPanel in an RTL layout. The splitter functions the same way, except that setLeftWidget sets the widget in the right pane, and setRightWidget sets the widget in the left pane. Also, the positioning of widgets with HorizontalSplitPanel.add() is right pane first, and then left pane. setSplitPosition still sets the size of the left pane in RTL mode.
-TextArea, TextBox, and Label implement the HasDirection interface. This interface allows those widgets that implement it to set their own directionality, thereby overriding the global direction.
-KitchenSink's tabs have been modified to 'look pretty' in RTL mode. This required the addition of another rounded corner image for use in RTL mode. While making this change, I also cleaned up (hopefully) some of the style switching code for first vs. other tabs, selected vs. unselected tabs. The existing code did not take advantage of dependent style names, so I updated the code to do so.
-User.gwt.xml now includes the I18N module. This change was necessary in order to support the use of LocaleInfo in the user libraries without having to move the LocaleInfo class into the user.client package
-Removed LocaleInfo_none_Test. This test was used to test that LocaleInfo functioned correctly when the I18N module was not included. Due to the above-mentioned change, this test is no longer relevant

Notes:

-To test any of the RTL behavior out, you need to be in a locale that has an RTL language. For my testing, I exposed the Arabic locale in the KitchenSink application:

  <extend-property name="locale" values="ar" /

-KitchenSink has not been completely rtl-ified. You'll notice some issues with punctuation being misplaced, and some minor layout issues. As per our previous discussion, we may go with KitchenSink++ in favor of the current KitchenSink, so it is not worth it to make KitchenSink totally RTL-friendly at this time.
-Right now, there is no mechanism to verify that those widgets that implement HasDirection do not also implement HasWidgets. Do you feel that it is sufficient to handle this with doc rather than via the type system?
-When the HorizontalSplitPanel is used in LTR layout and the window is resized, the right pane is the one that shrinks to accommodate the new window size. This is still the case when the layout is RTL. Should we be shrinking the left pane instead (@Shanjian)?
-@Kelly: Can you take a look at the code changes for HorizontalSplitPanel? It would definitely be nice to have an extra pair of eyes on that stuff.
-When using Safari and Opera, you'll notice that horizontal scrollbars will not appear when you move the slider and shrink the size of one of the panes in the HorizontalSplitPanel. These are general RTL bugs in Safari and Opera, and as such, I decided against putting in a specific fix for them in the HorizontalSplitPanel code.
-John and I looked at the compiled output to ensure that those blocks of code in if (LocaleInfo.getCurrentLocale().isRTL()) would be stripped if the clause evaluated to false. While the static evaluation is performed and the return value of LocaleInfo.getCurrentLocale().isRTL() is inlined, the blocks are not eliminated. I have filed a bug for this issue: http://code.google.com/p/google-web-toolkit/issues/detail?id=2123
-I'd like to get the a11y patch committed before this one. That way, on the second iteration of this patch, I can add RTL keyboard support for menus (keyboard support for menus was added in the a11y patch)
-This patch does not include Shanjian's bidiutils patch. That patch provides utilities for analyzing strings and determining if the string should be displayed left-to-right, or right-to-left. I envision those utilities being used in conjunction with the HasDirection implementation on Label, TextBox, and TextArea. I will be working on that patch and sending it in for review in the near future.


Patch By: adil.allawi, shanjian, rdayal, jgw, knorton, jat, ecc


Affected Files:

M      samples/kitchensink/src/com/google/gwt/sample/kitchensink/client/SinkList.java
M      samples/kitchensink/src/com/google/gwt/sample/kitchensink/public/KitchenSink.css
A      samples/kitchensink/src/com/google/gwt/sample/kitchensink/public/images/corner-rtl.gif
D      user/test/com/google/gwt/i18n/I18NTest_none.gwt.xml
D      user/test/com/google/gwt/i18n/client/LocaleInfo_none_Test.java
A      user/src/com/google/gwt/i18n/client/HasDirection.java
M      user/src/com/google/gwt/user/User.gwt.xml
M      user/src/com/google/gwt/user/client/ui/RootPanel.java
M      user/src/com/google/gwt/user/client/ui/TextBox.java
M      user/src/com/google/gwt/user/client/ui/TextArea.java
M      user/src/com/google/gwt/user/client/ui/HorizontalSplitPanel.java
M      user/src/com/google/gwt/user/client/ui/VerticalPanel.java
M      user/src/com/google/gwt/user/client/ui/HorizontalPanel.java
M      user/src/com/google/gwt/user/client/ui/DockPanel.java
M      user/src/com/google/gwt/user/client/ui/HasHorizontalAlignment.java
M      user/src/com/google/gwt/user/client/ui/TreeItem.java
M      user/src/com/google/gwt/user/client/ui/SuggestBox.java
M      user/src/com/google/gwt/user/client/ui/Tree.java
M      user/src/com/google/gwt/user/client/ui/MenuBar.java
M      user/src/com/google/gwt/user/client/ui/Label.java


Thanks,
Rajeev
bidilayout-r1910.patch

Adil Allawi

unread,
Feb 27, 2008, 3:41:26 PM2/27/08
to Rajeev Dayal, Joel Webber, Google Web Toolkit Contributors, Kelly Norton, Shanjian Li
Hello Rajeev

Its great to see the bidi work in your patch! I will give my considered comments later but here are some initial impressions:

1/ I really like the way the layout is tied to the locale. Being able to add "?locale=ar" to the URL is an ideal way to set the default direction. Is this also working for other RTL locales?

2/ Your patch file did not include corner-rtl.gif - I enclose it

3/ The implementation calls LocaleInfo.getCurrentLocale().isRTL()  to get the default RTL direction. Apart from Shanjian's comment, is this a quick way to get the RTL value? As I can imagine an application may need to get the RTL many times.

I can see from the code that the value is computed from CLDR but I cannot see how it is accessed as I do not know how this part of GWT is built.

4/ Not wanting to bring up any thorny issues but, what is the requirement to support Firefox 3 in GWT 1.5? I noticed several layout issues related to both Bidi and non-bidi cases. If these are new bugs in FF3 we should report them to Mozilla soon as FF3 is currently in beta.

5/ regarding the RTL HorizontalSplitPanel - I think it would be better to resize the left panel when you resize the window.

best regards

Adil

Content-Type: text/x-patch; name=bidilayout-r1910.patch
X-Attachment-Id: f_fd52xrls0
Content-Disposition: attachment; filename=bidilayout-r1910.patch

Attachment converted: adil:bidilayout-r1910.patch (TEXT/ttxt) (00651F80)

%corner-rtl.gif
corner-rtl.gif

John Tamplin

unread,
Feb 28, 2008, 10:31:30 AM2/28/08
to Google-Web-Tool...@googlegroups.com, Rajeev Dayal, Joel Webber, Kelly Norton, Shanjian Li
On Wed, Feb 27, 2008 at 3:41 PM, Adil Allawi <adil....@gmail.com> wrote:
3/ The implementation calls LocaleInfo.getCurrentLocale().isRTL()  to get the default RTL direction. Apart from Shanjian's comment, is this a quick way to get the RTL value? As I can imagine an application may need to get the RTL many times.

The generated code becomes a constant true or false.  Due to the way it is formulated to allow future multi-locale apps (ie, LocaleInfo.getLocale(String localeName)), the compiler currently calls LocaleInfo.<clinit> and dereferences a field to make sure it shouldn't throw a NPE.  In this case, the compiler should be able to know the reference can never be null and do away with the dereference, and I believe that can also do away with the clinit call.  Also, as Rajeev mentioned the compiler currently fails to realize that if ((clinit*(), instance0.impl, false)) can be optimized away.

Ultimately, all of those issues should be addressable in the compiler so the net result of the statement you give will be a literal true or false.  I am not sure how much of that optimizaiton will make it into 1.5, and if it does if it will make the milestone or be added by the RC.  Still, the compiler will remove duplicate clinit calls in a line of code and the extra overhead is just a few bytes and possibly a call of an empty function.  The bigger impact is not dead-stripping code from if(...isRTL()), but I think that is easier to fix and should definitely be fixed in 1.5.

I can see from the code that the value is computed from CLDR but I cannot see how it is accessed as I do not know how this part of GWT is built.

LocaleInfo wraps LocaleInfoImpl, which is generated to include the current locale name and the set of available locales, as well as CldrImpl, which is a Localizable interface that is currently hand-written from the CLDR data but will eventually include essentially all of the data available in CLDR.

--
John A. Tamplin
Software Engineer, Google

Rajeev Dayal

unread,
Feb 29, 2008, 10:56:59 AM2/29/08
to Shanjian Li, Joel Webber, Google Web Toolkit Contributors, Kelly Norton, Adil Allawi
Hey Shanjian,

Thanks for your comments. Replies inline:

On Wed, Feb 27, 2008 at 2:37 PM, Shanjian Li <shan...@google.com> wrote:
rajeev,

For one thing, we discussed about adding following API to DOM before,
void setRTLDirection(Element element, bool isRTL);
bool isRTLDirection(Element element);


Then following code :
+    DOM.setElementProperty(getElement(),
+        HasDirection.DIR_PROPERTY_NAME,
+        isRTL ? HasDirection.DIR_PROPERTY_VALUE_RTL : HasDirection.DIR_PROPERTY_VALUE_RTL);

can be replaced by:
    DOM.setRTLDirection(getElement(), isRTL);

This will greatly improve readability for RTL program, since the above 2 function will be called very often.

I do agree with decreasing the repetitiveness of the code. However, I think that something like this should live in another utility class. Typically, we do not include methods to set specific attributes in DOM-level classes. Perhaps I can introduce a BidiUtil class? I know you've introduced such a class already in an upcoming patch - maybe it could be called BidiDOMUtil instead.

Another thing we talked about before is margin in DOM, if we can have something like:
public static void setLeadingMargin(Element elem, string value) {
  if (isRTLDirection(elem)) {
    setElementAttribute(elem, "marginLeft", value);
  } else {
    setElementAttribute(elem, "marginRight", value);
  }

For same reason, such method is frequently used and thus could a be candidate for addition.

I would say that it would be better to let such attributes be controlled via CSS. I know that right now we do not support using differing stylesheets based on locale, but FooBundle (which will most likely be a part of GWT 1.6) will have this functionality. The problem with adding methods such as this into the DOM class is that the list can go on forever (setLeadingPadding, etc..). That is why the DOM class does not have any methods for setting style-specific attributes; we rely on CSS to do this.




shanjian

Rajeev Dayal

unread,
Feb 29, 2008, 11:15:58 AM2/29/08
to Adil Allawi, Joel Webber, Google Web Toolkit Contributors, Kelly Norton, Shanjian Li
Hey Adil,

Thanks for taking a look at this. Replies inline:

On Wed, Feb 27, 2008 at 3:41 PM, Adil Allawi <adil....@gmail.com> wrote:
Hello Rajeev

Its great to see the bidi work in your patch! I will give my considered comments later but here are some initial impressions:

1/ I really like the way the layout is tied to the locale. Being able to add "?locale=ar" to the URL is an ideal way to set the default direction. Is this also working for other RTL locales?

Yes, it is. The other RTL locales are: fa, he, ps, ur. By the way, all of this work was done as part of John's LocaleInfo patch. I just made use of the work that he did; he did all of the heavy lifting for me :).

2/ Your patch file did not include corner-rtl.gif - I enclose it

Whoops! Thanks for catching that.


4/ Not wanting to bring up any thorny issues but, what is the requirement to support Firefox 3 in GWT 1.5? I noticed several layout issues related to both Bidi and non-bidi cases. If these are new bugs in FF3 we should report them to Mozilla soon as FF3 is currently in beta.

I think that this question is really two separate questions. One question has to do with GWT's support for FF3 in general (i.e. non-bidi cases), and this question should be asked in a separate thread. In brief, I think that our aim to make sure that GWT apps actually run on FF3 (that is, no exceptions are thrown/errors are cause that would prevent you from using features of GWT). It is not our aim for the GWT 1.5 release to fix FF3 layout issues, unless they significantly impact the general usage case for a widget.

As for the bidi-related FF3 layout issues that you are seeing, can you detail what these might be? I think we would want to apply the rule mentioned above, which is that we would want to fix significant layout problems that occur in general usage cases, but not worry too much about corner cases at this time.


5/ regarding the RTL HorizontalSplitPanel - I think it would be better to resize the left panel when you resize the window.

That makes sense. I'll get to work on this.


best regards

Adil


 

At 7:06 -0500 26/2/08, Rajeev Dayal wrote:
Content-Type: text/x-patch; name=bidilayout-r1910.patch
X-Attachment-Id: f_fd52xrls0
Content-Disposition: attachment; filename=bidilayout-r1910.patch

John Tamplin

unread,
Feb 29, 2008, 1:46:32 PM2/29/08
to Google-Web-Tool...@googlegroups.com, Joel Webber, Kelly Norton, Shanjian Li, Adil Allawi
On Tue, Feb 26, 2008 at 7:06 PM, Rajeev Dayal <rda...@google.com> wrote:
-User.gwt.xml now includes the I18N module. This change was necessary in order to support the use of LocaleInfo in the user libraries without having to move the LocaleInfo class into the user.client package
-Removed LocaleInfo_none_Test. This test was used to test that LocaleInfo functioned correctly when the I18N module was not included. Due to the above-mentioned change, this test is no longer relevant

These changes and the use of LocaleInfo LGTM.

HasDirection needs javadoc on the interface, and the copyright message should be before the package line (didn't checkstyle complain?).

Rajeev Dayal

unread,
Feb 29, 2008, 4:58:11 PM2/29/08
to Google-Web-Tool...@googlegroups.com, Joel Webber, Kelly Norton, Shanjian Li, Adil Allawi
Hey John,

Thanks for the heads up - I noticed how horribly formatted this was the other day. I have no idea how I did not notice the Checkstyle errors. It must have been a late night when I wrote this interface. I will send out the properly-formatted version in the next patch.


Rajeev

Adil Allawi

unread,
Mar 3, 2008, 6:51:42 PM3/3/08
to Rajeev Dayal, Google Web Toolkit Contributors, Shanjian Li
Here are some more comments:

1/ in several places in the patch you have:

+  public void setRTLDirection(boolean isRTL) {
+    DOM.setElementProperty(getElement(),
+        HasDirection.DIR_PROPERTY_NAME,
+        isRTL ? HasDirection.DIR_PROPERTY_VALUE_RTL : HasDirection.DIR_PROPERTY_VALUE_RTL);

Shouldn't that be:

+  public void setRTLDirection(boolean isRTL) {
+    DOM.setElementProperty(getElement(),
+        HasDirection.DIR_PROPERTY_NAME,
+        isRTL ? HasDirection.DIR_PROPERTY_VALUE_RTL : HasDirection.DIR_PROPERTY_VALUE_LTR);

2/ Shanjian has suggested DOM.setRTLDirection() (or BidiDOMUtil ...) it is then worth also having: DOM.getRTLDirection() which returns one of 3 values: rtl, ltr or 'not set'. Or instead of 'not set' it could return LocaleInfo.getCurrentLocale().isRTL().

This would help with the clarity of code.

3/ DockLayout's rtl-EAST-means-left -  I am sure it was done for excellent reasons, but, something inside of me says this is wrong. I know it is documented, but the public will not take us seriously if we tell them that, for RTL, they have to swap the meaning of EAST and WEST. The class at least needs to have direction independent equivalents like AWT's BorderLayout.

Otherwise - to use a common phrase - LGTM!

best

Adil

At 7:06 -0500 26/2/08, Rajeev Dayal wrote:
Content-Type: text/x-patch; name=bidilayout-r1910.patch
X-Attachment-Id: f_fd52xrls0
Content-Disposition: attachment; filename=bidilayout-r1910.patch

Adil Allawi

unread,
Mar 7, 2008, 6:03:53 AM3/7/08
to Rajeev Dayal, Joel Webber, Google Web Toolkit Contributors, Kelly Norton, Shanjian Li
I did some testing on Firefox 3 with RTL layout  - the issues I previously noticed were fixed in one of the recent nightly builds but I did notice a general problem.

If you open KitchenSink (with locale = ar) in Firefox and look at the Lists tab you will notice that the tree widget items and the text box label are left aligned. GWT Panels are rendered in HTML as tables. In the Lists tab the content is made up of a vertical panel that is explicitly left aligned which embeds a horizontal panel that embeds the widgets so:

Vertical Panel (align = left) -> Horizontal Panel (default align) -> Widgets

In Firefox, the left alignment of the vertical panel is affecting the default alignment of the horizontal panel only if the layout is rtl. This is also a general issue. I made a simple test case and tried it in IE, FF and Safari (see http://www.diwan.com/w/rtl-table.html).

Each browser handles the inheritance of alignment into embedded tables in different and inconsistent ways. Even in tables that are not rtl.

With the bidi changes we have defined a new alignment for GWT called ALIGN_DEFAULT. Because of the inconsistency in browsers we need to be stricter about defining exactly what "default" alignment means (especially for rtl applications) and then apply this consistently in panels on each browser.

I suggest that ALIGN_DEFAULT should mean "right" if the locale is rtl and "left" otherwise. This should also be compatible with the way alignment is handled in GWT 1.4.

So in CellPanel.setCellHorizontalAlignment and also HTMLTable we need to do something like this:

  protected void setCellHorizontalAlignment(Element td,
      HorizontalAlignmentConstant align) {
    if (align == HasHorizontalAlignment.ALIGN_DEFAULT)
    {
      if (LocaleInfo.getCurrentLocale().isRTL())
        DOM.setElementProperty(td, "align", HasHorizontalAlignment.ALIGN_RIGHT.getTextAlignString());
      else
        DOM.setElementProperty(td, "align", HasHorizontalAlignment.ALIGN_LEFT.getTextAlignString());
    }
    else
      DOM.setElementProperty(td, "align", align.getTextAlignString());
  }

It is depressing to see an elegant implementation of alignment have to be changed because each browser cannot agree how alignment is to be inherited.

regards

Adil

Rajeev Dayal

unread,
Mar 13, 2008, 12:35:16 AM3/13/08
to Adil Allawi, Joel Webber, Google Web Toolkit Contributors, Kelly Norton, Shanjian Li
Hey all,

Sorry for the hiatus on the reply to this thread. I was busy with other GWT-1.5 related tasks.

Thanks so much for all of the feedback. I really appreciate it. I've put together a new patch for review. Here are the differences from the old patch:

-HasHorizontalAlignment.ALIGN_DEFAULT is assigned the value of HasHorizontalAlignment.ALIGN_RIGHT in an RTL environment, and HasHorizontalAlignment.ALIGN_LEFT in an LTR environment. This solves the FF3 (and FF2, in some cases) alignment issue that Adil mentioned

-DockPanel defines two new direction constants - LINE_START and LINE_END. They are bidi-friendly constants. In an RTL environment, LINE_START corresponds to the right side of the DockPanel. In an LTR environment, LINE_START corresponds to the left side of the DockPanel. WEST and EAST are now ALWAYS west and east - they do not vary based on directionality. WEST is always on the left, and EAST is always on the right.

-HorizontalSplitPanel defines some new getter/setter methods: setStartOfLineWidget/getStartOfLineWidget/setEndOfLineWidget/getEndOfLineWidget. I created them for a similar purpose as the LINE_START and LINE_END constants (thanks for pushing me on this Adil). setLeftWidget and setRightWidget will ALWAYS set the left/right widget, regardless of directionality.

-Some notes about HorizontalSplitPanel:
  - setSplitPosition always sets the position with respect to the left frame - so , this method is always sizing the left frame, regardless of directionality. Should we make a bidi-friendly equivalent?
   -When resizing the screen, the right panel is always the one that resizes. I am going to take Adil's suggestion to heart and make the left panel resize in an RTL environment in a separate patch

-Changed HasDirection.isRTLDirection to return a HasDirection.Direction enum. Values are RTL, LTR, or UNSET.

-Addition of BidiUtils.java (thanks Shanjian and Adil). This utility class defines two static methods setDirectionOnElement(elem, isRTL) and isDirectionRTLOnElement(elem). All of the current implementations of HasDirection basically delegate to these two helper methods. Adil, thanks for that good catch in the setRTLDirection implementation!

-I moved the DIR_PROPERTY_*** constants from HasDirection over to BidiUtils

-Added bidi keyboard support for menus

-Added an RTL-version of the submenu icon. In the future, this bundle containing both images should probably be replaced by a locale-sensitive FooBundle

-General note: Again, KitchenSink is not totally bidi-ified. If you switch to the 'ar' locale, you will get a pretty good feel for the bidi support that has been added. I think it would probably be better to concentrate on bidi-ifying John L's showcase app (and perhaps we'll run across more use-cases for widget bidi - for example, what about DisclosurePanel? I think we need a bidi-friendly arrow icon to represent the closed state. Right now, the arrow points to the right; it should probably point to the left in an RTL environment, right?)


@Shanjian: I did not add the setLeadingMargin/setTrailingMargin methods to BidiUtils. I'm still wary of doing this, as I feel that this type of thing should be done via CSS. I'm definitely open to more debate on the matter.

@Joel: I've included a zip file containing the image files that are part of this patch. Just unzip the file in the trunk directory, and they should show up in the right places.


Thanks,
Rajeev

On Fri, Mar 7, 2008 at 7:03 AM, Adil Allawi <adil....@gmail.com> wrote:
I did some testing on Firefox 3 with RTL layout  - the issues I previously noticed were fixed in one of the recent nightly builds but I did notice a general problem.

If you open KitchenSink (with locale = ar) in Firefox and look at the Lists tab you will notice that the tree widget items and the text box label are left aligned. GWT Panels are rendered in HTML as tables. In the Lists tab the content is made up of a vertical panel that is explicitly left aligned which embeds a horizontal panel that embeds the widgets so:

Vertical Panel (align = left) -> Horizontal Panel (default align) -> Widgets

In Firefox, the left alignment of the vertical panel is affecting the default alignment of the horizontal panel only if the layout is rtl. This is also a general issue. I made a simple test case and tried it in IE, FF and Safari (see http://www.diwan.com/w/rtl-table.html).

Each browser handles the inheritance of alignment into embedded tables in different and inconsistent ways. Even in tables that are not rtl.

With the bidi changes we have defined a new alignment for GWT called ALIGN_DEFAULT. Because of the inconsistency in browsers we need to be stricter about defining exactly what "default" alignment means (especially for rtl applications) and then apply this consistently in panels on each browser.

I suggest that ALIGN_DEFAULT should mean "right" if the locale is rtl and "left" otherwise. This should also be compatible with the way alignment is handled in GWT 1.4.

So in CellPanel.setCellHorizontalAlignment and also HTMLTable we need to do something like this:

  protected void setCellHorizontalAlignment(Element td,
      HorizontalAlignmentConstant align) {
    if (align == HasHorizontalAlignment.ALIGN_DEFAULT)
    {
      if (LocaleInfo.getCurrentLocale().isRTL())
        DOM.setElementProperty(td, "align", HasHorizontalAlignment.ALIGN_RIGHT.getTextAlignString());
      else
        DOM.setElementProperty(td, "align", HasHorizontalAlignment.ALIGN_LEFT.getTextAlignString());
    }
    else
      DOM.setElementProperty(td, "align", align.getTextAlignString());
  }

It is depressing to see an elegant implementation of alignment have to be changed because each browser cannot agree how alignment is to be inherited.

regards

Adil

At 11:15 -0500 29/2/08, Rajeev Dayal wrote:
bidilayout_updated-r2078.patch
rtlimages.zip

Adil Allawi

unread,
Mar 13, 2008, 4:30:55 PM3/13/08
to Rajeev Dayal, Joel Webber, Google Web Toolkit Contributors, Kelly Norton, Shanjian Li
Hi Rajeev,

Great work - and thanks for taking my comments to heart!

- your zip has an incorrect image it should have menuBarSubMenuIcon_rtl.gif but has menuBarSubMenuIcon.gif instead.

- I think setSplitPosition does not need a bidi equivalent as the graphics environment has its zero offset on the left regardless so programmers should be used to this.

- DisclosurePanel definitely needs a left-pointing arrow for rtl.

- setLeadingMargin/setTrailingMargin was kind-of my idea and I spent some time persuading Shanjian to accept it! When I decided to implement this I saw some widgets were setting their indent programmatically (e.g. the Tree widget). If it is the case that a lot of GWT apps and widgets set the indent explicitly then I think we need setLeadingMargin/setTrailingMargin. If the future is for indents to all go into CSS then we don't.

I did some quick testing and the patch seems to be working nicely.  So the patch looks good as far as I can see.

I also did some work to bidi-fy (if that is a real verb) the Showcase sample. Showcase is very impressive. I really like the new decoration and animation effects! I have the following notes:

- animations for menus etc, definitely need rtl equivalents.

- DecoratorPanel adds "left" and "right" decorations. To follow the same principle as DockPanel "left" and "right" should keep their meaning - that would save a lot of extra CSS work. So I modified it to swap the order of the TR's in createTR() depending on the locale rtl so:

  static Element createTR(String styleName) {
    Element trElem = DOM.createTR();
    setStyleName(trElem, styleName);
    if (LocaleInfo.getCurrentLocale().isRTL()) {
      DOM.appendChild(trElem, createTD(styleName + "Right"));
      DOM.appendChild(trElem, createTD(styleName + "Center"));
      DOM.appendChild(trElem, createTD(styleName + "Left"));
    }
    else {
      DOM.appendChild(trElem, createTD(styleName + "Left"));
      DOM.appendChild(trElem, createTD(styleName + "Center"));
      DOM.appendChild(trElem, createTD(styleName + "Right"));
    }
    return trElem;
  }

- I changed the GWT-default.rtl.css in Showcase to remove any special formatting for right and left decorations (enclosed).

- Showcase puts the shadow for the custom buttons on the opposite side for rtl layout. From experience shadows should not be swapped for rtl layout.

- Showcase has a method called: includeStyleSheets(). This replaces the current css file reference to refer to the rtl css file if the locale is rtl. It would be nice if the functionality of this method can be integrated into GWT proper so all an app has to do is include a ".rtl.css" and GWT will just know when to use it without any extra code.

- also I noticed that the rtl version of the css file had some errors. This made me think. In principle, when we have an RTL css, it should be done in a way where it only overrides the selectors that need to change not the whole file. That would make code more reliable. Would that be OK with all browsers or is it safer to replace the whole file?

- naming conventions. When we have a specific rtl version of a file we should have a standard naming convention. Currently there are three. e.g:  blah-rtl.png, blah_rtl.png or blah.rtl.png. I suggest we match the i18n files and have blah_rtl.png.

I think that's it.

regards

Adil

Content-Type: text/x-patch; name=bidilayout_updated-r2078.patch
X-Attachment-Id: f_fdqt80c50
Content-Disposition: attachment; filename=bidilayout_updated-r2078.patch

Attachment converted: adil:bidilayout_updated-r2078.patch (TEXT/ttxt) (0066C9FD)
Content-Type: application/zip; name=rtlimages.zip
X-Attachment-Id: f_fdqt8gm81
Content-Disposition: attachment; filename=rtlimages.zip

Attachment converted: adil:rtlimages.zip (pZIP/«IC») (0066C9FE)

%GWT-default.rtl.css
GWT-default.rtl.css

Adil

unread,
Mar 26, 2008, 8:26:03 AM3/26/08
to Google Web Toolkit Contributors, rda...@google.com, shan...@google.com
Hi Rajeev,

Now that popups have decorations there is a new issue with the bidi
patch to the MenuBar class.

in MenuBar.doItemAction() the callback passed to
popup.setPopupPositionAndShow() should use the offset width that is
passed to the callback and not get it from the item as the item width
does not have the decorations. So it should look like this:

popup.setPopupPositionAndShow(new PopupPanel.PositionCallback() {
public void setPosition(int offsetWidth, int offsetHeight) {

// depending on the bidi direction position a menu on the left
or right
// of its base item
if (LocaleInfo.getCurrentLocale().isRTL()) {
if (vertical) {
popup.setPopupPosition(MenuBar.this.getAbsoluteLeft() -
offsetWidth + 1,
item.getAbsoluteTop());
} else {
popup.setPopupPosition(item.getAbsoluteLeft() +
item.getOffsetWidth() - offsetWidth,
MenuBar.this.getAbsoluteTop() +
MenuBar.this.getOffsetHeight() - 1);
}
} else {
if (vertical) {
popup.setPopupPosition(MenuBar.this.getAbsoluteLeft() +
MenuBar.this.getOffsetWidth() - 1,
item.getAbsoluteTop());
} else {
popup.setPopupPosition(item.getAbsoluteLeft(),
MenuBar.this.getAbsoluteTop() +
MenuBar.this.getOffsetHeight() - 1);
}
}
}
});

regards

Adil
> HasDirection.Directionenum. Values are RTL, LTR, or UNSET.
> On Fri, Mar 7, 2008 at 7:03 AM, Adil Allawi <adil.all...@gmail.com> wrote:
> >  I did some testing on Firefox 3 with RTL layout  - the issues I
> > previously noticed were fixed in one of the recent nightly builds but I did
> > notice a general problem.
>
> > If you open KitchenSink (with locale = ar) in Firefox and look at the
> > Lists tab you will notice that the tree widget items and the text box label
> > are left aligned. GWT Panels are rendered in HTML as tables. In the Lists
> > tab the content is made up of a vertical panel that is explicitly left
> > aligned which embeds a horizontal panel that embeds the widgets so:
>
> > Vertical Panel (align = left) -> Horizontal Panel (default align) ->
> > Widgets
>
> > In Firefox, the left alignment of the vertical panel is affecting the
> > default alignment of the horizontal panel only if the layout is rtl. This is
> > also a general issue. I made a simple test case and tried it in IE, FF and
> > Safari (seehttp://www.diwan.com/w/rtl-table.html).
>  bidilayout_updated-r2078.patch
> 84KDownload
>
>  rtlimages.zip
> 1KDownload

Rajeev Dayal

unread,
Mar 26, 2008, 11:11:48 AM3/26/08
to Adil Allawi, Joel Webber, Google Web Toolkit Contributors, Kelly Norton, Shanjian Li
Hey Adil,

Sorry for the latency in the reply. Again, thanks very much for looking at this work so closely. Comments to your comments below:

On Thu, Mar 13, 2008 at 4:30 PM, Adil Allawi <adil....@gmail.com> wrote:
Hi Rajeev,

Great work - and thanks for taking my comments to heart!

- your zip has an incorrect image it should have menuBarSubMenuIcon_rtl.gif but has menuBarSubMenuIcon.gif instead.

DuH!. Thanks for catching this.


- I think setSplitPosition does not need a bidi equivalent as the graphics environment has its zero offset on the left regardless so programmers should be used to this.

Ok, that was my feeling as well, but it is nice to know that someone feels the same way.


- DisclosurePanel definitely needs a left-pointing arrow for rtl.

I'll put this in the next patch.
 

- setLeadingMargin/setTrailingMargin was kind-of my idea and I spent some time persuading Shanjian to accept it! When I decided to implement this I saw some widgets were setting their indent programmatically (e.g. the Tree widget). If it is the case that a lot of GWT apps and widgets set the indent explicitly then I think we need setLeadingMargin/setTrailingMargin. If the future is for indents to all go into CSS then we don't.

Really, Tree should not be setting the margins in code. They should be controlled by CSS. Unfortunately, since we've already gone down that path, we can't just switch the margins to use CSS, as this would be a visually-breaking change. But, the future is definitely for indents to go in CSS.


I did some quick testing and the patch seems to be working nicely.  So the patch looks good as far as I can see.

I also did some work to bidi-fy (if that is a real verb) the Showcase sample. Showcase is very impressive. I really like the new decoration and animation effects! I have the following notes:

I noticed your work on another thread; I'll check out your latest incarnation shortly.


- animations for menus etc, definitely need rtl equivalents.

Yes, the menu animation should use the top-right corner as the origin in RTL layout; right now, it uses the top-left corner.


- DecoratorPanel adds "left" and "right" decorations. To follow the same principle as DockPanel "left" and "right" should keep their meaning - that would save a lot of extra CSS work. So I modified it to swap the order of the TR's in createTR() depending on the locale rtl so:

  static Element createTR(String styleName) {
    Element trElem = DOM.createTR();
    setStyleName(trElem, styleName);
    if (LocaleInfo.getCurrentLocale().isRTL()) {
      DOM.appendChild(trElem, createTD(styleName + "Right"));
      DOM.appendChild(trElem, createTD(styleName + "Center"));
      DOM.appendChild(trElem, createTD(styleName + "Left"));
    }
    else {
      DOM.appendChild(trElem, createTD(styleName + "Left"));
      DOM.appendChild(trElem, createTD(styleName + "Center"));
      DOM.appendChild(trElem, createTD(styleName + "Right"));
    }
    return trElem;
  }

Good catch, I missed this. I can include this work as part of this patch.
 

- I changed the GWT-default.rtl.css in Showcase to remove any special formatting for right and left decorations (enclosed).

Great, I'll take a look at this.


- Showcase puts the shadow for the custom buttons on the opposite side for rtl layout. From experience shadows should not be swapped for rtl layout.

Ok, I'll talk with John L. about this.
 

- Showcase has a method called: includeStyleSheets(). This replaces the current css file reference to refer to the rtl css file if the locale is rtl. It would be nice if the functionality of this method can be integrated into GWT proper so all an app has to do is include a ".rtl.css" and GWT will just know when to use it without any extra code.

Yes, that is actually how it is going to work in the future. This solution is just an interim one. FooBundle will solve this problem. When we integrate FooBundle into GWT proper, the inclusion of the rtl.css file will work in the manner that you describe.


- also I noticed that the rtl version of the css file had some errors. This made me think. In principle, when we have an RTL css, it should be done in a way where it only overrides the selectors that need to change not the whole file. That would make code more reliable. Would that be OK with all browsers or is it safer to replace the whole file?

I think that there should be a way to override specific selectors in the rtl case, while leaving the others alone. I'm not in favor of maintaining two files, where they both have a fair amount with duplication. Even if there were some code generation solution, where an rtl version of a CSS file was generated from a single source, that would be better than having two separate CSS files. I know that there has been some discussion about the best way to do this on another thread, so we can continue the discussion there.


- naming conventions. When we have a specific rtl version of a file we should have a standard naming convention. Currently there are three. e.g:  blah-rtl.png, blah_rtl.png or blah.rtl.png. I suggest we match the i18n files and have blah_rtl.png.

Good point; I'll make the changes for those rtl-specific resources that we've included.


I think that's it.

regards

Adil

At 12:35 -0400 13/3/08, Rajeev Dayal wrote:

Rajeev Dayal

unread,
Mar 26, 2008, 11:13:31 AM3/26/08
to Adil, Google Web Toolkit Contributors, shan...@google.com
Good catch, Adil!
I'll address this in the updated patch (coming soon to a forum near you).

Joel Webber

unread,
Apr 1, 2008, 5:13:16 PM4/1/08
to Google-Web-Tool...@googlegroups.com, Adil, shan...@google.com
BidiUtils, HasDirection:
  - isDirectionRTLOnElement() implies boolean return, but is enum (should probably be get)
  - Direction.UNSET could be DEFAULT. You could then make setDirectionOnElement() take an enum as well (DEFAULT => clear property)
TextBox:
  - Minor formatting issues in setDirection()
HorizontalSplitPanel:
  - The 'final' qualifier was removed from a number of methods (e.g., getRightWidget()). Was this done for any particular reason?
MenuBar:
  - TODO left in (we should just make an issue)
MenuBar:
  - menuBarSubMenuIcon_rtl.gif appears to be missing
  - Probably shouldn't be adding an rtl image to this image bundle, as it generates two *simultaneous* images. See below.
SuggestBox:
  - Could use some tests for popup positioning (that stuff does get a bit complicated...)
ListBox:
  - Not showing up RTL on Safari3 (could simply be a browser bug)

A few notes on RTL images:
  MenuBar, Tree, & DisclosurePanel have the problem that they allow images to be specified that might need to change for RTL locales. We need a way to make these user-specifiable (they currently all use ImageBundle) while only loading one image at a time. One proposal would be to continue to allow the ImageBundle to be passed into the ctor, and have the default ctor check for RTL and possibly pass a replacement to the ImageBundle ctor. An example would be:

public interface TreeImages extends ImageBundle {


  /**

   * An image indicating an open branch.

   * 

   * @return a prototype of this image

   */

  AbstractImagePrototype treeOpen();


  // ...

}


public interface TreeImagesRTL extends TreeImages {


  @Resource("treeOpenRTL.gif")

  AbstractImagePrototype treeOpen();


  // ...

}


Tree {

  public Tree() {

    if (rtl) {

      this(GWT.<TreeImages>create(TreeImages.class));

    } else {

      this(GWT.<TreeImagesRTL>create(TreeImagesRTL.class));

    }

  }


  public Tree(TreeImages images) {

    // ...

  }

}

Rajeev Dayal

unread,
Apr 2, 2008, 5:57:25 PM4/2/08
to Google-Web-Tool...@googlegroups.com, Joel Webber, Adil, shan...@google.com
Hey Joel,

Thanks for the review. Comments inline:

On Tue, Apr 1, 2008 at 5:13 PM, Joel Webber <j...@google.com> wrote:
BidiUtils, HasDirection:
  - isDirectionRTLOnElement() implies boolean return, but is enum (should probably be get)
  - Direction.UNSET could be DEFAULT. You could then make setDirectionOnElement() take an enum as well (DEFAULT => clear property)

Done.
 
TextBox:
  - Minor formatting issues in setDirection()

Fixed.
 
HorizontalSplitPanel:
  - The 'final' qualifier was removed from a number of methods (e.g., getRightWidget()). Was this done for any particular reason?

I was just getting some IDE warnings about them. Since HorizontalSplitPanel is a final class, the finals on the methods are redundant. However, I have no problem adding them back in. Maybe it is a better idea to have finals on all of the methods just in case someone decides to remove the final from the class definition

MenuBar:
  - TODO left in (we should just make an issue)
MenuBar:
  - menuBarSubMenuIcon_rtl.gif appears to be missing
  - Probably shouldn't be adding an rtl image to this image bundle, as it generates two *simultaneous* images. See below.

Fixed.
 

SuggestBox:
  - Could use some tests for popup positioning (that stuff does get a bit complicated...)

Right, I will write some tests for the popup positioning.

ListBox:
  - Not showing up RTL on Safari3 (could simply be a browser bug)

This looks like it is a browser bug. FF and IE seem to do the right thing.


A few notes on RTL images:
  MenuBar, Tree, & DisclosurePanel have the problem that they allow images to be specified that might need to change for RTL locales. We need a way to make these user-specifiable (they currently all use ImageBundle) while only loading one image at a time. One proposal would be to continue to allow the ImageBundle to be passed into the ctor, and have the default ctor check for RTL and possibly pass a replacement to the ImageBundle ctor. An example would be:

public interface TreeImages extends ImageBundle {


  /**

   * An image indicating an open branch.

   * 

   * @return a prototype of this image

   */

  AbstractImagePrototype treeOpen();


  // ...

}


public interface TreeImagesRTL extends TreeImages {


  @Resource("treeOpenRTL.gif")

  AbstractImagePrototype treeOpen();


  // ...

}


Tree {

  public Tree() {

    if (rtl) {

      this(GWT.<TreeImages>create(TreeImages.class));

    } else {

      this(GWT.<TreeImagesRTL>create(TreeImagesRTL.class));

    }

  }


  public Tree(TreeImages images) {

    // ...

  }

}

Good idea, I made the relevant changes for DisclosurePanel, Tree, and MenuBar. In the case of Tree and MenuBar, I had to create an init method that the constructors would call, since you can only invoke one constructor from another if the invocation is on the first line.

I also made the fix to MenuBar that Adil suggested a couple of messages ago in this thread.

Just as a note, here are other bidi-related things that I want to get into GWT 1.5 (but after the M2 build):

-Shanjian's patch to include bidi text utilities
-Adil's RTL fixes for showcase (which also rtl-ify the menu animations and the DecoratorPanel drop shadows)
-HorizontalSplitPanel - have the left pane shrink on window resize as opposed to the right pane shrinking
-unit tests


Thanks,
Rajeev

bidilayout_updated2-r2309.patch
rtlimages_updated2.zip

Joel Webber

unread,
Apr 3, 2008, 12:32:01 PM4/3/08
to Rajeev Dayal, Google-Web-Tool...@googlegroups.com, Adil, shan...@google.com
LGTM. The only question I have is whether it's actually useful to create TreeImagesRTL. It's only used internally, but doesn't actually redefine any images. Is it just there for future expansion? (If so, feel free to go ahead and commit and just tell me so!)

Rajeev Dayal

unread,
Apr 3, 2008, 1:55:46 PM4/3/08
to Joel Webber, Google-Web-Tool...@googlegroups.com, Adil, shan...@google.com
Hey Joel,

Thanks for the review. As for your question about TreeImagesRTL, you are right - it is for future expansion. I just want other developers to be aware that if the Tree images ever change, they may have to add RTL equivalents.

Committed as r2350.


Rajeev
Reply all
Reply to author
Forward
0 new messages