997921 - Regression - Cursor doesn't change to RTL shape

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Yaron]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140414143035

Steps to reproduce:

Position the cursor in the URL bar (or search box) on the left side.
Press Alt+Shift to switch input language (from English to Hebrew in my case). 


Actual results:

The cursor doesn't change to RTL shape (i.e. top turned to left).
It only changes if you press Ctrl+Shift+X and move the cursor to the right.

This is a regression.

Comment 1

[:Gijs (out for now; he/him)]

Are you able to use mozregression to figure out when this regressed?

Comment 2

[Yaron]

Please see the following posts. Some code change must have caused the regression.

https://bugzilla.mozilla.org/show_bug.cgi?id=979653
https://bugzilla.mozilla.org/show_bug.cgi?id=993806

Comment 3

[:Gijs (out for now; he/him)]

So I would like to figure out what regressed this and so on, but on all the OSes I've tried (Windows 8, Windows 7, OS X), the cursor inside the input box for the url and search box is just a vertical line. There is no indication of the text direction. I must be misunderstanding comment #0, or not know about some option that's at play here (can you alter the cursor? Is the cursor different by default on RTL Windows versions?). Yaron, can you clarify what you mean, or how to reproduce this?

Comment 4

[Yaron]

bidi.browser.ui is set to true (default) on RTL Windows versions.
This setting makes the cursor change shape according to input language.

Thank you. I appreciate it. :)

Comment 5

[Yaron]

Or maybe using Firefox Hebrew sets bidi.browser.ui to true.

Comment 6

[(no longer active)]

We decide on the direction of the caret based on both bidi.browser.ui being true and the currently active keyboard layout (the code is here: <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCaret.cpp#1062>), so in order to reproduce the bug, you need to set the pref, install an RTL keyboard locale, and switch to it.  On Mac, I cannot reproduce using a Persian keyboard.

Also please note that neither of the bugs mentioned in comment 2 are relevant here.

Yaron, which version of Firefox did this work in for you and which version broke it?  Do you get the RTL shape caret in text boxes on web pages?  Just in order to check, can you please go to about:config and make sure that the bidi.browser.ui pref is set to true in your profile?

Comment 7

[Yaron]

Thank you.

It used to work properly in FF 28 (and earlier versions).
bidi.browser.ui is set to true.
On web pages the behavior is OK.

I've attached a screenshot to make it clearer.
In the first image the input language is English, and the cursor is on the left (the shape is correct).
In the second image the input language is Hebrew, and the cursor is on the left (the shape is WRONG).
In the third image the input language is Hebrew, and the cursor is on the right(the shape is correct).

http://s29.postimg.org/9ppghvamf/Caret.png

Comment 8

[Simon Montagu :smontagu]

I suspect that what is happening here is that the positioning of the caret, or the size of the padding in the input field, has changed, with the result that when the directional pointer points left it is hanging outside the visible area of the input field.

To test this theory, try typing a single space character when the input language is Hebrew and the caret is at the left edge of the field, and see if the directional pointer reappears.

Comment 9

[(no longer active)]

(In reply to Simon Montagu :smontagu from comment #8)
> I suspect that what is happening here is that the positioning of the caret,
> or the size of the padding in the input field, has changed, with the result
> that when the directional pointer points left it is hanging outside the
> visible area of the input field.

That would be surprising to me.  I thought that this basically paints the caret on top of whatever is already there: <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCaret.cpp#527>.  CCing roc and Matt to verify my claim.

Comment 10

[Yaron]

This is clever!
Typing a single space character and, lo and behold, the hook reappears.

Could you please refer me to the relevant code?

Thank you.

Comment 11

[(no longer active)]

(In reply to comment #10)
> This is clever!
> Typing a single space character and, lo and behold, the hook reappears.

Thanks!  That confirms Simon's theory.

> Could you please refer me to the relevant code?

See comment 9 please.  :-)

Comment 12

[Yaron]

Thank you Ehsan.

I'm not a pro. :)
Where is the file located in omni.ja?
Should I compare the relevant part with the file of FF 28 and change accordingly?

Anyway, I don't know how to create a patch. Would you do that?

*

And, of course, thanks to Simon.

Comment 13

[(no longer active)]

(In reply to comment #12)
> Thank you Ehsan.
> 
> I'm not a pro. :)
> Where is the file located in omni.ja?
> Should I compare the relevant part with the file of FF 28 and change
> accordingly?

Oh, this file is part of the C++ code that gets compiled into native binary code located somewhere in xul.dll.  There is no easy way to patch this up on your side.

> Anyway, I don't know how to create a patch. Would you do that?

I can write a patch once I know what the right fix is...  Currently waiting on the answer to comment 9.  Please be patient.  :-)

Thanks!

Comment 14

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/1f4f766ea3df
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140116120222
Bad:
http://hg.mozilla.org/mozilla-central/rev/bcbe93f41547
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140116120752
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1f4f766ea3df&tochange=bcbe93f41547

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/994659cbc145
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140115185609
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/36e4fcbd07d3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140115192806
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=994659cbc145&tochange=36e4fcbd07d3

Regressed by:
	36e4fcbd07d3	Alex Henrie — Bug 157846. Main patch: Propagate text <input> padding down to its scrolled child and add 1px left/right padding to stylesheets as needed. Fix tests. r=roc Original patch by Charly Molter. Help from Robert O'Callahan and Jim Mathies.

Comment 15

[Yaron]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #13)
> (In reply to comment #12)
> > Thank you Ehsan.
> > 
> > I'm not a pro. :)
> > Where is the file located in omni.ja?
> > Should I compare the relevant part with the file of FF 28 and change
> > accordingly?
> 
> Oh, this file is part of the C++ code that gets compiled into native binary
> code located somewhere in xul.dll.  There is no easy way to patch this up on
> your side.
> 
> > Anyway, I don't know how to create a patch. Would you do that?
> 
> I can write a patch once I know what the right fix is...  Currently waiting
> on the answer to comment 9.  Please be patient.  :-)
> 
> Thanks!

Thanks for the explanation. I appreciate it.

Comment 16

[(no longer active)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #9)
> (In reply to Simon Montagu :smontagu from comment #8)
> > I suspect that what is happening here is that the positioning of the caret,
> > or the size of the padding in the input field, has changed, with the result
> > that when the directional pointer points left it is hanging outside the
> > visible area of the input field.
> 
> That would be surprising to me.  I thought that this basically paints the
> caret on top of whatever is already there:
> <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCaret.cpp#527>.
> CCing roc and Matt to verify my claim.

If my understanding is not correct, given comment 14, is the right fix here adjusting the paddings in the theme?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If it only happens in the URL bar, yes. If it happens on all text inputs that obviously won't work.

Comment 18

[:Gijs (out for now; he/him)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (PTO April 25 to May 3) from comment #17)
> If it only happens in the URL bar, yes. If it happens on all text inputs
> that obviously won't work.

Yaron, does this happen in input boxes on webpages or just the search bar and url bar?

Comment 19

[Yaron]

On web pages the behavior is OK.
The problem exists in the search bar, find bar and url bar.

Comment 20

[Dão Gottwald [:dao]]

(In reply to Yaron from comment #19)
> On web pages the behavior is OK.
> The problem exists in the search bar, find bar and url bar.

The find bar doesn't live in browser/.

What's supposed to happen here in terms of CSS adjustments?

Comment 21

[(no longer active)]

(In reply to comment #20)
> (In reply to Yaron from comment #19)
> > On web pages the behavior is OK.
> > The problem exists in the search bar, find bar and url bar.
> 
> The find bar doesn't live in browser/.

Hmm, this could actually be a bug with <xul:textbox> styling...

> What's supposed to happen here in terms of CSS adjustments?

As part of bug 157846, the anonymous div under input elements now inherits the padding property, and I think the change in the padding is what has regressed this, so my best guess would be that a change to he padding-start/end is what is needed here.

Comment 22

[Yaron]

Hi Ehsan,

If you have some concrete CSS code, I'd be glad to test it.

Comment 23

[(no longer active)]

(In reply to comment #22)
> Hi Ehsan,
> 
> If you have some concrete CSS code, I'd be glad to test it.

I don't sorry.  I'm not the best person to fix this bug.

Comment 24

[Yaron]

Thanks for trying. I appreciate it.

Comment 25

[:Gijs (out for now; he/him)]

I asked this in bug 1007065 as well, but I wonder if the two are related, and particularly, if bug 966992 might have something to do with this as well.

Comment 26

[Yaron]

Thank you Gijs.
Don't give up. :)

Comment 27

[(no longer active)]

Yaron, do you mind testing this on tonight's Nightly build please?

Comment 28

[Yaron]

With pleasure.
Are Nightly builds localized?
Could you please send me a link?

Comment 29

[:Gijs (out for now; he/him)]

(In reply to Yaron from comment #28)
> With pleasure.
> Are Nightly builds localized?

I don't believe so.

> Could you please send me a link?

In English, this would be: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/05/2014-05-12-03-02-02-mozilla-central/firefox-32.0a1.en-US.win32.zip

You can use this add-on: https://addons.mozilla.org/firefox/addon/force-rtl/

to make the en-US build pretend English is an RTL language (I'm not sure this is necessary in order to check if this bug is fixed)

Comment 30

[Yaron]

Hello gentlemen,

Unfortunately the bug is not fixed in the latest Nightly.
Without the 'Force RTL' add-on, the cursor didn't have the hook indicator at all in the URL bar.
I could still test it in the about:config search bar and in the add-on manager search bar (not fixed).

With the 'Force RTL' add-on, the hook indicator appeared in the URL bar as well (same result).

Comment 31

[:Gijs (out for now; he/him)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> If it only happens in the URL bar, yes. If it happens on all text inputs
> that obviously won't work.

It seems to happen in all XUL text boxes (which have their inputs set to 0 padding because of styling in textbox.css), and I can also reproduce on HTML inputs with 0 padding. For instance, on https://www.google.co.uk, at least in the version google serves me, the input box "seems" to have padding, but that's just a <div> wrapped in a <td> wrapped around it (ugh) and the actual <input> has padding and margin set to 0 with border: none, and so for the first character you type, the left-pointing horizontal indicator of RTL input is not visible (presumably clipped by the <input> box). Changing the margin has no effect. Changing the padding does help make the indicator visible.

Robert, does that mean that we should investigate fixing this in core rather than toolkit's textbox styling after all? Or is setting padding to 0 on a textbox just a "don't do that" for web devs? :-)

Comment 32

[:Gijs (out for now; he/him)]

(I'll also note that the default input box horizontal padding of 1px shows part of the indicator, but not all of it, as far as I can tell, as the indicator seems to be 2px wide, at least on my windows 7 machine)

Comment 33

[Alex Henrie]

Attachment: Increased address bar padding demonstration

(In reply to :Gijs Kruitbosch from comment #31)
> It seems to happen in all XUL text boxes (which have their inputs set to 0
> padding because of styling in textbox.css), and I can also reproduce on HTML
> inputs with 0 padding.
> 
> Robert, does that mean that we should investigate fixing this in core rather
> than toolkit's textbox styling after all? Or is setting padding to 0 on a
> textbox just a "don't do that" for web devs? :-)

(In reply to :Gijs Kruitbosch from comment #32)
> (I'll also note that the default input box horizontal padding of 1px shows
> part of the indicator, but not all of it, as far as I can tell, as the
> indicator seems to be 2px wide, at least on my windows 7 machine)

To answer your questions:

1. Yes, setting textboxes to 0 padding is generally a no-no for Web developers, but it is permitted for rare needs and compatibility with other web browsers.

2. I do not speak any RTL languages, but when the cursor is near the edge, I think it's OK that Firefox shows only 1px of the 2px hook. The letters you type often obscure part of the hook anyway.

3. The problem with the XUL textboxes in the Firefox UI cannot be solved by simply increasing their padding. I tried increasing the URL bar's padding from 1px to 20px (patch attached) and the padding was clearly increased but the cursor hook was still not visible. I think the favicon is obscuring it.

Robert, you are of course welcome to add your opinions/insights as well :)

Comment 34

[:Gijs (out for now; he/him)]

(In reply to Alex Henrie from comment #33)
> Created attachment 8423579 [details] [diff] [review]
> 
> 3. The problem with the XUL textboxes in the Firefox UI cannot be solved by
> simply increasing their padding. I tried increasing the URL bar's padding
> from 1px to 20px (patch attached) and the padding was clearly increased but
> the cursor hook was still not visible. I think the favicon is obscuring it.

No, your patch increases the textbox padding, not the <input> one. XUL textboxes have an anonymous <input> child each. You can inspect this with the Dom Inspector add-on, if you toggle anonymous content. Adding padding to the anonymous child works (although you need !important because the 0 padding rule has that, too).

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One option is to draw the caret one pixel to the right if the RTL hook would otherwise be clipped by a containing scrollframe. How does that sound?

Comment 36

[Alex Henrie]

Attachment: Restore pre-FF29 XUL textbox padding.

(In reply to :Gijs Kruitbosch from comment #34)
> No, your patch increases the textbox padding, not the <input> one. XUL
> textboxes have an anonymous <input> child each. You can inspect this with
> the Dom Inspector add-on, if you toggle anonymous content. Adding padding to
> the anonymous child works (although you need !important because the 0
> padding rule has that, too).

In that case, my fix for bug 157846 (https://hg.mozilla.org/mozilla-central/rev/36e4fcbd07d3) was incorrect because it put <input>'s previously irremovable 1px left/right padding on the <textbox> instead of its <input>. Fixing that fixes this bug.

Robert, could you look over this patch's Mac test failures? It's hard to tell whether the test failures are something I introduced or not: https://tbpl.mozilla.org/?tree=Try&rev=ab82e96cd2a6

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> One option is to draw the caret one pixel to the right if the RTL hook would
> otherwise be clipped by a containing scrollframe. How does that sound?

That's not a bad idea, but I consider it a separate issue. Here let's just give <textbox>'s anonymous <input> child its 1px left/right padding back.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Alex Henrie from comment #36)
> Robert, could you look over this patch's Mac test failures? It's hard to
> tell whether the test failures are something I introduced or not:
> https://tbpl.mozilla.org/?tree=Try&rev=ab82e96cd2a6

I'm pretty sure they're due to this patch. You can see that the text has moved one pixel to the right. Don't you need to fix autocomplete padding in Mac autocomplete.css?

Comment 38

[Yaron]

1) The hook indicator width is 2 px on my system.
2) See attached images: 
   - in the first image (typed in the URL bar) the cursor is over and hiding the left part of the letter "c". 
   - in the second image (typed in a page text box) the cursor is one pixel to the left and "c" is seen properly.

Use an icon editor for further tests.

http://s22.postimg.org/6p724t9y5/image.png
http://s22.postimg.org/hq6snkrkt/image.png

Comment 39

[Alex Henrie]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> Don't you need to fix autocomplete padding in Mac autocomplete.css?

Mac's autocomplete.css already sets the textbox padding to 0. I'm cc'ing Ryan of bug 1011184 to see if he has any ideas.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

I just filed the bug on the intermittent failure that was occurring in automation. Not sure what I can do to help here...

Comment 41

[Alex Henrie]

Can you tell if the failures on https://tbpl.mozilla.org/?tree=Try&rev=ab82e96cd2a6 are also part of bug 1011184 or if they are a separate issue?

Also, if anyone knows of a Mac-loving Mozillan who could help debug this, that would be great.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Looks similar to me (albeit perma-failing in your Try run). Also, bug 1011184 happened on WinXP vs. OSX.

Comment 43

[Lukas Blakk [:lsblakk] use ?needinfo]

Not seeing a lot of dupes or input that suggests this is widely impacting users so I think we can untrack this but consider a low-risk uplift nomination when one is ready, and we can take it to branches as timing in the cycle allows.  At this point in FF30 we are too close to shipping so it's likely a wontfix there unless a backout was a possibility (I'm guessing it's not because of the volume of work in bug 157846).

Comment 44

[Alex Henrie]

Attachment: Restore pre-FF29 XUL textbox padding (try 2)

When Robert took over my patch for bug 157846, he added these lines to editor/reftests/xul/input.css:

#mac html|input.ac {
  padding: 0 3px !important;
}

I'm pretty sure this was just a hack to get the tests to pass on Mac. We weren't intending to give Mac autocomplete different padding than Linux autocomplete, and with this new patch, Mac and Linux will have the same padding again.

Anyway, the bottom line is that the cursor hook issue can now be fixed with all tests passing: https://tbpl.mozilla.org/?tree=Try&rev=13f049b3da4f

Comment 45

[Alex Henrie]

Requesting checkin of attachment 8428294 [details] [diff] [review]

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/f17a26ae2dab

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f17a26ae2dab

Comment 48

[Yaron]

https://bugzilla.mozilla.org/show_bug.cgi?id=1083833