Last Comment Bug 654057 - Combining character doesn't work right in "Segoe UI" font
: Combining character doesn't work right in "Segoe UI" font
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on:
Blocks: 635639
  Show dependency treegraph
 
Reported: 2011-05-02 01:07 PDT by Brett Zamir
Modified: 2011-05-11 12:30 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, fix handling of non-spacing marks in Segoe UI (2.04 KB, patch)
2011-05-05 00:44 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review
reftest for diacritic in Segoe UI (2.19 KB, patch)
2011-05-05 03:32 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review

Description Brett Zamir 2011-05-02 01:07:18 PDT
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 

I'm not sure whether this is some other issue, but when I try to do an alert using a combining diacritical mark, it looks off, but when added into the page, it looks fine.


Reproducible: Always

Steps to Reproduce:
1. Add a character with a combining mark into an HTML page: À
2. Add a script into the same page to alert the same character:
<script type="text/javascript">
alert('A\u0300'); // The diacritic appears to the top left of the character instead of the top
</script>


Actual Results:  
The characters appear differently, with the alerted character having the diacritic askew.

Expected Results:  
See the combining character seamlessly integrated into the base character.
Comment 1 Boris Zbarsky [:bz] 2011-05-02 04:59:58 PDT
This worksforme (on Mac).

Are you sure this isn't just an issue of the web page and the alert using different fonts and the font for the latter (or our handling of it) being broken in your case?
Comment 2 Brett Zamir 2011-05-02 05:07:21 PDT
Yes, that may well be. I'm sorry I don't know how to confirm, just know that the default alert font isn't working--not sure that's something u can do anything about or not.
Comment 3 Boris Zbarsky [:bz] 2011-05-02 05:15:38 PDT
Hm.  What hapens if you style your HTML with "font-family: sans-serif"?
Comment 4 Brett Zamir 2011-05-02 05:30:25 PDT
HTML is still ok (and alert still not good). I forgot though that I was testing with a XUL-based HTML editor, and oddly, it is only in that environment that it appears incorrectly---in regular HTML loaded inside a regular browser tab, it shows correctly.
Comment 5 Boris Zbarsky [:bz] 2011-05-02 05:53:18 PDT
Does the alert you get in the editor look the same as the regular HTML one?  Or is it a separate dialog instead of a tab-modal box?
Comment 6 Boris Zbarsky [:bz] 2011-05-02 05:54:39 PDT
What happens in HTML styled with "font: menu"?
Comment 7 Brett Zamir 2011-05-02 06:00:11 PDT
The alert in the editor says "[JavaScript application]" in the titlebar and its text is conveniently copy-pasteable as in Firefox 3.6 (unlike Firefox 4 now).

 * {font:menu} indeed makes the HTML messed up like the alert (whether applied in a tab or the editor).
Comment 8 Boris Zbarsky [:bz] 2011-05-02 06:02:27 PDT
OK, yeah.  In the editor you're getting a modal dialog alert, not a tab-modal alert.  And they use different font styles.

Sounds like something is up with our interaction with your "menu" font.  Any idea what font that is?  ;)
Comment 9 Brett Zamir 2011-05-02 06:10:06 PDT
Ok, the "winner" is: "Segoe UI" (Windows 7)
Comment 10 Jonathan Kew (:jfkthame) 2011-05-02 08:49:26 PDT
Ugh... I suspect this is a regression from bug 635639. We may have to try and make the patch there more selective in its effects.
Comment 11 Jonathan Kew (:jfkthame) 2011-05-05 00:44:06 PDT
Created attachment 530252 [details] [diff] [review]
patch, fix handling of non-spacing marks in Segoe UI

The problem here is that the patch for bug 635639 was not quite right (although simply reverting it wouldn't fix things, as that leaves a gap where the diacritic occurs due to its non-zero advance).

In hb_ot_layout_position_finish(), it was incorrect to touch the diacritic's x_offset value, because at this stage GPOS::position_finish() has not yet been called. So all we need to do here is zero the advance width, and then let position_finish() do the proper attachment.
Comment 12 Jonathan Kew (:jfkthame) 2011-05-05 03:32:09 PDT
Created attachment 530268 [details] [diff] [review]
reftest for diacritic in Segoe UI
Comment 13 John Daggett (:jtd) 2011-05-10 23:25:30 PDT
Comment on attachment 530268 [details] [diff] [review]
reftest for diacritic in Segoe UI

Might be nice to have a font that simply replicates the problem found in Segoe UI so that we would catch this on all platforms.  But I'll let you judge whether that's worth the effort or not.
Comment 14 Jonathan Kew (:jfkthame) 2011-05-11 12:30:26 PDT
http://hg.mozilla.org/mozilla-central/rev/800a2d46bb4e (patch)
http://hg.mozilla.org/mozilla-central/rev/427fa3603d4c (testcase)

We could create a custom font that replicates the problem, but I don't see it as a high priority right now; as long as we're running tests on Win7, at least, we'll know if we regress this (e.g. when taking a harfbuzz update). If we happen to run across a free font that exhibits the same issue, we could add that to the testcase.

Note You need to log in before you can comment on or make changes to this bug.