Closed Bug 407861 Opened 12 years ago Closed 12 years ago

Bolding the found text in autocomplete breaks ligatures

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta5

People

(Reporter: smontagu, Assigned: Mardak)

References

()

Details

(Keywords: intl)

Attachments

(12 files, 7 obsolete files)

13.39 KB, image/png
Details
2.22 KB, application/vnd.mozilla.xul+xml
Details
1.07 KB, application/vnd.mozilla.xul+xml
Details
66.37 KB, image/png
beltzner
: ui-review-
Details
17.89 KB, image/png
Details
17.32 KB, image/png
Details
17.60 KB, image/png
Details
14.46 KB, image/png
Details
14.32 KB, image/png
Details
14.45 KB, image/png
Details
4.08 KB, patch
Gavin
: review+
beltzner
: approval1.9b5+
Details | Diff | Splinter Review
14.68 KB, image/png
Details
In the screenshot the highlighted letter Qaf should connect to the surrounding letters: the first entry should appear as جريدة القاهرة, not جريدة ال‌ق‌اهرة.

The new text run code would handle this correctly if the highlighted text were styled with a color change instead of font-weight.
Summary: Highlighting found text in location bar breaks ligatures → Highlighting found text in autocomplete breaks ligatures
Summary: Highlighting found text in autocomplete breaks ligatures → Bolding the found text in autocomplete breaks ligatures
Blocks: 399664
Flags: blocking-firefox3?
Keywords: intl
Simon, if we switched from <label> to <html:span>, would this be fixed?

I think we're going to switch to html:span anyways, for the RTL issues.
Changing to html:span won't fix this without the change from font-weight to color. I don't know if changing to color will fix it without changing to html:span, but I suspect not.
I spoke with Pav about this - using just underline will work here. Italic will get us in similar problems as bold.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P1
> I spoke with Pav about this - using just underline will work here. Italic will get us in similar problems as > bold.

Mike, are you suggesting that we should stop bolding and just do underline?

Note, sayre suggested in bug #406254 that instead of bold we could use text color instead of bold for emphasis.


actually, I'm not 100% sure that we can't still do bold and html:span.

using ted's live xul editor (ted, you rule!), the problem, once again, is using label (which breaks ligatures) as opposed to html:span.

I'll attach some xul that should show that this will work, but I need Simon's expert eye to confirm.
actually, upon zooming in to get a better look, pav is right, bold breaks the ligature even if we use html:span.
Are there any languages where breaking ligatures actually changes the meaning of the text, or does it only change the aesthetically pleasing kerning?  Quickly scanning the wikipedia article, this appears to be the case: "The Arabic alphabet, historically a cursive derived from the Nabataean alphabet, most letters take a variant shape depending on which they are followed (word-initial), preceded (word-final) or both (medial) by other letters." http://en.wikipedia.org/wiki/Typographical_ligature#Non-Latin_alphabets

Also, is there anyway for us to fall back to just underlining only in situations where we are displaying characters from a language that requires ligatures? (similar to how my carat in this text field is picking up a tick mark on the right?)
Attached image proposal (obsolete) —
This is based on attachment 294887 [details] and wouldn't break ligatures.
Attachment #295491 - Flags: ui-review?(beltzner)
In reply to comment #9
Yes, in Arabic many letters take up a "tail" or "flourish" when they are word-final (i.e. in the final or isolate presentation form). Replacing two joined letters of the same word by a word-final + word initial shape would mean introducing a word break, which may or may not give a "meaningful" result, but certainly not the "intended" meaning.

In addition, in Arabic the lettergroup lam + alif is mandatorily represented as a single glyph. Using (initial or medial lam) + (final alif) would be more than untediousness, it would be a fault. I'm not convinced that it would be possible to mark the boundary "in the middle of" such a glyph in any way -- now the Arabic article "al" is written as a single word with what follows it, so that searching for a word starting in alif would, if it were preceded by the article, require exactly such an "impossible" boundary.
The previous testcase didn't remove the margin/padding of the label, so the comparision was not really fair. Also added a hbox of <description> to see if ligatures survive that. (they should...)
Attachment #292674 - Attachment is obsolete: true
I think the testcases show that underlining instead of bolding isn't a great solution for Arabic, because many glyphs go down below the baseline and the underline tends to make the text significantly less readable :(
(In reply to comment #13)
> underline tends to make the text significantly less readable :(

We highlight what you've just typed, so this shouldn't be a big deal. The *context* of the text is interesting.
<html:span> is a done deal (bug 402118) so IMO the only question is between underline and color. (text-decoration: blink also works well)
Target Milestone: --- → Firefox 3 beta4
All of the above suck for various reasons:

Bolding breaks ligatures.
Underlining hurts readability in some locales
Text color, other than selection-like highlight, is not sec508 compliant, since high-contrast users, etc, won't be able to distinguish much.
Attached image updated proposal
With highlight colors and without underline, as already proposed a week ago via e-mail.
Attachment #295491 - Attachment is obsolete: true
Attachment #304080 - Flags: ui-review?(beltzner)
Attachment #295491 - Flags: ui-review?(beltzner)
(In reply to comment #14)
> (In reply to comment #13)
> > underline tends to make the text significantly less readable :(
> We highlight what you've just typed, so this shouldn't be a big deal. The
> *context* of the text is interesting.
With bug 415403, we would show different terms being highlighted, so it could be useful to see what is being matched where.

So font-color and background-color should work perhaps with a border with the same color as the highlight and negative margin so that the highlighted area is just a little bit bigger than the text it selects.
The attachment in comment 17 is ugly.  It's hard to read text when it jumps from dark-on-light to light-on-dark.  A yellow highlight would be better.
I've used this for several weeks without realizing that this is hard to read. That's because it's actually not hard to read and/or because I spot but don't read the highlighted text, since I just typed it.

(In reply to comment #18)
> So font-color and background-color should work perhaps with a border with the
> same color as the highlight and negative margin so that the highlighted area is
> just a little bit bigger than the text it selects.

Border would break ligatures, outline with or without an offset would overlap the highlighted or the adjacent text.
I think we should be open to the possibility of using a different indicator depending on language.  (For example, Google uses bold for English but red for Chinese.)  Maybe the best thing to do is bold+underline for English but underline only for Arabic.
(In reply to comment #19)
> It's hard to read text when it jumps from dark-on-light to light-on-dark.

Except that's what you get when you select text in a text box or even any text on this page, when you cursor down any menu or tree (e.g. in the bookmarks sidebar) or when using autoFill - IOW: whenever something is highlighted. So: why do something different only in _this_ case?
Because this isn't a selection, and the immediate context is more important.
Keywords: uiwanted
Whiteboard: [needs review mconnor/beltzner]
Need to make a decision on this today.
Assignee: nobody → faaborg
(In reply to comment #21)
> I think we should be open to the possibility of using a different indicator
> depending on language.  (For example, Google uses bold for English but red for
> Chinese.)  Maybe the best thing to do is bold+underline for English but
> underline only for Arabic.

Do we have an easy way to do this?
No. Plus, what would you do if the search term is half Arabic and half English?
I thought the idea was that when parsing the results to determine what section matches against the result (ie: what section to apply the .ac-emphasize-text rule to) we could also determine if the character set was one that required something like .ac-emphasize-text-intl.

I also notice that we have an intl.css; is this something we can just leave to localizers?
I guess JS could look for Arabic characters (or characters that need similar treatment) in the search terms and mark matches with something like .ac-emphasize-text-alt. That would use a highlight like in https://bugzilla.mozilla.org/attachment.cgi?id=304080, which is ugly but probably our only option (can't use underline, can't use bolding or any other font change, can't use text color). Otherwise we'd use bold+underline like today.
Target Milestone: Firefox 3 beta4 → Firefox 3
Note that while I used generic Blue for the mockup, this color should be the system highlight color
highlight is a background color, not a text color. The text color would be highlightText, and it has to be used together with the highlight background. Otherwise you'll get text that's either invisible or not differentiable from normal text with random OS themes.
Comment on attachment 304080 [details]
updated proposal

icky :(
Attachment #304080 - Flags: ui-review?(beltzner) → ui-review-
I'm still waiting for someone to tell me if we can handle this in intl.css - IMO it's not worth making 99% of the locales look worse because we break things in 1% of the locales.

Also, it's unclear to me if what we're doing is making the text look worse, or actually totally breaking the rendering of the text.
(In reply to comment #32)
 
> I'm still waiting for someone to tell me if we can handle this in intl.css -
> IMO it's not worth making 99% of the locales look worse because we break things
> in 1% of the locales.
>

For the record 7.5% of the locales released in Beta4 have this problem (Arabic, Gujarati and Punjabi), but anyway locales and intl.css are pretty much orthogonal to this -- you don't need to be in an Arabic locale to surf pages with Arabic titles.

> Also, it's unclear to me if what we're doing is making the text look worse, or
> actually totally breaking the rendering of the text.
> 

Breaking it. See comment 11.
Is there a good list of what character sets this breaks? I've got Mardak looking into the solution posited in comment 28, where if we detect any such characters in the search string we use .ac-emphasize-text-alt which can fall back to something like underline-only.
Attached image screenshot of v1
Using 2 types of emphasis.
Attached patch v1 (obsolete) — Splinter Review
Detect when certain emphasis text areas need to be alternately emphasized. It's possible for a title/url/tag to have different combinations of emphasis because only the part that needs alternate emphasis gets the alternate.
Comment on attachment 310266 [details] [diff] [review]
v1

>+  color: red;

That's going to be another possible vector for bug 409974.
I know. ;) I was going to do something crazy like color: red; background-color: green; just to make sure people knew it wasn't the final color.
In reply to comment #35 and comment #37
Mardak: What happens when the highlight boundary is halfway a ligature? E.g., searching for اسلام "Islam" and expecting to find also الاسلام "al-Islam" (meaning: the Islam)
I didn't quite understand.. but I think you're asking something similar to

"What if you matched "<ligature><non-ligature>" (no spaces) when searching for "<ligature> <non-ligature>?" Both will be emphasized as non-ligature, e.g., all red.
FYI, it's not really matching <ligature> vs <non-ligature>. Right now it's just checking if the unicode value is in the arabic ranges. So it'll be alternately styled even if it didn't have ligatures.

What other ranges should we include for the alternate?
(In reply to comment #41)
> I didn't quite understand.. but I think you're asking something similar to
> 
> "What if you matched "<ligature><non-ligature>" (no spaces) when searching for
> "<ligature> <non-ligature>?" Both will be emphasized as non-ligature, e.g., all
> red.
> 

What I mean is, trying to match the alif part but not the laam part of a laam-alif digraph, as with the example I gave: when searching for اسلام (Islam) I expect to find also الاسلام (meaning "the Islam"). How will that be highlighted, considering that the match starts halfway the لا laam-alif digraph? Could you give a screenshot?
Is that related to this bug? What do we do right now on trunk? Unless "the" of "the Islam" is before/after "Islam", I don't think it would match.

Pretend it was english. Would "Islam" match "Isthelam"? (But I have no idea how the language works..)
That's interesting... ;)
Oh. It makes more sense now that I realize "the" is made up of two characters ا ل which happens to be the same starting character in islam. Seems like both the letter pairs ال and لا appear in "the islam" except the latter has a special ligature.
Exactly. "The" of "the Islam" is before (right of, in this RTL language) "Islam", BUT:

1. The article "al" is always written together with its substantive (which it precedes), with no intervening space, as if they were one word;
2. Whenever the letter laam (including the ending -l of the article) is followed by the letter alif (including the first letter of any word beginning in a vowel) with no intervening space, both MUST be written as a digraph.

In the V-like لا sign near the beginning (right end) of الاسلام , the curved line on the right and bottom is the laam (l of the article al- "the"), and the oblique line on the left is the alif (i of "Islam"). When "Islam" is written اسلام without the article, that same initial alif of "Islam" is the vertical letter at far right; while in الاسلام (al-Islam) the initial vertical at far right of the "article+substantive" is the initial alif representing the a- of the "al" article.

And thanks for that screenshot. :-)
(In reply to comment #42)
> What other ranges should we include for the alternate?

As an opening bid I would say just include U+0600 through U+109F (Arabic, Syriac and all the Indic languages) The Arabic Presentation Forms blocks at U+FB50 and U+FE70 actually don't need to be included, because they are precomposed ligatures and connecting forms.

Attached patch v1.1 (obsolete) — Splinter Review
(In reply to comment #48)
> As an opening bid I would say just include U+0600 through U+109F (Arabic,
> Syriac and all the Indic languages)
Sounds good. Makes it simpler to check as well. :)

Keeping it as red for now, but it'll eventually probably depend on the url color (per platform).

gavin: This is right after bug 407946 in my stack (and before bug 415403).
Attachment #310266 - Attachment is obsolete: true
Attachment #310414 - Flags: review?(gavin.sharp)
Depends on: 407946
Whiteboard: [needs review mconnor/beltzner] → [has patch][need review gavin][need bug 407946]
Well, initially we also kept the green "for now", knowing that it would cause trouble. Months later, it's still not fixed. Now that we're even closer to the release, I would definitely reject another hard coded color ... but of course I'm neither a peer nor doing ui reviews.
Target Milestone: Firefox 3 → Firefox 3 beta5
Attached patch v1.2 (obsolete) — Splinter Review
No additional color.. just underline now.
Attachment #310414 - Attachment is obsolete: true
Attachment #310512 - Flags: review?(gavin.sharp)
Attachment #310414 - Flags: review?(gavin.sharp)
You can try out this build (apparently windows and linux only):

https://build.mozilla.org/tryserver-builds/2008-03-19_11:56-edward.lee@engineering.uiuc.edu-unescape.ligature.sleepwake/

It has the alternate emphasis as underline.

Bug 335418 - Pause downloads when computer is about to go to sleep
Bug 407946 - emphasize all matching text in title and url, not just the first match in title and url
** Bug 407861 - Bolding the found text in autocomplete breaks ligatures
Bug 415403 - Show matches for all search words for location bar autocomplete
Bug 418257 - Show what part of which tags match for urlbar autocomplete
Bug 392143 - show keywords as url bar autocomplete choices
Bug 249468 - Add all bookmark keywords to location bar autocomplete drop-down list
Bug 395161 - make it possible to restrict the url bar autocomplete results to bookmarks, tags, or history entries
Bug 407204 - adjust the title and url text sizes
Bug 406257 - reduce the number of rows in url bar autocomplete from 10 to 6
Bug 420437 - Search and emphasize quoted strings with spaces
Bug 414326 - Use DownloadUtils for software update downloads
Bug 422491 - Optimize AwesomeBar if it finished searching and found fewer than maxResults
Bug 422698 - different levels of URL decoding for address bar and autocomplete suggestions
Attached patch v1.3 (obsolete) — Splinter Review
Clean up the autocomplete/browser.css files so that autocomplete.css keeps the structural and browser.css gets the styling.

This build should correctly bold things on windows again:

https://build.mozilla.org/tryserver-builds/2008-03-19_22:48-edward.lee@engineering.uiuc.edu-nativeurl.clearcurrent/
Assignee: faaborg → edilee
Attachment #310512 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #310715 - Flags: review?(gavin.sharp)
Attachment #310512 - Flags: review?(gavin.sharp)
(In reply to comment #54)
> Clean up the autocomplete/browser.css files so that autocomplete.css keeps the
> structural and browser.css gets the styling.

fwiw, that distinction doesn't really exist ... both are theme files. It might actually be better to keep the styling in toolkit, so that a non-browser autocomplete consumer get an emphasis without having to add custom CSS. Or is there a reason for not emphasizing that stuff by default?
Attached patch v1.4Splinter Review
Move emphasis styling back into toolkit and cleaned up selectors.

html|*.ac-emphasize-text -> .ac-normal-text > html|span
Attachment #310715 - Attachment is obsolete: true
Attachment #310831 - Flags: review?(gavin.sharp)
Attachment #310715 - Flags: review?(gavin.sharp)
Attachment #310831 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][need review gavin][need bug 407946] → [has patch][need bug 407946]
stephend: "Create a bookmark with title 'الاسلام (The Islam)' (copy/paste) and url 'http://theislam/الاسلام'. Search for 'ل' (copy/paste) in the location bar. Make sure the result *doesn't* look like the left result in attachment 310832 [details]. Notably, the emphasized part should *not* look like a 'J'. Instead it should look like the right side in terms of it not emphasizing the match as 'J'."
Flags: in-litmus?
Comment on attachment 310831 [details] [diff] [review]
v1.4

a1.9b5? for P1 bug.

litmus test is described in comment #58.
Attachment #310831 - Flags: approval1.9b5?
Comment on attachment 310831 [details] [diff] [review]
v1.4

a=beltzner, yay!
Attachment #310831 - Flags: approval1.9b5? → approval1.9b5+
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.126; previous revision: 1.125
done
Checking in toolkit/themes/gnomestripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.21; previous revision: 1.20
done
Checking in toolkit/themes/pinstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.21; previous revision: 1.20
done
Checking in toolkit/themes/winstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.31; previous revision: 1.30
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: uiwanted
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [has patch][need bug 407946]
in-litmus+

https://litmus.mozilla.org/show_test.cgi?id=5212
Flags: in-litmus? → in-litmus+
Verified FIXED:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008032204 Minefield/3.0b5pre

 Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008032204 Minefield/3.0b5pre

-and-

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008032204 Minefield/3.0b5pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.