Closed Bug 653688 Opened 9 years ago Closed 8 years ago

[RTL] Faulty margin value for edit textboxes

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(blocking-fennec1.0 -)

VERIFIED FIXED
Tracking Status
blocking-fennec1.0 --- -

People

(Reporter: linostar, Assigned: ehsan)

References

Details

(Keywords: testcase-wanted, Whiteboard: [fennec 7.0b5])

Attachments

(2 files)

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
Assignee: nobody → linux.anas
Status: NEW → ASSIGNED
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.
This issue is caused
Summary: [RTL] Faulty padding value for edit textboxes → [RTL] Faulty margin value for edit textboxes
Depends on: 646027
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.
Isn't it perhaps possible to use -moz-padding-start/-moz-padding-end and/or -moz-margin-start/-moz-margin-end?
Changing the padding/margin start/end of ".anonymous-div:after" won't do. We should look for a fix elsewhere...
(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?
(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.
(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...
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?
(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.
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.
(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.
(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.
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?
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.
Attached image screenshot
Whiteboard: [fennec 7.0b5]
Duplicate of this bug: 713635
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.
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.
Keywords: testcase-wanted
Attached file 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.
this should be blocking fennec 1.0 as well.  its causing real problems with RTL on current native fennec android.
blocking-fennec1.0: --- → ?
Chris, where are you encountering this? Do you have a testcase?
Anas  and Cristian,  Can you provide test cases for comment 0 and comment 16?
blocking-fennec1.0: ? → -
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?
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
Assignee: linux.anas → ehsan
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
VERIFIED FIXED in current builds.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.