[HarfBuzz] Arabic mark width-zeroing regression

[Jonathan Kew]

Hi Behdad,

As per Mozilla bug https://bugzilla.mozilla.org/show_bug.cgi?id=873902, 
it looks like commit f368ba4a9edec4e297616698097546e8e6c89e53 has 
regressed behavior for some "old" Arabic fonts.

It looks like we need to keep the mark-zeroing *after* GPOS for Arabic, 
even if we're changing it to rely on GDEF rather than Unicode 
categories. Possible patch attached; this appears to work correctly with 
both Amiri and old fonts.

JK
-------------- next part --------------
diff --git a/src/hb-ot-shape-complex-arabic.cc b/src/hb-ot-shape-complex-arabic.cc
index 2f069d0..7f8778a 100644
--- a/src/hb-ot-shape-complex-arabic.cc
+++ b/src/hb-ot-shape-complex-arabic.cc
@@ -352,6 +352,6 @@ const hb_ot_complex_shaper_t _hb_ot_complex_shaper_arabic =
   NULL, /* decompose */
   NULL, /* compose */
   setup_masks_arabic,
-  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF,
+  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF_LATE,
   true, /* fallback_position */
 };
diff --git a/src/hb-ot-shape-complex-default.cc b/src/hb-ot-shape-complex-default.cc
index ca092b5..d6afa0e 100644
--- a/src/hb-ot-shape-complex-default.cc
+++ b/src/hb-ot-shape-complex-default.cc
@@ -215,6 +215,6 @@ const hb_ot_complex_shaper_t _hb_ot_complex_shaper_default =
   NULL, /* decompose */
   compose_default,
   NULL, /* setup_masks */
-  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE,
+  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE_LATE,
   true, /* fallback_position */
 };
diff --git a/src/hb-ot-shape-complex-myanmar.cc b/src/hb-ot-shape-complex-myanmar.cc
index ff13bdd..e5af893 100644
--- a/src/hb-ot-shape-complex-myanmar.cc
+++ b/src/hb-ot-shape-complex-myanmar.cc
@@ -540,6 +540,6 @@ const hb_ot_complex_shaper_t _hb_ot_complex_shaper_myanmar =
   NULL, /* decompose */
   NULL, /* compose */
   setup_masks_myanmar,
-  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF,
+  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF_EARLY,
   false, /* fallback_position */
 };
diff --git a/src/hb-ot-shape-complex-private.hh b/src/hb-ot-shape-complex-private.hh
index 1474a3d..62151ba 100644
--- a/src/hb-ot-shape-complex-private.hh
+++ b/src/hb-ot-shape-complex-private.hh
@@ -41,8 +41,10 @@
 
 enum hb_ot_shape_zero_width_marks_type_t {
   HB_OT_SHAPE_ZERO_WIDTH_MARKS_NONE,
-  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE,
-  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF
+//  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE_EARLY,
+  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE_LATE,
+  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF_EARLY,
+  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF_LATE
 };
 
 
diff --git a/src/hb-ot-shape-complex-thai.cc b/src/hb-ot-shape-complex-thai.cc
index 5cbb6e3..4594533 100644
--- a/src/hb-ot-shape-complex-thai.cc
+++ b/src/hb-ot-shape-complex-thai.cc
@@ -373,6 +373,6 @@ const hb_ot_complex_shaper_t _hb_ot_complex_shaper_thai =
   NULL, /* decompose */
   NULL, /* compose */
   NULL, /* setup_masks */
-  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE,
+  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE_LATE,
   false,/* fallback_position */
 };
diff --git a/src/hb-ot-shape.cc b/src/hb-ot-shape.cc
index f65861f..a0cc755 100644
--- a/src/hb-ot-shape.cc
+++ b/src/hb-ot-shape.cc
@@ -419,11 +419,11 @@ hb_ot_position_default (hb_ot_shape_context_t *c)
 
   }
 
-  /* Zero'ing mark widths by GDEF (as used in Myanmar spec) happens
-   * *before* GPOS. */
   switch (c->plan->shaper->zero_width_marks)
   {
-    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF:
+    /* Zero'ing mark widths by GDEF (as used in Myanmar spec) may happen
+     * *before* GPOS. */
+    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF_EARLY:
       for (unsigned int i = 0; i < count; i++)
 	if ((c->buffer->info[i].glyph_props() & HB_OT_LAYOUT_GLYPH_PROPS_MARK))
 	{
@@ -432,9 +432,21 @@ hb_ot_position_default (hb_ot_shape_context_t *c)
 	}
       break;
 
+    /* Not currently used for any shaper:
+    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE_EARLY:
+      for (unsigned int i = 0; i < count; i++)
+	if (_hb_glyph_info_get_general_category (&c->buffer->info[i]) == HB_UNICODE_GENERAL_CATEGORY_NON_SPACING_MARK)
+	{
+	  c->buffer->pos[i].x_advance = 0;
+	  c->buffer->pos[i].y_advance = 0;
+	}
+      break;
+    */
+
     default:
     case HB_OT_SHAPE_ZERO_WIDTH_MARKS_NONE:
-    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE:
+    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE_LATE:
+    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF_LATE:
       break;
   }
 }
@@ -472,7 +484,7 @@ hb_ot_position_complex (hb_ot_shape_context_t *c)
    * *after* GPOS. */
   switch (c->plan->shaper->zero_width_marks)
   {
-    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE:
+    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE_LATE:
       for (unsigned int i = 0; i < count; i++)
 	if (_hb_glyph_info_get_general_category (&c->buffer->info[i]) == HB_UNICODE_GENERAL_CATEGORY_NON_SPACING_MARK)
 	{
@@ -481,9 +493,19 @@ hb_ot_position_complex (hb_ot_shape_context_t *c)
 	}
       break;
 
+    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF_LATE:
+      for (unsigned int i = 0; i < count; i++)
+	if ((c->buffer->info[i].glyph_props() & HB_OT_LAYOUT_GLYPH_PROPS_MARK))
+	{
+	  c->buffer->pos[i].x_advance = 0;
+	  c->buffer->pos[i].y_advance = 0;
+	}
+      break;
+
     default:
     case HB_OT_SHAPE_ZERO_WIDTH_MARKS_NONE:
-    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF:
+    //case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE_EARLY:
+    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF_EARLY:
       break;
   }