647755 - [RTL] Edit bookmark's textboxes have a wrong layout direction in RTL

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Steps to reproduce:
 * Launch fennec and open about:config
 * Pan to the right to show the left scrollbar
 * Click/Tap on the bookmark's star
 * On the new dialog opened, try to type about:config/ as a name/url for the bookmark

Actual result:
 * The '/' is wrongly positioned

Expected result:
 * The '/' is positioned at the end of the url


Anas,
do you know what is the correct behavior for the first textbox? This textbox is normally reserved for the name of the bookmark, but if the page does not have any title, the name is represented by the url.
So does this textbox should be rtl oriented or stays ltr?

For the second textbox, there is clearly 2 bugs:
 * the textbox should have the generic class 'uri-element' that force the display to rtl
 * the textbox should have type="url" so VKB's handling on the device will be better


For the third textbox, I have no idea what is needed for RTL!

Comment 1

[Anas Husseini [:linostar]]

Let's start with the easiest one:

- The third textbox should be rtl, since tags are usually comma-separated and no fear there as long as the last tag finishes with a point or comma (which is unlikely). 

- The second textbox should be made ltr, since it may only contain an url. 

- As for he first textbox, and the most troublesome one, I suggest to leave rtl for now, so it can correctly display Arabic titles with parantheses and points (and even mixed Arabic and English titles). The side-effect is, as you mentioned, for titles that start with an English letter and ends with a point/slash/comma/etc. Firefox's RTL folk are working on a method that decides the directionality based on the few first characters of the content. If that leads to the desired result, we may port it to Fennec.

I'll prepare a patch that carries out the modifications I suggested above, if you have no objection on them.

Comment 2

[Anas Husseini [:linostar]]

Attachment: Screenshot showing mixed content in edit bookmark fields

Notice that you should set bidi.browser.ui to 'true' so that the comma-seperated tags appear in right order.

Comment 3

[Anas Husseini [:linostar]]

By(In reply to comment #0)
> For the second textbox, there is clearly 2 bugs:
>  * the textbox should have the generic class 'uri-element' that force the
> display to rtl
I believe uri-element keeps the display ltr, because that's how the text looks in the url bar.

>  * the textbox should have type="url" so VKB's handling on the device will be
> better
in bindings.xml, the textbox inherit from "value=uri". does this give a different effect?

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #3)
> By(In reply to comment #0)
> > For the second textbox, there is clearly 2 bugs:
> >  * the textbox should have the generic class 'uri-element' that force the
> > display to rtl
> I believe uri-element keeps the display ltr, because that's how the text looks
> in the url bar.

Obviously! :)

> >  * the textbox should have type="url" so VKB's handling on the device will be
> > better
> in bindings.xml, the textbox inherit from "value=uri". does this give a
> different effect?

Yes. value=uri means the editable value of the textbox will be set to the string from the uri attribute.

Adding type="url" on the textbox will be inherited by the inner html:input of the textbox binding (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/textbox.xml#23).
This information will later be retrieve by the IME code (http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#305) when the element gains focus and sent thought the window widget for Android  to the native android Java wrapper code (http://mxr.mozilla.org/mozilla-central/source/widget/src/android/nsWindow.cpp#1755 -- http://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoSurfaceView.java#352) and so the displayed keyboard on such device will be adapted to the field's type




(In reply to comment #1)
> Let's start with the easiest one:
> 
> - The third textbox should be rtl, since tags are usually comma-separated and
> no fear there as long as the last tag finishes with a point or comma (which is
> unlikely). 

Works for me.

> 
> - The second textbox should be made ltr, since it may only contain an url. 

Agreed.

> 
> - As for he first textbox, and the most troublesome one, I suggest to leave rtl
> for now, so it can correctly display Arabic titles with parantheses and points
> (and even mixed Arabic and English titles). The side-effect is, as you
> mentioned, for titles that start with an English letter and ends with a
> point/slash/comma/etc. Firefox's RTL folk are working on a method that decides
> the directionality based on the few first characters of the content. If that
> leads to the desired result, we may port it to Fennec.

I assume this is platform's code? If so it is likely that's we won't have to change anything in Fennec since it is the same code as Firefox Desktop, we will just say "Wouahou!"

> 
> I'll prepare a patch that carries out the modifications I suggested above, if
> you have no objection on them.

Go ahead! I like reviewing rtl Front-End patches :)

Comment 5

[Anas Husseini [:linostar]]

Attachment: patch switching only the second textbox to ltr

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 524389 [details] [diff] [review]
patch switching only the second textbox to ltr

>--- a/chrome/content/bindings.xml	Fri Apr 01 02:27:05 2011 +0200
>-            <xul:textbox anonid="uri" xbl:inherits="value=uri"/>
>+            <xul:textbox anonid="uri" xbl:inherits="value=uri" xbl:type="url"/>

type="url" is enough, adding xbl: is because the element is declared in the xul namespace (xul:textbox) so by default attributes will point to this namespace (afaict)

You also need to add class="uri-element" (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#67)

>             <xul:textbox anonid="tags" xbl:inherits="value=tags" emptytext="&editBookmarkTags.label;"/>
>           </xul:vbox>


>diff -r 8167d57cab8e themes/core/browser.css
> 
>+placeitem[ui="manage"] > .bookmark-manage > vbox > vbox > textbox[anonid="uri"]:-moz-locale-dir(rtl) {
>+  direction: ltr;
>+  text-align: right;
>+}
>+

The rule seems bit too specific to me, sounds like if we change the inner structure of the xbl it will be broken.
Maybe something like: 
placeitem[ui="manage"] > .bookmark-manage textbox[anonid="uri"]:-moz-locale-dir(rtl) {
}

Also there is no need for direction: ltr if you add class="uri-element". I wonder if we need to start using something like uri-element-right-align? 

see http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#117
see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#207

What do you think about that?

Comment 7

[Anas Husseini [:linostar]]

Sounds a good idea. class="uri-element" would take care of the ltr direction. However, the uri-element-right-align looks inadequate to me for generic use. in style/form.css it is specific to ".anonymous-div", and in content/browser.css it mentions nothing about text-align. Do you suggest making one generic css class that overrules firefox and fennec?

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #7)
> Sounds a good idea. class="uri-element" would take care of the ltr direction.
> However, the uri-element-right-align looks inadequate to me for generic use. in
> style/form.css it is specific to ".anonymous-div",

.anonymous-div is the name of div frame created for every input elements but invisible in DOM tree (it belongs to the frame tree). So this is not that much specific.

> and in content/browser.css
> it mentions nothing about text-align. Do you suggest making one generic css
> class that overrules firefox and fennec?

It looks like we don't want such things to not regress http://hg.mozilla.org/mozilla-central/rev/108aec53a280 . 
It looks like the separation has been done to not break the switch text direction feature (that I've never used!)

Comment 9

[Anas Husseini [:linostar]]

I see. I'll prepare a patch that uses uri-element-right-align, as soon as I get back to my box.

Comment 10

[Anas Husseini [:linostar]]

Attachment: patch v2

Thinking about this again, I didn't see it reasonable to use "uri-element-right-align" since it will be applied to both ltr and rtl fennec. Therefore I used "uri-element" with "text-align: right" in browser.css.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Does using the last ">" still make the css rules works? (.bookmark-manage > textbox[anonid="uri"])

Comment 12

[Anas Husseini [:linostar]]

Attachment: patch v3

Does using the last ">" still make the css rules works? (.bookmark-manage >
textbox[anonid="uri"])

For an abovious reason, no :)

Comment 13

[Anas Husseini [:linostar]]

> For an abovious reason, no :)

That was supposed to be "obvious", but it wasn't quite obvious itself.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 525339 [details] [diff] [review]
patch v3

Again thanks for the patch :)

And sorry for the delay taken for landing this one and the other 2 bugs I have r+'ed, mobile-browser is gone and has merge with mozilla-central since last week.

Which mean I need to take good habits and start using MQ right now, but I'm not used to land things with MQ. I will try to do that this afternoon, asking around what should I do.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #13)
> > For an abovious reason, no :)
> 
> That was supposed to be "obvious", but it wasn't quite obvious itself.

obviously!

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mozilla-central/rev/bdc8cb9b2270

Comment 17

[Anna (Waverley)]

Following the steps to reproduce this is how I get: please see the screenshot.
As a name for the bookmarked page I have: "about:config". If I type "/", I get: "/about:confi". Shouldn't it be: "about:config/" ?

I don't think this is the expected result. Should I reopen the bug?

Comment 18

[Anna (Waverley)]

Attachment: actual result

Comment 19

[Anas Husseini [:linostar]]

Yes, this is expected. For now, the title box (the first textbox) has the same direction as the browser locale, because the title may be in English, Arabic or in both. Once bug 548206 if fixed, the title box direction will automatically adapt to the textbox/label content.

Comment 20

[Anna (Waverley)]

So, can I validate this one (since it's resolved fixed, but still depends on bug 548206) or should I wait for bug 548206?

Comment 21

[Anas Husseini [:linostar]]

I think it is better to wait for bug 548206 to get fixed. Depending on the way that bug is fixed, this bug may need to be reopened.

Comment 22

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Cannot verify until bug 548206 is fixed.