653688 - [RTL] Faulty margin value for edit textboxes

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anas Husseini [:linostar]]

In RTL Fennec, RTL textboxes have negative padding values on the right, while LTR textboxes have the same values but positive. Both are wrong and undesired.

See the first and second edit textboxes in the following screenshot:
https://bug647755.bugzilla.mozilla.org/attachment.cgi?id=529064

Comment 1

[Anas Husseini [:linostar]]

I should also mention that this doesn't happen in all builds. So far, nightly linux builds are free of this issue. Android builds and a local linux build (that has been compiled with --enable-ui-locale=ar) are not.

Comment 2

[Anas Husseini [:linostar]]

This issue is caused

Comment 3

[Anas Husseini [:linostar]]

Note: nightly builds are also affected. the builds I tested before was older than the patch of bug 646027.

One way of solving this, without regressing bug 646027, is to use a css pseudo-class called -moz-element-dir(), a pseudo-class yet to be created. This is because adding the space 'after' or 'before' anonymous-div depends on the element direction, not the the browser direction.

Should we take the chance to create this pseudo-class that'll be useful for rtl design hacks.

Comment 4

[Martijn Wargers (dead)]

Isn't it perhaps possible to use -moz-padding-start/-moz-padding-end and/or -moz-margin-start/-moz-margin-end?

Comment 5

[Anas Husseini [:linostar]]

Changing the padding/margin start/end of ".anonymous-div:after" won't do. We should look for a fix elsewhere...

Comment 6

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

(In reply to comment #3)
> Note: nightly builds are also affected. the builds I tested before was older
> than the patch of bug 646027.
> 
> One way of solving this, without regressing bug 646027, is to use a css
> pseudo-class called -moz-element-dir(), a pseudo-class yet to be created. This
> is because adding the space 'after' or 'before' anonymous-div depends on the
> element direction, not the the browser direction.

I wonder if bug 562169 is what you're looking for?

Comment 7

[Anas Husseini [:linostar]]

(In reply to comment #6)
> I wonder if bug 562169 is what you're looking for?

Sounds not a complete solution because of this:

>  The selectors are not affected by the value of the CSS 'direction'
>  property, only the directionality declared in the markup language.

Comment 8

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

(In reply to comment #7)
> (In reply to comment #6)
> > I wonder if bug 562169 is what you're looking for?
> 
> Sounds not a complete solution because of this:
> 
> >  The selectors are not affected by the value of the CSS 'direction'
> >  property, only the directionality declared in the markup language.

Right.
This make me wonder if :ltr/rtl selectors are useful at all...

Comment 9

[(no longer active)]

Well, the patch in bug 646027 seems like a bad idea to me.  We should have just used -moz-padding-end: 16px on the container of the text.  Why is that not an option?

Comment 10

[(no longer active)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I wonder if bug 562169 is what you're looking for?
> > 
> > Sounds not a complete solution because of this:
> > 
> > >  The selectors are not affected by the value of the CSS 'direction'
> > >  property, only the directionality declared in the markup language.
> 
> Right.
> This make me wonder if :ltr/rtl selectors are useful at all...

Those selectors are probably going to be used for a different purpose: bug 548206.  But in general, I think for most cases we could do without a selector which selects on the computed direction, since we can just use the selector that we have used to set the direction property on.

Comment 11

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

As per IRC discussion with Anas there are (at least) 3 solutions for this bug:
 * do not use :after/:before for adding some scrollable margin to the textboxes
(I don't really know how to do this without that)
 * add a -moz-element-dir() similar to -moz-locale-dir() but for a particular
element
 * Make :after/:before direction aware. Actually they works depending on the
document direction but not on the element direction)

Personally I think the later is the way to go but I would like some
confirmation from people more rtl aware.

Comment 12

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

(In reply to comment #9)
> Well, the patch in bug 646027 seems like a bad idea to me.  We should have just
> used -moz-padding-end: 16px on the container of the text.  Why is that not an
> option?

Because this additional space need to be displayable by scrolling and clickable, which does not work with padding.

Comment 13

[(no longer active)]

(In reply to comment #11)
> As per IRC discussion with Anas there are (at least) 3 solutions for this bug:
>  * do not use :after/:before for adding some scrollable margin to the textboxes
> (I don't really know how to do this without that)

See below.

>  * add a -moz-element-dir() similar to -moz-locale-dir() but for a particular
> element

I don't believe this is necessary.

>  * Make :after/:before direction aware. Actually they works depending on the
> document direction but not on the element direction)

:after/:before work based on the DOM tree, and I don't think that's something that we can change.

(In reply to comment #12)
> (In reply to comment #9)
> > Well, the patch in bug 646027 seems like a bad idea to me.  We should have just
> > used -moz-padding-end: 16px on the container of the text.  Why is that not an
> > option?
> 
> Because this additional space need to be displayable by scrolling and
> clickable, which does not work with padding.

Can you please provide a sample HTML test case which shows what we need to achieve?  When you're talking about displayable by scrolling, I'm thinking of having an element with overflow-x:scroll which has an element with a padding-end inside it.  When you're talking about clickable, padding should work, since the padding is part of the element's width.  I don't know enough about the Fennec UI to tell whether I'm on the right track here though.

Comment 14

[Anas Husseini [:linostar]]

Just to make the matter more complicated. Making the position of the margin/padding based on the element direction isn't accurate. In fact, the margin should be next to the position of the cursor when positioned after the last letter. In case we have an rtl textbox and we want to input english text in it, the desired margin place will be at the right (because the cursor stands at right after an english letter), and therefore the margin will be at the *start* of the textbox. But if the same textbox was used for entering arabic text, the margin will be needed at the left (the end of the textbox). The content of a text being unpredicatble, we should set a margin values at both ends of the textbox, in a way that makes it appear the same whatever were the locale, the element direction, or the element content. I'd suggest to make the margin value a little smaller, e.g. 8px (equivalent to 1 letter width), to lessen the white spaces added within the element.

What do you think?

Comment 15

[(no longer active)]

That would be more complicated, and the platform doesn't provide support for this right now.

That said, the original problem here might just be bug 542677.

Comment 16

[Cristian Nicolae (:xti)]

Attachment: screenshot

Comment 18

[Simon Montagu :smontagu]

Tomer pointed me to a further problem which I think is a consequence of this bug: with some combinations of text the empty "after" text and its margin participate in bidi reordering.

To reproduce, enter "!א" in a left-to-right textbox, or "!a" in a right-to-left textbox.

Comment 19

[(no longer active)]

Anas, are you still working on this bug?

Is there any way to reproduce this bug in the desktop Firefox?  Using the layout debugger might be a very good way of showing us what's going wrong here.

Comment 20

[Simon Montagu :smontagu]

Attachment: Failed testcase

This was an attempt to reproduce the bug by copying anything connected to <input> from mobile/android/themes/core/*.css, but it doesn't exhibit the bug on desktop. Maybe someone will be able to find what's missing.

Comment 21

[chris hofmann]

this should be blocking fennec 1.0 as well.  its causing real problems with RTL on current native fennec android.

Comment 22

[Martijn Wargers (dead)]

Chris, where are you encountering this? Do you have a testcase?

Comment 23

[chris hofmann]

Anas  and Cristian,  Can you provide test cases for comment 0 and comment 16?

Comment 24

[Tomer Cohen :tomer]

This bug still occur on Aurora 18 and is affecting every RTL text input, including web content (see bug 713635), making Firefox non functional for RTL content on Android. What is required in order to fix this bug?

Comment 25

[(no longer active)]

The "fix" for bug 646027 was completely broken and bogus, and I backed it out in order to fix this bug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ae169b47105

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ae169b47105

Comment 27

[Simon Montagu :smontagu]

VERIFIED FIXED in current builds.