Last Comment Bug 617203 - "First character must be the start of a cluster ..." assertion failure with pango-graphite
: "First character must be the start of a cluster ..." assertion failure with p...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Karl Tomlinson (:karlt)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-06 17:16 PST by Karl Tomlinson (:karlt)
Modified: 2012-11-04 04:27 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
ignore is_cursor_position for the position before the first character (2.82 KB, patch)
2010-12-06 18:24 PST, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review

Description Karl Tomlinson (:karlt) 2010-12-06 17:16:37 PST
http://hg.mozilla.org/mozilla-central/annotate/2b44a6a3bfd8/gfx/thebes/gfxFont.cpp#l3917

When pango-graphite is installed, it adds a PangoEngineLang, which changes the
behavior of pango_break.

The behaviour differs enough that it leads to failed assertions that the first
character of a run begins a cluster, and it is not possible to move the cursor
to the beginning of text if Graphite fonts are used.

This is because Graphite's break does not set is_cursor_position for the
position before the first character.

STR:

1. Install pango-graphite and Dai Banna SIL Book
   http://scripts.sil.org/DaiBannaSIL

2. Copy 'ᦎᦷ ᦑᦺ' to the search bar.
   See assertion failures (with debug builds).

3. Try to use arrow keys to move cursor to beginning of the text.
   Fail.
Comment 1 Karl Tomlinson (:karlt) 2010-12-06 17:17:43 PST
I suspect assertions would fail even in very old builds.
The cursor behavior is a regression since 2010-10-14.
Comment 2 Karl Tomlinson (:karlt) 2010-12-06 17:18:22 PST
I'm tempted to blacklist GraphiteEngineLang because it claims support for
multiple scripts, but, for non-graphite fonts (Engine selection is not
font-dependent), it merely runs the basic pango_default_break, overriding
script-specific Engines - and it runs pango_default_break twice.
See graphite_engine_break at
http://scripts.sil.org/svn-public/graphite/graphite/trunk/wrappers/pangographite/silgraphite-break.c

However, I think it is a bug in our code to rely on specific behavior from
pango_break, and we should fix that.
http://hg.mozilla.org/mozilla-central/annotate/2b44a6a3bfd8/gfx/thebes/gfxPangoFonts.cpp#l2571
Comment 3 Karl Tomlinson (:karlt) 2010-12-06 18:24:16 PST
Created attachment 495735 [details] [diff] [review]
ignore is_cursor_position for the position before the first character
Comment 4 Jonathan Kew (:jfkthame) 2011-01-12 09:18:34 PST
With the improvements we've made to gfxPlatform::SetupClusterBoundaries() (see bug 553981), do we really still need to be calling pango_break here, or could we remove the static SetupClusterBoundaries function from the gfxPangoFonts.cpp code altogether (thereby resolving both this and bug 614476)?
Comment 5 Karl Tomlinson (:karlt) 2011-05-18 14:06:37 PDT
We now only call pango_break (this static SetupClusterBoundaries()) when using Pango for shaping.

Pango's indic PangoEngineLangs has some special breaking rules that I assume gfxPlatform::SetupClusterBoundaries() does not handle, and it seems appropriate to use pango_break when using pango_shape (as I assume we do for indic scripts).
Comment 6 Karl Tomlinson (:karlt) 2012-01-11 12:41:29 PST
This code was removed in bug 703100.

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