problems when highlighting part of a title in url bar autocomplete results when the title is a RTL value specifically, here's the problem: visit a page like http://www.ynet.co.il, and then try to autocomplete on a hebrew word in the title. because of the way the patch in bug #399664 the parts of the title will be out of order. take the example where the title is "this is a test." and the user types "is a" in the url bar. we'd up with: <scrollbox> <label value="this "/> <label style="font-weight: bold; value="is a"/> <label value=" test."/> </scrollbox> looking like: "this *is a* test." this works for LTR, but if the title was in a RTL language, the pieces would appear out of order, looking like: " test.*is a*this " In a previous attempt to get this working for RTL, I set the direction of the title scrollbox to match the chrome direction, which puts the labels in the right order, but they'd be out of order if the title was not in a RTL language. It's not the chrome direction that matters, it's the direction of the text that matters. Note, nsTreeBodyFrame::CheckTextForBidi() has code to detect if a string has any bidi characters. Other issues I ran into while trying to make #399664 safe for RTL titles which are still open: 1) making sure the correct ellipsis was shown 2) ensuring the "beginning" of the title was visible.

Does using <description> with some child bolding element work here? I'm assuming that rtl is handled in this case.

> Does using <description> with some child bolding element work here? I'm > assuming that rtl is handled in this case. that appears to work. I'll continue to investigate to make sure that I can scroll to the bolding element.

description works and solves my rtl problem, thanks for the suggestion Neil. there may be a slight performance hit the way I'm currently doing it, though. I'll attach a new patch to bug #399664.

> 1) making sure the correct ellipsis was shown > 2) ensuring the "beginning" of the title was visible. there is only one ellipsis now and we don't scroll, so those issues are currently invalid. Simon or Mano, do you guys see any RTL problems with the following screen shots: https://bugzilla.mozilla.org/attachment.cgi?id=290615 https://bugzilla.mozilla.org/attachment.cgi?id=290616 Marking invalid, but will reopen or log new bugs if the fix in bug #399664 has RTL issues.

Both the screen shots look fine to me.

There are still issues when the highlighted part is at a directional change boundary (which includes the beginning and end of an all-RTL title in LTR interface). This was reported on the Mozilla Israel forum at http://mozilla.org.il/board/viewtopic.php?p=27236#27236 (Hebrew)

thanks for reopening this bug, simon. our richlist items currently have the format: <description>abc<label value="def"/>ghi</description> Previously, neil@httl.net suggested (see bug #399664 comment #96) that span would be better than labels. <description>abc<html:span>def</html:span>ghi</description> I tried this, and it was noticeably slower, but since then vlad has made some suggestions that might help. I'll report back with the bugs for vlad's suggestions. As far as this RTL issue, using the live xul editor, <description style="direction: rtl">abc<html:span>def</html:span>ghi</description> will work, but: <description style="direction: rtl">abc<label value="def"/>ghi</description> does not seem to work. (Note, I'm using hebrew and not abc,def,ghi when testing.)

Neil and I talked this over on IRC last night and came to the conclusion that using html:span is the only way to guarantee correct ordering in bidi titles, including the case where the highlighted portion needs to be visually non-contiguous. Example: visit http://www.halonot2000.co.il/ and then enter "חלונות 2" in the location bar.

Hmm, is it possible to only use span if there's bidi chunks? Not sure if there's an easy way to detect that case...

We can always scan the text testing for UCS2_CHAR_IS_BIDI. That will have its own performance hit, of course. Fixing bug 407861 might also depend on using span (the example there happens to be RTL, but LTR complex scripts have the same bug).

I think what we want to do is: 1) switch from label to html:span 2) in the autocomplete C++ code, call UCS2_CHAR_IS_BIDI (notice, the existing tree code does this, too: http://lxr.mozilla.org/seamonkey/search?string=UCS2_CHAR_IS_BIDI) and if the title contains bidi we need to pass that through, so that we can set direction style on the "title" description to be rtl or ltr. my understanding from using the live xul editor is that in addition to using html:span (instead of label), we need to set the direction style.

(In reply to comment #12) > if the > title contains bidi we need to pass that through, so that we can set direction > style on the "title" description to be rtl or ltr. Do you mean always set direction to rtl if the title contains bidi? I don't think that's right. If we don't know the direction of the original page, the best we can do is use rule P1-P3 from the Unicode Bidi Algorithm, i.e. determine the direction by the first character in the title with strong directionality. I have code for that somewhere... http://www.unicode.org/reports/tr9/#The_Paragraph_Level

fix to use span instead of label, and to remove the scrollboxes (per vlad) from testing ה on http://mozilla.org.il/board/login.php does the right thing, so doing BIDI detection (in C++) and setting the text direction style on the description does not appear to be necessary. I'm sure there is a good reason, perhaps because description / span already does the right thing?

> Do you mean always set direction to rtl if the title contains bidi? that is what I meant, but it doesn't appear to be necessary (or correct, from your comment.) See the latest screen shot of my build running with the patch attached that uses <html:span> instead of <label>

Comment on attachment 292722 [details] [diff] [review] updated patch includes the fix for this bug, as well as fixes for bug #402118, bug #408024, and bug #407984

oops, I meant: bug #407951, bug #408024, and bug #407984

Comment on attachment 292722 [details] [diff] [review] updated patch note, using https://bugzilla.mozilla.org/attachment.cgi?id=292723 to measure it, this patch is a performance win

What do the l10n folk think is the right solution for the directionality of the title in localized builds in RTL languages? Having it follow the direction of the chrome seems the obvious answer at first, but since the URI has to remain LTR, having the title and URI in opposite directions may look a little strange.

Comment on attachment 292722 [details] [diff] [review] updated patch >- <xul:scrollbox anonid="title-scrollbox" class="ac-title"> >- <xul:scrollbox anonid="url-scrollbox" class="ac-url"> > .autocomplete-richlistbox > scrollbox { > overflow-x: hidden !important; > } ???

Comment on attachment 292722 [details] [diff] [review] updated patch >Index: toolkit/content/widgets/autocomplete.xml > <method name="_setUpDescription"> >- <parameter name="aScrollBoxObject"/> >+ <parameter name="aBoxObject"/> >+ setTimeout(function(self) { self._setUpEllipsis(aBoxObject.boxObject, aDescriptionElement.boxObject, aEllipsis); }, 0, this); "aBoxObject.boxObject" kind of looks wrong... sounds like you should pass in .boxObject directly in the _setUpDescription callers to avoid having to do this here. Great work on digging into improving perf here!

(Good catch by Neil, in comment 23 too: that style rule isn't needed anymore.)

> What do the l10n folk think is the right solution for the directionality of the > title in localized builds in RTL languages? Simon, what did firefox 2 do? From my tests, we seem to do what firefox 2 did, which is to have the url and title be in LTR, no matter what the chrome direction is. If you think there is still an issue, can you make it a spin off bug?

neil / gavin: > .autocomplete-richlistbox > scrollbox { > overflow-x: hidden !important; > } That rule is still needed to prevent a horizontal scrollbar from appearing. I'll attach a screen shot. Note, I replaced the scrollbox (with an hbox) in the content of the richlistitems, not the richlistbox.

Oh, I misread that as s/box/item/. Sorry.

carrying forward gavin's r+

fixed. Checking in toolkit/content/widgets/autocomplete.xml; /cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v <-- autocomplete.x ml new revision: 1.100; previous revision: 1.99 done Checking in toolkit/themes/pinstripe/global/autocomplete.css; /cvsroot/mozilla/toolkit/themes/pinstripe/global/autocomplete.css,v <-- autoco mplete.css new revision: 1.15; previous revision: 1.14 done Checking in toolkit/themes/winstripe/global/autocomplete.css; /cvsroot/mozilla/toolkit/themes/winstripe/global/autocomplete.css,v <-- autoco mplete.css new revision: 1.23; previous revision: 1.22 done

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).