416013 - [rtl] bidi UI switching should not affect the display of the emptyText

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Dão Gottwald [:dao]]

basically bug 342007, but for toolkit.

Comment 1

[Dão Gottwald [:dao]]

Mano, is this only about the alignment of the text?

Comment 2

[Asaf Romano (gone)]

No, it's about both direction and alignment.

Comment 3

[Asaf Romano (gone)]

That said, except for something like "חפש ב-amazon" (Search In Amazon), direction & alignment are identical testing-wise.

Comment 4

[Dão Gottwald [:dao]]

Attachment: first try

Ok, messing with the style property is ugly... Anyway, I'd like to know: Does this even work for you?

Comment 5

[Asaf Romano (gone)]

er, what happend to style.direction?

Comment 6

[Asaf Romano (gone)]

This ought to be done using a class IMO.

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #5)
> er, what happend to style.direction?

I'm using the unicode characters to force the direction... that's the idea at least.

(In reply to comment #6)
> This ought to be done using a class IMO.

Currently we don't have the direction information in the stylesheet, so we would still have to use getComputedStyle and set a "rtl" / "ltr" class, right?

Comment 8

[Asaf Romano (gone)]

cannot you set chromedir on the inputfield?

Comment 9

[Asaf Romano (gone)]

As in, there's not much point in all this getComputedStyle mess, you should use the chromedir attribute.

Comment 10

[Dão Gottwald [:dao]]

Adding chromedir to the bindings <content> means that every binding that extends textbox with its own <content> needs to be updated.

Comment 11

[Asaf Romano (gone)]

I'm fine with that.

Comment 12

[Dão Gottwald [:dao]]

We don't control all bindings that extend textbox. Since this bug is only discoverable with bidi UI support turned on, third-party widgets that got emptyText for free will remain broken here. That's hardly better than the getComputedStyle mess. It should be possible to combine both approaches.

Comment 13

[Asaf Romano (gone)]

Bindings which don't care about this (as in, don't ship localized RTL emptyText) won't update this, I'm fine with too. ;)

Comment 14

[Dão Gottwald [:dao]]

You can ship localized RTL emptyText without knowing about this issue.

Comment 15

[Asaf Romano (gone)]

Fine, document that on MDC: if you're setting empty text for a customized text field and ship he/ar localization in your extension, you should add the chromedir attribute.

Comment 16

[Dão Gottwald [:dao]]

no need to document it if we can make it work out of the box.

Comment 17

[Dão Gottwald [:dao]]

Attachment: patch

This should work. I didn't get it to work with the direction CSS property instead of the left-to-right / right-to-left override.

Comment 18

[Asaf Romano (gone)]

as i told you, this isn't about alignment, so you need to set here both direction and alignment.

Comment 19

[Dão Gottwald [:dao]]

That's why the left-to-right / right-to-left overrides are there. Testing with "חפש ב-amazon", this gives me a consistent result. Otherwise I get "amazon-..." after switching the textbox direction (shift+ctrl+x).

Comment 20

[Asaf Romano (gone)]

Why do you use getComputedStyle, there's this global.dtd entry....

Comment 21

[Asaf Romano (gone)]

and i don't want to make it work it out of the box if we can document this very special case, getComputedStyle is expensive.

Comment 22

[Asaf Romano (gone)]

Comment on attachment 301801 [details] [diff] [review]
patch

So, thinking about this more, i prefer the first patch then if we cannot get this to work without js :(

Comment 23

[Asaf Romano (gone)]

Comment on attachment 301790 [details] [diff] [review]
first try

I guess this will do for now.

Comment 24

[Dão Gottwald [:dao]]

Hrm, the first one was really just a proof-of-concept. Don't you think the chrome direction should be cached at least? As you say, getComputedStyle is expensive.

Comment 25

[Dão Gottwald [:dao]]

Attachment: patch v2

this also handles |textbox { text-align: center }| gracefully.

Comment 26

[Dão Gottwald [:dao]]

Btw, we don't support text-align:end yet. It's just in there for future compatibility.

Comment 27

[Asaf Romano (gone)]

There's more harm in caching this IMO.

Comment 28

[Asaf Romano (gone)]

I would care about the expensiveness of getComputedStyle if we did this for every textbox within the chrome (or if we did this every other minute), but this is just for focus/blur events of textboxes with the emptyText property set.

Comment 29

[Dão Gottwald [:dao]]

What about textboxes that have an explicit text-align set? (left/right/center)

Comment 30

[Dão Gottwald [:dao]]

Attachment: patch v2

updated test

Comment 31

[(no longer active)]

This is wanted-next+.  Any chance of this making 1.9.1?

Comment 32

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 33

[Neil Deakin]

It looks OK, but wouldn't mano be a better reviewer here?

Comment 34

[(no longer active)]

With this patch, will the textbox's with explicit direction styles be set correctly?

For example, what happens when an RTL app (with locale.dir=="rtl") sets a <textbox> to LTR via CSS?

Comment 35

[Dão Gottwald [:dao]]

Depends on the CSS. As for bug 467251: the patch should work for the location bar.

Comment 36

[Dão Gottwald [:dao]]

Enn: ping?
(As said on irc, I think mano is busy with other things.)

Comment 37

[Neil Deakin]

I already implied that I don't think I'm a suitable reviewer for this.

Comment 38

[Dão Gottwald [:dao]]

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

Mano, can you get to this before the next beta code freeze?

Comment 39

[(no longer active)]

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

I haven't tested the patch myself, but the code looks fine.

I have one question though.  What happens if you don't specify the alignments?  I think the direction rule should take care of that as well, right?

Comment 40

[Dão Gottwald [:dao]]

(In reply to comment #39)
> I have one question though.  What happens if you don't specify the alignments? 
> I think the direction rule should take care of that as well, right?

It doesn't... tested in the search bar and the location bar.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

I guess I don't really understand this bug or bug 342007. Why do we need to treat the emptyText differently than normal textbox values?

Comment 42

[Dão Gottwald [:dao]]

There shouldn't be a difference by default. But you can switch the direction of a textbox with accel+shift+x in order to write LTR in an RTL setting or vice versa.

Comment 43

[:Gavin Sharp [email: [e-mail]]]

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

Ah, ok, so we need this to ensure that the emptyText is always in the chrome direction, even if the textbox is switched.

I guess the idea is that these styles take precedence over the styling triggered by the "dir" attribute set in nsEditor::SwitchTextDirection?

Comment 44

[Dão Gottwald [:dao]]

(In reply to comment #43)
> I guess the idea is that these styles take precedence over the styling
> triggered by the "dir" attribute set in nsEditor::SwitchTextDirection?

Yes, that's an assumption we make.

Comment 45

[Dão Gottwald [:dao]]

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

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090215 Minefield/3.2a1pre ID:20090215020424

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090215 Minefield/3.2a1pre ID:20090215124130

Comment 47

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner

Comment 48

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3bd25b099b5b

Comment 49

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified on 1.9.1 with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090221 Shiretoko/3.1b3pre ID:20090221020541

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090221 Shiretoko/3.1b3pre ID:20090221033633

Tracy, would you mind adding a new testgroup/subgroup for RTL tests? We have a couple of tests which could be added.