404149 - Underline width doesn't correspond to character width in Arabic/Persian script

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[(no longer active)]

Attachment: Testcase for the bug

Please view the attached testcase.

The first 6 buttons show the accesskey set to the 1st, 2nd, 3rd, ... letter of the word آزمایش.  The accesskey underlines for the 3rd and 5th button are not properly positioned (shorter than the letter م in the 3rd button, and longer than the letter ی in the 5th button).

Also, the behavior differs based on the XUL control type.  For example, setting ب as an accesskey for a button, a label and a checkbox results in two different presentations: the larger than necessary underline in case of a button and label, and the correct presentation (thanks to bug 236135 being fixed) on trunk.

1.8 branch (Firefox 2.0.0.9 stock builds) shows nearly the same behavior, except for the checkbox control, where it breaks the joining of letters at the underlined letter.  This behavior is because the checkbox control shows its text as a html:span, with an embedded html:span to turn on the underline style by assigning the accesskey CSS class (this can be observed from the DOM Inspector), and therefore, bug 236135 kicks in for the 1.8 branch.

Comment 1

[(no longer active)]

This is very nice to get fixed at least for Arabic and Persian localizations, because these localizations are full of weird underlines for accesskeys throughout the whole user interface of Firefox (and any other XUL-based app).

Comment 2

[(no longer active)]

This happens on both 1.8 and Trunk.  Not sure how to specify it in Bugzilla...

Comment 3

[Behnam Esfahbod [:zwnj]]

Thanks Ehsan, but all of these problems are know.

The later problem is reported on bug 347981, Underline breaks Arabic joining in XUL text.

And about the first one. We had an old bug, with some patches from smontagu, which AFAIR has been FIXED on trunk, and WONTFIX on branch. It should be somewhere on the dependency tree of bug persian, but I couldn't find it. simon?

Comment 4

[Behnam Esfahbod [:zwnj]]

Found it: Bug 162081 – Wrong letter is underlined as accesskey / mnemonic when widget direction is RTL

Comment 5

[(no longer active)]

(In reply to comment #3)
> Thanks Ehsan, but all of these problems are know.

That's what I suspected.  :-)

> The later problem is reported on bug 347981, Underline breaks Arabic joining in
> XUL text.

It's actually the problem in bug 312436, which is fixed on trunk (the testcase is not affected with this problem on trunk) and it happens because the XUL checkbox uses a span element to underline part of its text, and the span causes the letters before/after and inside the span to break because of that bug.

> And about the first one. We had an old bug, with some patches from smontagu,
> which AFAIR has been FIXED on trunk, and WONTFIX on branch. It should be
> somewhere on the dependency tree of bug persian, but I couldn't find it. simon?
> 

(In reply to comment #4)
> Found it: Bug 162081 – Wrong letter is underlined as accesskey / mnemonic
> when widget direction is RTL
> 

Hmmm, this does not seem to be the problem I'm talking about.  View my testcase again: the correct letter is underlined, *but* for some letters, the underline bounds do not match the letter's bounds.  Plus, bug 162081 has been fixed on 1.8, but this problem affects both 1.8 and trunk currently.

Comment 6

[Simon Montagu :smontagu]

Seeing this on Linux also (Mac UI doesn't underline access keys). Changing summary to focus on the issue not fixed on trunk.

It looks as if the width of the underline corresponds to the width of the isolated form of the character, rather than the contextual form that actually gets rendered. It's probably not a bidi issue at all, but would occur with ligatures in any script.

Comment 7

[Behnam Esfahbod [:zwnj]]

Attachment: Minefield-nightly, Screenshot of the testcase, linux

Seems the computation of the position of the left side is correct, and just width of the underline is incorrect in some cases. (which are wider in this case)

Comment 8

[(no longer active)]

Attachment: Minefield-nightly, Screenshot of the testcase, vista

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

As you'd expect, the problem is that nsTextBoxFrame::CalculateUnderline is stupid and just measures the width of the character. The best way to fix this is probably to have bidiUtils->RenderText should return the width of the underline as well as its position, and just get rid of CalculateUnderline. Sounds like a job for Simon! :-)

Comment 10

[Simon Montagu :smontagu]

Attachment: Hindi testcase

This shows that at least in theory this isn't just a bidi issue, but as far as I can discover all LTR complex scripts use Latin access keys, so it's not too urgent to fix the non-bidi case.

Comment 11

[Daniel Veditz [:dveditz]]

Not blocking security releases, but if someone fixes this on the trunk (Firefox 3) we can investigate adopting that patch if it's low risk.

Comment 12

[(no longer active)]

Attachment: Patch (v1)

Simon, sorry for stealing this bug.  :-)

So, here is the approach roc suggested in comment 9.  With this approach, the access keys in attachment 289129 [details] render correctly.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 359997 [details] [diff] [review]
Patch (v1)

OK AFAIK, but honestly I don't understand this code at all.

Comment 14

[Simon Montagu :smontagu]

Comment on attachment 359997 [details] [diff] [review]
Patch (v1)

>               // Skipping to the "left part".
>               visualLeftPart = aText + start;
>+              // In LTR mode this is the same as visualLeftPart
>+              visualRightSide = visualLeftPart;
>             }


I don't understand why this isn't
                visualRightSide = visualLeftPart + 1;
in LTR.

Comment 15

[(no longer active)]

OK, let me explain the logic here.  We already calculate |subWidth| which is the width of the visual left offset of the text in the current subrun.  What we need to do here is to extend this width to include the current character as well and subtract |subWidth| from it, which will result in the width of the current character in its current form (after joining or any other stuff being applied to it) as will appear on screen.

The complication is that |subWidth| is calculated in a different way in RTL than in LTR.  I hope the below samples can demonstrate my algorithm better.  I included a space between characters for clarity, so the width of "A" for example is considered to be the width of "A ".

LTR (suppose access key is "D"):
   A B C D E F          (logical index: 3, visual index: 3)
   ^ (visualLeftPart)
   ^ (visualRightSide)
   visualLeftLength == 3
   ^^^^^^ (subWidth)
   ^^^^^^^^ (aprocessor.GetWidth() -- the 2nd one)
         ^^ (posResolve->visualWidth)

RTL (suppose access key is "‎ﻣ‎"):
   آ ز م‍ ا ی‍ ش          (logical index: 2, visual index: 3)
       ^ (visualLeftPart)
         ^ (visualRightSide)
   visualLeftLength == 3
   ^^^^^^ (subWidth)
   ^^^^^^^^ (aprocessor.GetWidth() -- the 2nd one)
         ^^ (posResolve->visualWidth)

Comment 16

[Simon Montagu :smontagu]

Comment on attachment 359997 [details] [diff] [review]
Patch (v1)

OK, now I follow. It would be great to put something like your last comment into the code. Probably better to use the left-to-right/TFEL-OT-THGIR convention rather than putting Arabic into the source file, though.

Comment 17

[(no longer active)]

Attachment: Patch (v1 + reftest)

I really wanted this to have a reliable reftest.  After experimenting with a number of ideas, I think this refrest tests exactly the problem.  The main xul file has the access key ف on the initial form of this letter.  Without this patch, the access key underline is wider than necessary, and therefore bleeds out of the containing <label>.  The ref xul file has the same access key only on the isolated form of the letter ف, so even without this patch the position of this access key is calculated correctly.  The test positions the labels outside of the viewport with only enough of them visible to capture the "bleed out" for the main xul file.  I have used a custom font to make sure that this won't get affected by different font configurations on different platforms, since the positioning of this test is critical.

I'm not sure if Simon should review the test as well as roc or not, so asking him for the sake of completeness.  Code-wise this is the same patch as attachment 359997 [details] [diff] [review] except for the added comment as suggested in comment 16.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 360549 [details] [diff] [review]
Patch (v1 + reftest)

Looks great. Just one question: what's the license for FreeFarsi? Can you include a link to it in the README?

Comment 19

[(no longer active)]

(In reply to comment #18)
> (From update of attachment 360549 [details] [diff] [review])
> Looks great. Just one question: what's the license for FreeFarsi? Can you
> include a link to it in the README?

It's GPL.  It's OK, right?

As for the link, the license is stated at <http://fpf.sourceforge.net/per/index.html#license> (in Persian) and also in the ZIP package downloaded from sourceforge.net.  Can I just link to the project page <http://sourceforge.net/projects/fpf/>?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 360549 [details] [diff] [review] [details])
> > Looks great. Just one question: what's the license for FreeFarsi? Can you
> > include a link to it in the README?
> 
> It's GPL.  It's OK, right?

Gerv?

> As for the link, the license is stated at
> <http://fpf.sourceforge.net/per/index.html#license> (in Persian) and also in
> the ZIP package downloaded from sourceforge.net.  Can I just link to the
> project page <http://sourceforge.net/projects/fpf/>?

Yes.

Comment 21

[Gervase Markham [:gerv]]

Using the unmodified GPL for fonts is not a good idea; even the FSF doesn't recommend it:
http://www.gnu.org/licenses/gpl-faq.html#FontException
http://blogs.zdnet.com/open-source/?p=247
http://yro.slashdot.org/article.pl?sid=05/04/17/2118203&from=rss

I could carefully look into what it would mean in this case, but it would be a lot easier for me if there was an alternative suitable font. Is there?

In the mean time, it would also be good to point out these problems to the creators of FreeFarsi.

Gerv

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I bet Jonathan could make one.

Comment 23

[(no longer active)]

(In reply to comment #21)
> I could carefully look into what it would mean in this case, but it would be a
> lot easier for me if there was an alternative suitable font. Is there?

I know a number of other Persian fonts as well but they're all released under the GPL.  What license should I be looking for?  Maybe I can find some free Arabic fonts under a suitable license.

(In reply to comment #22)
> I bet Jonathan could make one.

Great!  Johnathan, is there anything I can help with here?

Comment 24

[(no longer active)]

Oh, there is another font called Terafik which comes under a license similar to that of Bitstream Vera license.  Here is the full license copied from the license filed of the font:

<license>
Copyright (c) 2003 by Sharif FarsiWeb, Inc. All Rights Reserved.
Copyright (c) 2003 by Bitstream, Inc. All Rights Reserved.TerafikRegularSharifFarsiWeb: Terafik: 2004Version 2.900 2004Sharif FarsiWeb, Inc.You may freely redistribute this font, but you may not change or rename it.http://www.farsiweb.info/Copyright (c) 2003 by Sharif FarsiWeb, Inc. All Rights Reserved.

This font is based on the 
Roya and Bitstream Vera fonts.

The original source of the
Roya font is unknown, and the version this font is based on, had a copyright phrase of "(c) 1999 Everybody!". You may freely redistribute this font, but you may not change it in any way. Versions with other licensing will become available from Sharif FarsiWeb, which you may contact at http://www.farsiweb.info/ or mailto:[e-mail].

Bitstream Vera is Copyright (c) 2003 by Bitstream, Inc.
All Rights Reserved.
Bitstream Vera is a trademark of Bitstream, Inc.

Permission is hereby granted, free of charge, to any person obtaining a copy of the fonts accompanying this license ("Fonts") and associated documentation files (the "Font Software"), to reproduce and distribute the Font Software, including without limitation the rights to use, copy, merge, publish, distribute, and/or sell copies of the Font Software, and to permit persons to whom the Font Software is furnished to do so, subject to the following conditions:

The above copyright and trademark notices and this permission notice shall be included in all copies of one or more of the Font Software typefaces.

The Font Software may be modified, altered, or added to, and in particular the designs of glyphs or characters in the Fonts may be modified and additional glyphs or characters may be added to the Fonts, only if the fonts are renamed to names not containing either the words "Bitstream" or the word "Vera".

This License becomes null and void to the extent applicable to Fonts or Font Software that has been modified and is distributed under the "Bitstream Vera" names.

The Font Software may be sold as part of a larger software package but no copy of one or more of the Font Software typefaces may be sold by itself.

THE FONT SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO ANY WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF COPYRIGHT, PATENT, TRADEMARK, OR OTHER RIGHT. IN NO EVENT SHALL BITSTREAM OR THE GNOME FOUNDATION BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, INCLUDING ANY GENERAL, SPECIAL, INDIRECT, INCIDENTAL, OR CONSEQUENTIAL DAMAGES, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF THE USE OR INABILITY TO USE THE FONT SOFTWARE OR FROM OTHER DEALINGS IN THE FONT SOFTWARE.

Except as contained in this notice, the names of Gnome, the Gnome Foundation, and Bitstream Inc., shall not be used in advertising or otherwise to promote the sale, use or other dealings in this Font Software without prior written authorization from the Gnome Foundation or Bitstream Inc., respectively. For further information, contact: fonts at gnome dot org.
</license>

Gerv, is this OK for our purpose?

Comment 25

[Gervase Markham [:gerv]]

"You may freely redistribute this font, but you may not change it in any way."

That's definitely not free software :-( Sorry.

We are less constrained for test code than for product code. Suitable licenses would include anything BSD-like (including the unmodified Bitstream-style licenses), Apache, the Open Font License, the tri-license (unlikely) or probably, for test code and if we can't find anything better, the LGPL. Even GPL with font exception would be better.

Gerv

Comment 26

[Jonathan Kew [:jfkthame]]

The SIL Arabic fonts (eg Scheherazade) are OFL, so we could use that.

Alternatively, we could make a small Latin test font that does a contextual substitution of a glyph with a very different width; AIUI that would also show the issue, and might be simpler for non-Arabic/Persian readers to understand what's going on.

Comment 27

[(no longer active)]

(In reply to comment #26)
> The SIL Arabic fonts (eg Scheherazade) are OFL, so we could use that.
> 
> Alternatively, we could make a small Latin test font that does a contextual
> substitution of a glyph with a very different width; AIUI that would also show
> the issue, and might be simpler for non-Arabic/Persian readers to understand
> what's going on.

I don't have any experience of creating fonts.  If you think making a Latin test font is worth the effort here, and can spare some time to do that, then I'd be grateful!  Otherwise I'll just switch to the Scheherazade font.

Comment 28

[(no longer active)]

Is it a requirement for the src rule in @font-face to point to an absolute URI?  On the previous patch it seems like the @font-face defined font was not being picked up, and the font I had installed in my system was being used instead.  I found this out when trying out the Scheherazade font which I did not have installed.

Upon further testing, it seems that the @font-face defined fonts are only used when the URI is absolute, and I can't get them to work with relative URIs.  OTOH, all of the @font-face reftests use ../fonts/xyz.ttf, and not the absolute URI.  What am I missing?

Comment 29

[Jonathan Kew [:jfkthame]]

Note that relative URIs like ../fonts/xyz.ttf that contain parent-directory path elements won't work if you are loading the document using the file: scheme. The reftests use http(..) for this reason (see the reftest.list file in layout/reftests/font-face).

For local testing, it may be simplest to put the font in the same directory as your HTML document, so that the path doesn't include "..".

Comment 30

[(no longer active)]

(In reply to comment #29)
> Note that relative URIs like ../fonts/xyz.ttf that contain parent-directory
> path elements won't work if you are loading the document using the file:
> scheme. The reftests use http(..) for this reason (see the reftest.list file in
> layout/reftests/font-face).
> 
> For local testing, it may be simplest to put the font in the same directory as
> your HTML document, so that the path doesn't include "..".

Yes, that's what I did.  I also tried to copy them to a local webserver and test using http, but the same problem appeared there as well (relative path names did not work).

Comment 31

[(no longer active)]

Attachment: Patch (v1 + new reftest)

OK, I finally managed to get the reftest to work using the Scheherazade font that is already added to the tree.  Requesting review on the test changes.  I have verified that this test fails with a non-patched build, but succeeds with a patched one.

Comment 32

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c2dad7721eea

Comment 33

[(no longer active)]

Comment on attachment 368488 [details] [diff] [review]
Patch (v1 + new reftest)

Drivers: this is a fix for the most visible UI polish bug for complex script locales, such as Arabic and Persian (and maybe others as well).  It also has a reftest.  It's *really* nice to have for 1.9.1.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 368488 [details] [diff] [review]
Patch (v1 + new reftest)

a1.9.1=dbaron

Comment 35

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5891a17a2391

Comment 36

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.

Comment 37

[(no longer active)]

(In reply to comment #36)
> This bug's priority relative to the set of other polish bugs is:
> P2 - Polish issue that is in a secondary interface, occasionally encountered,
> and is easily identifiable.

I think this assessment is wrong.  This bug used to appear in primary UI (menu bar for example), and in fact anywhere that we display access keys (in other XUL apps as well).  And it was encountered all the time.  This was the single most noticeable bug that I saw users who ran Persian Firefox reported after working with the browser for a few minutes.

Comment 38

[Alex Faaborg [:faaborg] (Firefox UX)]

>I think this assessment is wrong.  This bug used to appear in primary UI (menu
>bar for example), and in fact anywhere that we display access keys (in other
>XUL apps as well).  And it was encountered all the time.  This was the single
>most noticeable bug that I saw users who ran Persian Firefox reported after
>working with the browser for a few minutes.

Thanks, I didn't really have good context and didn't actually think about underlines in chrome. Switching to P1.