Closed
Bug 407861
Opened 17 years ago
Closed 17 years ago
Bolding the found text in autocomplete breaks ligatures
Categories
(Firefox :: Address Bar, defect, P1)
Firefox
Address Bar
Tracking
()
VERIFIED
FIXED
Firefox 3 beta5
People
(Reporter: smontagu, Assigned: Mardak)
References
()
Details
(Keywords: intl)
Attachments
(12 files, 7 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
application/vnd.mozilla.xul+xml
|
Details | |
(deleted),
application/vnd.mozilla.xul+xml
|
Details | |
(deleted),
image/png
|
beltzner
:
ui-review-
|
Details |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Gavin
:
review+
beltzner
:
approval1.9b5+
|
Details | Diff | Splinter Review |
(deleted),
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.
Updated•17 years ago
|
Summary: Highlighting found text in location bar breaks ligatures → Highlighting found text in autocomplete breaks ligatures
Updated•17 years ago
|
Summary: Highlighting found text in autocomplete breaks ligatures → Bolding the found text in autocomplete breaks ligatures
Comment 1•17 years ago
|
||
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.
Reporter | ||
Comment 2•17 years ago
|
||
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.
Comment 3•17 years ago
|
||
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
Comment 4•17 years ago
|
||
> 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.
Comment 5•17 years ago
|
||
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.
Updated•17 years ago
|
Comment 6•17 years ago
|
||
Comment 7•17 years ago
|
||
actually, upon zooming in to get a better look, pav is right, bold breaks the ligature even if we use html:span.
Updated•17 years ago
|
Attachment #292673 -
Attachment is obsolete: true
Comment 8•17 years ago
|
||
Comment 9•17 years ago
|
||
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?)
Comment 10•17 years ago
|
||
This is based on attachment 294887 [details] and wouldn't break ligatures.
Attachment #295491 -
Flags: ui-review?(beltzner)
Comment 11•17 years ago
|
||
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.
Comment 12•17 years ago
|
||
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
Reporter | ||
Comment 13•17 years ago
|
||
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 :(
Comment 14•17 years ago
|
||
(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.
Reporter | ||
Comment 15•17 years ago
|
||
<html:span> is a done deal (bug 402118) so IMO the only question is between underline and color. (text-decoration: blink also works well)
Updated•17 years ago
|
Target Milestone: --- → Firefox 3 beta4
Comment 16•17 years ago
|
||
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.
Comment 17•17 years ago
|
||
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)
Assignee | ||
Comment 18•17 years ago
|
||
(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.
Comment 19•17 years ago
|
||
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.
Comment 20•17 years ago
|
||
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.
Comment 21•17 years ago
|
||
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.
Comment 22•17 years ago
|
||
(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?
Comment 23•17 years ago
|
||
Because this isn't a selection, and the immediate context is more important.
Comment 25•17 years ago
|
||
(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?
Comment 27•17 years ago
|
||
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.
Updated•17 years ago
|
Target Milestone: Firefox 3 beta4 → Firefox 3
Comment 29•17 years ago
|
||
Note that while I used generic Blue for the mockup, this color should be the system highlight color
Comment 30•17 years ago
|
||
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 31•17 years ago
|
||
Comment on attachment 304080 [details]
updated proposal
icky :(
Attachment #304080 -
Flags: ui-review?(beltzner) → ui-review-
Comment 32•17 years ago
|
||
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.
Reporter | ||
Comment 33•17 years ago
|
||
(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.
Comment 34•17 years ago
|
||
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.
Assignee | ||
Comment 35•17 years ago
|
||
Using 2 types of emphasis.
Assignee | ||
Comment 36•17 years ago
|
||
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.
Assignee | ||
Comment 37•17 years ago
|
||
Comment 38•17 years ago
|
||
Comment on attachment 310266 [details] [diff] [review]
v1
>+ color: red;
That's going to be another possible vector for bug 409974.
Assignee | ||
Comment 39•17 years ago
|
||
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.
Comment 40•17 years ago
|
||
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)
Assignee | ||
Comment 41•17 years ago
|
||
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.
Assignee | ||
Comment 42•17 years ago
|
||
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?
Comment 43•17 years ago
|
||
(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?
Assignee | ||
Comment 44•17 years ago
|
||
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..)
Assignee | ||
Comment 45•17 years ago
|
||
That's interesting... ;)
Assignee | ||
Comment 46•17 years ago
|
||
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.
Comment 47•17 years ago
|
||
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. :-)
Reporter | ||
Comment 48•17 years ago
|
||
(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.
Assignee | ||
Comment 49•17 years ago
|
||
(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)
Assignee | ||
Updated•17 years ago
|
Depends on: 407946
Whiteboard: [needs review mconnor/beltzner] → [has patch][need review gavin][need bug 407946]
Comment 50•17 years ago
|
||
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.
Updated•17 years ago
|
Target Milestone: Firefox 3 → Firefox 3 beta5
Assignee | ||
Comment 51•17 years ago
|
||
No additional color.. just underline now.
Attachment #310414 -
Attachment is obsolete: true
Attachment #310512 -
Flags: review?(gavin.sharp)
Attachment #310414 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 52•17 years ago
|
||
Assignee | ||
Comment 53•17 years ago
|
||
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
Assignee | ||
Comment 54•17 years ago
|
||
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)
Comment 55•17 years ago
|
||
(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?
Assignee | ||
Comment 56•17 years ago
|
||
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)
Updated•17 years ago
|
Attachment #310831 -
Flags: review?(gavin.sharp) → review+
Updated•17 years ago
|
Whiteboard: [has patch][need review gavin][need bug 407946] → [has patch][need bug 407946]
Assignee | ||
Comment 57•17 years ago
|
||
Assignee | ||
Comment 58•17 years ago
|
||
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?
Assignee | ||
Comment 59•17 years ago
|
||
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 60•17 years ago
|
||
Comment on attachment 310831 [details] [diff] [review]
v1.4
a=beltzner, yay!
Attachment #310831 -
Flags: approval1.9b5? → approval1.9b5+
Assignee | ||
Comment 61•17 years ago
|
||
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: 17 years ago
Keywords: uiwanted
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [has patch][need bug 407946]
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.
Description
•