problems when highlighting part of a title in url bar autocomplete results when the title is a RTL value

RESOLVED FIXED in Firefox 3 beta3

Status

()

defect
P3
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: moco, Assigned: moco)

Tracking

(Blocks 1 bug, {rtl})

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

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.
Blocks: 399664
No longer depends on: 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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
Both the screen shots look fine to me.
Posted image Remaining issue
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)
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
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.)
Status: REOPENED → ASSIGNED
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M11
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...
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
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.
Assignee: nobody → sspitzer
Status: ASSIGNED → NEW
(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
Component: Places → Location Bar and Autocomplete
QA Contact: places → location.bar
Duplicate of this bug: 407980
Posted patch patch (obsolete) — Splinter Review
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>
Status: NEW → ASSIGNED
Posted patch updated patch (obsolete) — Splinter Review
Attachment #292714 - Attachment is obsolete: true
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
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
Attachment #292722 - Flags: review?(gavin.sharp)
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!
Attachment #292722 - Flags: review?(gavin.sharp) → review+
(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+
Attachment #292722 - Attachment is obsolete: true
Attachment #292794 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 409273
Blocks: fx35-l10n-fa
No longer blocks: Persian-Fx3.5
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
No longer blocks: fx35-l10n-fa
Blocks: Persian
You need to log in before you can comment on or make changes to this bug.