Closed Bug 416013 Opened 12 years ago Closed 11 years ago

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

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: dao, Assigned: dao)

References

()

Details

(Keywords: intl, rtl, verified1.9.1)

Attachments

(1 file, 4 obsolete files)

basically bug 342007, but for toolkit.
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9beta4
Summary: emptyText should be displayed in the chrome direction → emptyText should always be displayed in the chrome direction
Mano, is this only about the alignment of the text?
No, it's about both direction and alignment.
That said, except for something like "חפש ב-amazon" (Search In Amazon), direction & alignment are identical testing-wise.
Attached patch first try (obsolete) — Splinter Review
Ok, messing with the style property is ugly... Anyway, I'd like to know: Does this even work for you?
er, what happend to style.direction?
This ought to be done using a class IMO.
(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?
cannot you set chromedir on the inputfield?
As in, there's not much point in all this getComputedStyle mess, you should use the chromedir attribute.
Adding chromedir to the bindings <content> means that every binding that extends textbox with its own <content> needs to be updated.
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.
Bindings which don't care about this (as in, don't ship localized RTL emptyText) won't update this, I'm fine with too. ;)
You can ship localized RTL emptyText without knowing about this issue.
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.
no need to document it if we can make it work out of the box.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #301790 - Attachment is obsolete: true
as i told you, this isn't about alignment, so you need to set here both direction and alignment.
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).
Why do you use getComputedStyle, there's this global.dtd entry....
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 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 on attachment 301790 [details] [diff] [review]
first try

I guess this will do for now.
Attachment #301790 - Attachment is obsolete: false
Attachment #301790 - Flags: review+
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.
Attached patch patch v2 (obsolete) — Splinter Review
this also handles |textbox { text-align: center }| gracefully.
Attachment #301801 - Attachment is obsolete: true
Attachment #301900 - Flags: review?(mano)
Btw, we don't support text-align:end yet. It's just in there for future compatibility.
There's more harm in caching this IMO.
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.
What about textboxes that have an explicit text-align set? (left/right/center)
Summary: emptyText should always be displayed in the chrome direction → bidi UI switching should not affect the display of the emptyText
Attached patch patch v2 (obsolete) — Splinter Review
updated test
Attachment #301900 - Attachment is obsolete: true
Attachment #302919 - Flags: review?(mano)
Attachment #301900 - Flags: review?(mano)
Flags: blocking1.9? → blocking1.9+
Target Milestone: mozilla1.9beta4 → mozilla1.9
Flags: tracking1.9+ → wanted-next+
Attachment #302919 - Flags: review?(mano)
Assignee: dao → nobody
Target Milestone: mozilla1.9 → ---
This is wanted-next+.  Any chance of this making 1.9.1?
Blocks: 467251
Attached patch patch v3Splinter Review
Assignee: nobody → dao
Attachment #301790 - Attachment is obsolete: true
Attachment #302919 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #351009 - Flags: review?(enndeakin)
It looks OK, but wouldn't mano be a better reviewer here?
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?
Depends on the CSS. As for bug 467251: the patch should work for the location bar.
Enn: ping?
(As said on irc, I think mano is busy with other things.)
I already implied that I don't think I'm a suitable reviewer for this.
Attachment #351009 - Flags: review?(enndeakin) → review?(gavin.sharp)
Comment on attachment 351009 [details] [diff] [review]
patch v3

Mano, can you get to this before the next beta code freeze?
Attachment #351009 - Flags: review?(gavin.sharp) → review?(mano)
Attachment #351009 - Flags: review?(mano)
Attachment #351009 - Flags: review?(gavin.sharp)
Attachment #351009 - Flags: review?(ehsan.akhgari)
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?
Attachment #351009 - Flags: review?(ehsan.akhgari) → review+
(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.
Keywords: intl, rtl
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?
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 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?
Attachment #351009 - Flags: review?(gavin.sharp) → review+
(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.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/b5120305d873
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #351009 - Flags: approval1.9.1?
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Summary: bidi UI switching should not affect the display of the emptyText → [rtl] bidi UI switching should not affect the display of the emptyText
Attachment #351009 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
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.
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.