282126 - Fix support for the 'ch' length unit to match spec (fallback for no '0' glyph, vertical metrics)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Mats Palmgren (inactive)]

What to do about the 'ch' length unit? (Mozilla vendor specific)

(Followup from bug 281972)

The only real internal dependence is:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/content/src/nsHTMLPreElement.cpp&rev=1.63&root=/cvsroot&mark=127,145,146#125

That is, to wrap a <pre> exactly at the number of characters specified by
WIDTH (or COLS) we need a unit that is based on the width of a character
in the current font.

The problem is that all the relative widths that are font based are
relative the height, not the width:
http://www.w3.org/TR/CSS21/syndata.html#length-units
http://www.w3.org/TR/2001/WD-css3-values-20010713/#lengths

So, to get rid of this Mozilla vendor unit I see two options.
1. convince the CSS working group that 'ch' is needed
2. remove it and introduce a "nsPreFrame" that looks at the
   WIDTH/COLS attr to set its width during reflow (similar to what
   nsTextControlFrame does).
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsTextControlFrame.cpp&rev=3.191&root=/cvsroot&mark=1583,1606,1607#1582

http://lxr.mozilla.org/mozilla/search?string=eStyleUnit_Chars
http://lxr.mozilla.org/mozilla/search?string=eCSSUnit_Char
http://lxr.mozilla.org/mozilla/search?string=eCSSKeyword_ch

Comment 1

[Anne (:annevk)]

It has been proposed:
 <http://lists.w3.org/Archives/Public/www-style/1999Dec/0026.html>

(I could not find any other emails about it.)

Comment 2

[Mats Palmgren (inactive)]

I suppose this is an alternative as well:
3. drop the 'ch' keyword and only use eCSSUnit_Char internally

Comment 3

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

Or (a variant of (3)) we could rename it to -moz-ch to allow round-tripping.

That said, it's needed to allow the cascade to work correctly, and we should
probably be using it for input and textarea as well for exactly that reason.

Comment 4

[Mats Palmgren (inactive)]

I agree with the cascade argument, but input and textarea is using
nsIFontMetrics::GetAveCharWidth() rather than the width of a 'M',
so it's not the same measure. Hm, -moz-avg-ch perhaps? 

Comment 5

[Mats Palmgren (inactive)]

Given that 'ch' is only intended for use with fixed width fonts at the moment,
I suppose we could change this:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsHTMLReflowState.cpp&rev=1.218&root=/cvsroot&mark=2277,2294-2306#2276

to use nsIFontMetrics::GetAveCharWidth() instead.
I think it's reasonable to expect it to be equal to the width of a 'M' in a
fixed width font, yes?

If so, we can use it for input and textarea too.

Also, I would like length units to be universal, at the moment 'ch' can't
be used to specify 'height' for example (it results in zero height).

Comment 6

[Boris Zbarsky [:bzbarsky]]

So there are two issues here:

1)  Should we have an internal "ch" unit that we use for <pre> and <input>?  I
think the answer is yes.

2)  Should this be accessible from CSS stylesheets (that is, parsed)?  I think
the answer here should be "not unless it's a -moz unit".

For the rest, having this work for all lenths could be interesting -- the "ch"
unit depends on the font metrics, which depend on the lang group, which comes
from the "visibility" struct.  But we have lengths in the "display" struct,
which makes "display" depend on "visibility".  I don't think that's desirable
(and I'm not sure it doesn't introduce a circular dependency).  Also, should
"ch" do all the weirdness that nsTextControlFrame does right now for
non-fixed-width fonts?  I suspect it should...

Comment 7

[Ng Ming Hong]

If there is a "ch" unit, what will be the impact to characters that is wider
than "M", especially double-byte characters. Consider this:
<pre style="width: 10ch">
1234567890
&#19968;&#20108;&#19977;&#22235;&#20116;&#20845;&#19971;&#20843;&#20061;&#21313;
</pre>

Should Mozilla wraps the second line at "&#20116;" or "&#20845;" (around the tenth charcter
on the first line), or "&#21313;" (exactly the tenth character)?

If it is the former case, maybe it should be name "byte" so as to reflect the
fact that it is not counting character-width but byte-width.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> If there is a "ch" unit, what will be the impact to characters that is wider
> than "M"

That's up to the GetAveCharWidth implementation in the relevant nsIFontMetrics.
 You can test it using <input size="10">, since that already uses the function.
 And yes, this should be counting characters, not bytes.

Comment 9

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

The algorithm used by text inputs handles both fixed-width and proportional
fonts.  Perhaps something like that algorithm would provide a reasonable
definition for 'ch' for all cases?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsTextControlFrame.cpp&rev=3.191&mark=1602-1644#1582

Comment 10

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

(In reply to comment #6)
> For the rest, having this work for all lenths could be interesting -- the "ch"
> unit depends on the font metrics, which depend on the lang group, which comes
> from the "visibility" struct.  But we have lengths in the "display" struct,
> which makes "display" depend on "visibility".  I don't think that's desirable
> (and I'm not sure it doesn't introduce a circular dependency).

Yeah, this is a problem.  I've said a few times that the CSS WG should have a
dependency map for computed value calculation.  We probably need to do the same,
and make sure that we don't get dependencies across structs.

Comment 11

[[e-mail]]

The rationale for bug 281630 which this bug has been marked as blocking is that
although the default Windows and Linux UI fonts are reasonably similar so that
dialogs designed on one work on the other their aspect ratio and that of the
default Mac UI font differ sufficiently for us to have had to put various hacks
into e.g. the preferences and account manager windows to size them appropriately
which a native "ch" (or whatever) unit would have accomplished automatically.

Of course the bug also affects people who change their UI font to something
sufficiently dissimilar but they've currently just had to live with it.

Comment 12

[Mats Palmgren (inactive)]

"CSS3 Values and Units" now includes a 'ch' unit:
http://www.w3.org/TR/css3-values/#relative0

Should we morph this bug into a meta bug with dependencies to the bugs that
handles the specific errors?

Comment 13

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

css3-values says:

  The width of the "0" (ZERO, U+0030) glyph found in the font for the font size used to render. If the "0" glyph is not found in the font, the average character width may be used.

AddCoord in nsFrame.cpp and GetAbsoluteCoord in nsLayoutUtils.cpp use the width of the "M".

nsTextControlFrame::CalcIntrinsicSize does something more complicated, "for compatibility with IE".

(If my memory is correct, the wording in css3-values is there because it's what the IE folks said IE does.  So it might be even more compatible with IE.)

Comment 14

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

Oh, and nsTextControlFrame::CalcIntrinsicSize also accounts for letter-spacing, which is harder on a unit...

Comment 15

[Zack Weinberg (:zwol)]

See patches just added to bug 363706 - we could make CalcIntrinsicSize use GetZeroOrAveCharWidth() instead and possibly take out some of the "for IE compatibility" complications.

Comment 16

[j.j.]

Isn't this fixed with bug 363706?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Not really, no.

Comment 18

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

https://drafts.csswg.org/css-values-4/#ch has a spec for the 'ch' unit now.

Comment 19

[Cameron McCormack (:heycam)]

We do use the "0" glyph's horizontal advance since bug 363706, but because we look at nsFontMetrics::zeroOrAveCharWidth, we'll end up using the OS/2 xAvgCharWidth value if there's no "0" glyph in the font.  Instead we should be falling back to using 0.5em, according to the spec. 

We do do that falling back correctly when we fail to get font metrics, here:

https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/servo/components/style/values/specified/length.rs#168-193

Also I don't think we correctly use the vertical advance of the "0" glyph when we're in a vertical writing mode with upright glyphs:

https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/gfx/thebes/gfxFont.cpp#4071-4076

Comment 20

[Cameron McCormack (:heycam)]

Attachment: Bug 282126 - Part 1: Allow getting zero glyph width from nsFontMetrics without falling back to average glyph width.

Review commit: https://reviewboard.mozilla.org/r/251774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251774/

Comment 21

[Cameron McCormack (:heycam)]

Attachment: Bug 282126 - Part 2: Allow FontMetricsProvider to produce ex height and zero width independently.

We are always able to produce an x height, but depending on whether the
glyph exists, we sometimes can't produce a zero glyph width.

Review commit: https://reviewboard.mozilla.org/r/251776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251776/

Comment 22

[Cameron McCormack (:heycam)]

Attachment: Bug 282126 - Part 3: Don't compute ch to the average glyph width when there is no zero glyph.

Review commit: https://reviewboard.mozilla.org/r/251778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251778/

Comment 23

[Cameron McCormack (:heycam)]

Attachment: Bug 282126 - Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always.

Review commit: https://reviewboard.mozilla.org/r/251780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251780/

Comment 24

[Cameron McCormack (:heycam)]

Attachment: Bug 282126 - Part 5: Tests.

Review commit: https://reviewboard.mozilla.org/r/251782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251782/

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8986412 [details]
Bug 282126 - Part 2: Allow FontMetricsProvider to produce ex height and zero width independently.

https://reviewboard.mozilla.org/r/251776/#review258160

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8986413 [details]
Bug 282126 - Part 3: Don't compute ch to the average glyph width when there is no zero glyph.

https://reviewboard.mozilla.org/r/251778/#review258162

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8986414 [details]
Bug 282126 - Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always.

https://reviewboard.mozilla.org/r/251780/#review258168

Could you explain why this is needed / desirable, please? And a spec quote for this would be ideal :).

css-values says:

> Thus, the ch unit falls back to 0.5em in the general case, and to 1em when it would be typeset upright (i.e. writing-mode is vertical-rl or vertical-lr and text-orientation is upright).

So I guess that means the check should be `wm.is_vertical() && wm.is_upright()`?

Also, it does seem weird to make ex depend on the writing mode in a slightly different way. I think given the spec doesn't special-case the fall-back, we should indeed always be using horizontal metrics... But what do other browsers do?

Comment 28

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #27)
> Also, it does seem weird to make ex depend on the writing mode in a slightly
> different way. I think given the spec doesn't special-case the fall-back, we
> should indeed always be using horizontal metrics... But what do other
> browsers do?

(Or at least be consistent with ex about vertical vs. horizontal.)

Comment 29

[Jonathan Kew [:jfkthame]]

Comment on attachment 8986411 [details]
Bug 282126 - Part 1: Allow getting zero glyph width from nsFontMetrics without falling back to average glyph width.

https://reviewboard.mozilla.org/r/251774/#review258170

::: gfx/thebes/gfxFont.h:1678
(Diff revision 1)
>          gfxFloat spaceWidth;
> -        gfxFloat zeroOrAveCharWidth;  // width of '0', or if there is
> -                                      // no '0' glyph in this font,
> -                                      // equal to .aveCharWidth
> +        gfxFloat zeroWidth;
> +
> +        gfxFloat ZeroOrAveCharWidth() const
> +        {
> +          return zeroWidth ? zeroWidth : aveCharWidth;

In principle, the font could have a '0' glyph that exists but has a width of zero (i.e. a non-spacing glyph). In that case, this will return `aveCharWidth` instead of the true `zeroWidth` value of 0.

To avoid that, maybe we should use a negative value for `zeroWidth` to represent "unavailable", and accept 0.0 as a valid width?

(Is there potential breakage anywhere if the `ch` unit evaluates to zero? Should the spec address this edge case?)

Comment 30

[Emilio Cobos Álvarez (:emilio)]

I talked with Jonathan a bit over IRC in: https://mozilla.logbot.info/layout/20180620#c14907419

Our vertical font metrics for x height / 0 width are pretty arbitrary[1], so I wonder if it may make sense to just take the fallback path on style unconditionally? Or, alternatively whether the line in part 1 which you turned from:

-metrics->zeroOrAveCharWidth = metrics->aveCharWidth;
+metrics->zeroWidth = metrics->aveCharWidth;

Should turn instead into:

-metrics->zeroWidth = metrics->aveCharWidth;
+metrics->zeroWidth = 0.0;

(Or the negative value Jonathan suggests in comment 29)

[1]: https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/gfx/thebes/gfxFont.cpp#4071

Comment 31

[Cameron McCormack (:heycam)]

So I just did some quick testing to see what other browsers do with ex in vertical writing modes (both with and without text-orientation:upright).

http://mcc.id.au/temp/ex.html

Chrome, Edge, and Safari all seem to use the same value in vertical writing modes.  So I think we should do that rather than synthesize the "emHeight / 2" value.

For ch, falling back to 1em is *probably* OK, but of course it's possible to create a font whose zero glyph has a vertical advance different from that.  I tested that:

http://mcc.id.au/temp/chv.html

Edge and Safari don't support text-orientation:upright, it seems.  But Safari does use the vertical advance of the zero glyph for ex.  Firefox, Chrome, and Edge are all using 1em, although I can't be sure where this value is coming from (maybe something in the font?) for Chrome and Edge.

Regarding the possibility of 0.0 advance glyphs, yes I should have thought of that.

(I wonder if would be nicer to have separate horizontal and vertical metrics requests, and instead have gfxFont::Metrics and nsFontMetrics be able to store all metrics, horizontal and vertical, and for consumers then to use the ones they're interested in.)

Comment 32

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #31)
> (I wonder if would be nicer to have separate horizontal and vertical metrics
> requests, and instead have gfxFont::Metrics and nsFontMetrics be able to
> store all metrics, horizontal and vertical, and for consumers then to use
> the ones they're interested in.)

*not to have separate horizontal and vertical metrics requests

Comment 33

[Cameron McCormack (:heycam)]

Comment on attachment 8986414 [details]
Bug 282126 - Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always.

https://reviewboard.mozilla.org/r/251780/#review258168

You probably found it, but the spec requirement is https://drafts.csswg.org/css-values-3/#ch.  I guess you can think of it as if the zero glyph's baseline will be parallel with inline axis of the block it's in, then we should use the horizontal advance, and otherwise the vertical advance.  Since "0" is one of those glyphs that is intrinsically horizontal, we should only ever use the vertical advance when text-orientation:upright and writing-mode is one of the vertical ones.

If your use case for using ch is as an approximation for how long some text of a certain number of characters will be, then using the vertical advance only in this case makes sense.

I think `wm.is_upright()` implies `wm.is_vertical()` so I didn't bother checking for `is_vertical()`.  I can do that if you think it's clearer.

Comment 34

[Cameron McCormack (:heycam)]

(In reply to Jonathan Kew (:jfkthame) from comment #29)
> (Is there potential breakage anywhere if the `ch` unit evaluates to zero?
> Should the spec address this edge case?)

I don't see anything fundamentally difficult about allowing ch units to evaluate to zero, although I wouldn't be surprised if some implementations fall back to a guess like 0.5em in that case.  Some testing for that shows that Chrome, Firefox, and Safari all evaluate ch to zero if that's what the zero glyph advance is, but Edge does not.

http://mcc.id.au/temp/chzero.html

Comment 35

[Cameron McCormack (:heycam)]

For now I'm not going to fix the vertical metrics for ch.  I'll file a followup.  (But I've written a test.)

Comment 36

[Cameron McCormack (:heycam)]

Comment on attachment 8986411 [details]
Bug 282126 - Part 1: Allow getting zero glyph width from nsFontMetrics without falling back to average glyph width.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251774/diff/1-2/

Comment 37

[Cameron McCormack (:heycam)]

Comment on attachment 8986412 [details]
Bug 282126 - Part 2: Allow FontMetricsProvider to produce ex height and zero width independently.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251776/diff/1-2/

Comment 38

[Cameron McCormack (:heycam)]

Comment on attachment 8986413 [details]
Bug 282126 - Part 3: Don't compute ch to the average glyph width when there is no zero glyph.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251778/diff/1-2/

Comment 39

[Cameron McCormack (:heycam)]

Comment on attachment 8986414 [details]
Bug 282126 - Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251780/diff/1-2/

Comment 40

[Cameron McCormack (:heycam)]

Comment on attachment 8986415 [details]
Bug 282126 - Part 5: Tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251782/diff/1-2/

Comment 41

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8986414 [details]
Bug 282126 - Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always.

https://reviewboard.mozilla.org/r/251780/#review258378

::: servo/components/style/values/specified/length.rs:178
(Diff revision 2)
>              },
>              FontRelativeLength::Ch(length) => {
>                  if context.for_non_inherited_property.is_some() {
>                      context.rule_cache_conditions.borrow_mut().set_uncacheable();
>                  }
> -                let metrics = query_font_metrics(context, reference_font_size);
> +                let is_upright = context.style().writing_mode.is_upright();

I don't think `is_upright` implies `is_vertical`. At least I don't see what would prevent[1] to be hit with an horizontal writing mode.

So I do think the is_vertical check is required, please correct me if I'm wrong.

With that fixed (and ideally a spec quote / spec link), this looks good, thanks!

[1]: https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/servo/components/style/logical_geometry.rs#91

Comment 42

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8986415 [details]
Bug 282126 - Part 5: Tests.

https://reviewboard.mozilla.org/r/251782/#review258380

Looks great, thank you! If I was right in the previous commit, a test with `text-decoration: upright` and `writing-mode: horizontal-tb` would be really appreciated :)

::: testing/web-platform/meta/css/css-values/ch-unit-009.html.ini:2
(Diff revision 2)
> +[ch-unit-009.html]
> +  expected: FAIL

You can annotate this with `bug: <bug-link>` for extra niceness. :)

Comment 43

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #42)
> Looks great, thank you! If I was right in the previous commit, a test with
> `text-decoration: upright` and `writing-mode: horizontal-tb` would be really
> appreciated :)

You are right, so I've added a test for that. :-)

Comment 44

[Cameron McCormack (:heycam)]

Comment on attachment 8986414 [details]
Bug 282126 - Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251780/diff/2-3/

Comment 45

[Cameron McCormack (:heycam)]

Comment on attachment 8986415 [details]
Bug 282126 - Part 5: Tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251782/diff/2-3/

Comment 46

[Jonathan Kew [:jfkthame]]

Comment on attachment 8986411 [details]
Bug 282126 - Part 1: Allow getting zero glyph width from nsFontMetrics without falling back to average glyph width.

https://reviewboard.mozilla.org/r/251774/#review258784

::: gfx/thebes/gfxDWriteFonts.h:86
(Diff revision 2)
>  
>      bool HasBitmapStrikeForSize(uint32_t aSize);
>  
>      cairo_font_face_t *CairoFontFace();
>  
> -    gfxFloat MeasureGlyphWidth(uint16_t aGlyph);
> +    bool MeasureGlyphWidth(uint16_t aGlyph, gfxFloat* aResult);

Giving this a bool (success) return value and using an outparam for the actual result makes me a bit sad... see below.

::: gfx/thebes/gfxDWriteFonts.cpp:279
(Diff revision 2)
>  
>      ucs = L'0';
>      if (SUCCEEDED(mFontFace->GetGlyphIndicesW(&ucs, 1, &glyph))) {
> -        mMetrics->zeroOrAveCharWidth = MeasureGlyphWidth(glyph);
> +        if (!MeasureGlyphWidth(glyph, &mMetrics->zeroWidth)) {
> +            mMetrics->zeroWidth = -1.0;  // indicates not found
> -    }
> +        }

I'd suggest we do something more like

    if (SUCCEEDED(mFontFace->GetGlyphIndicesW(&ucs, 1, &glyph) && glyph != 0) {
        mMetrics->zeroWidth = MeasureGlyphWidth(...);
    } else {
        mMetrics->zeroWidth = -1.0;
    }

so that we can keep MeasureGlyphWidth() as a simple function returning the value directly, rather than having to change it to return a success code and then use an outparam for the real result.

In fact, on further thought I think that's really what we want to do. If the font doesn't support the character '0', GetGlyphIndicesW will return glyph ID 0, but AFAIK that won't cause MeasureGlyphWidth to fail; it'll just return the width of the font's .notdef.

(While you're here, I guess we should similarly check that GetGlyphIndicesW gave us a non-zero glyph ID for the 'x' just above, as measuring the .notdef glyph doesn't seem like a very useful idea.)

Comment 47

[Chris Mills [:cmills]]

Not sure if we need anything in the docs about this, but adding ddn just in case.

Comment 48

[Jonathan Kew [:jfkthame]]

Comment on attachment 8986415 [details]
Bug 282126 - Part 5: Tests.

https://reviewboard.mozilla.org/r/251782/#review258882

Yeah, text-orientation should have no effect at all for horizontal-tb writing mode (or for sideways-{lr,rl}, for that matter); it's only relevant to vertical typographic modes.[1] (But it doesn't hurt to verify that it does in fact have no effect!)

[1] https://drafts.csswg.org/css-writing-modes-4/#text-orientation

Comment 49

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37f68cdb5380eb7071e56f5dd3f19fa0d85b9197

Comment 50

[Cameron McCormack (:heycam)]

Comment on attachment 8986411 [details]
Bug 282126 - Part 1: Allow getting zero glyph width from nsFontMetrics without falling back to average glyph width.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251774/diff/2-3/

Comment 51

[Cameron McCormack (:heycam)]

Comment on attachment 8986412 [details]
Bug 282126 - Part 2: Allow FontMetricsProvider to produce ex height and zero width independently.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251776/diff/2-3/

Comment 52

[Cameron McCormack (:heycam)]

Comment on attachment 8986413 [details]
Bug 282126 - Part 3: Don't compute ch to the average glyph width when there is no zero glyph.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251778/diff/2-3/

Comment 53

[Cameron McCormack (:heycam)]

Comment on attachment 8986414 [details]
Bug 282126 - Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251780/diff/3-4/

Comment 54

[Cameron McCormack (:heycam)]

Comment on attachment 8986415 [details]
Bug 282126 - Part 5: Tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251782/diff/3-4/

Comment 55

[Cameron McCormack (:heycam)]

Comment on attachment 8986411 [details]
Bug 282126 - Part 1: Allow getting zero glyph width from nsFontMetrics without falling back to average glyph width.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251774/diff/3-4/

Comment 56

[Cameron McCormack (:heycam)]

Comment on attachment 8986412 [details]
Bug 282126 - Part 2: Allow FontMetricsProvider to produce ex height and zero width independently.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251776/diff/3-4/

Comment 57

[Cameron McCormack (:heycam)]

Comment on attachment 8986413 [details]
Bug 282126 - Part 3: Don't compute ch to the average glyph width when there is no zero glyph.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251778/diff/3-4/

Comment 58

[Cameron McCormack (:heycam)]

Comment on attachment 8986414 [details]
Bug 282126 - Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251780/diff/4-5/

Comment 59

[Cameron McCormack (:heycam)]

Comment on attachment 8986415 [details]
Bug 282126 - Part 5: Tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251782/diff/4-5/

Comment 60

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e4bc368871bca600e7837bff53a7b2f198536b0

Comment 61

[Jonathan Kew [:jfkthame]]

Comment on attachment 8986411 [details]
Bug 282126 - Part 1: Allow getting zero glyph width from nsFontMetrics without falling back to average glyph width.

https://reviewboard.mozilla.org/r/251774/#review259000

LGTM, thanks!

::: gfx/thebes/gfxDWriteFonts.cpp:240
(Diff revision 4)
> -        mMetrics->spaceWidth = 0;
> -    } else {
>          mSpaceGlyph = glyph;
>          mMetrics->spaceWidth = MeasureGlyphWidth(glyph);
> +    } else {
> +        mMetrics->spaceWidth = 0;

Maybe change the constant to 0.0 while you're touching this line?

::: gfx/thebes/gfxFont.h:1678
(Diff revision 4)
>          gfxFloat spaceWidth;
> -        gfxFloat zeroOrAveCharWidth;  // width of '0', or if there is
> -                                      // no '0' glyph in this font,
> -                                      // equal to .aveCharWidth
> +        gfxFloat zeroWidth;  // -1 if there was no zero glyph
> +
> +        gfxFloat ZeroOrAveCharWidth() const
> +        {
> +          return zeroWidth >= 0 ? zeroWidth : aveCharWidth;

Again, 0.0 would be nice.

Comment 62

[Cameron McCormack (:heycam)]

There were some test failures on Windows and macOS I need to look into.

Comment 63

[Cameron McCormack (:heycam)]

Fixed the test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=134f142181a6606c09f196254cc7aaded7cd5cd9

Comment 64

[Cameron McCormack (:heycam)]

Attachment: Bug 282126 - Part 1: Allow getting zero glyph width from nsFontMetrics without falling back to average glyph width.

While we're here, fix the measurement of ' ' and 'x' so that we don't
measure the .notdef glyph if those glyphs aren't present.

Comment 65

[Cameron McCormack (:heycam)]

Attachment: Bug 282126 - Part 2: Allow FontMetricsProvider to produce ex height and zero width independently.

We are always able to produce an x height, but depending on whether the
glyph exists, we sometimes can't produce a zero glyph width.

Depends on D23423

Comment 66

[Cameron McCormack (:heycam)]

Attachment: Bug 282126 - Part 3: Don't compute ch to the average glyph width when there is no zero glyph.

Depends on D23424

Comment 67

[Cameron McCormack (:heycam)]

Attachment: Bug 282126 - Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always.

Depends on D23425

Comment 68

[Cameron McCormack (:heycam)]

Attachment: Bug 282126 - Part 5: Tests.

MozReview-Commit-ID: 1c0yMdUt7QG

Depends on D23426

Comment 69

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bf9ef4d7dfdd38da79fd73e0ba691aaf5181eec

Comment 70

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1d00af0de723650901bccec507fa78e135b7fa7

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/731c89a0319a
Part 1: Allow getting zero glyph width from nsFontMetrics without falling back to average glyph width. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/3948415fd493
Part 2: Allow FontMetricsProvider to produce ex height and zero width independently. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7416f3d9b117
Part 3: Don't compute ch to the average glyph width when there is no zero glyph. r=emilio
https://hg.mozilla.org/integration/autoland/rev/06ce1360f2ef
Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always. r=emilio
https://hg.mozilla.org/integration/autoland/rev/2cc3f72859cb
Part 5: Tests. r=jfkthame,emilio

Comment 72

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/731c89a0319a
https://hg.mozilla.org/mozilla-central/rev/3948415fd493
https://hg.mozilla.org/mozilla-central/rev/7416f3d9b117
https://hg.mozilla.org/mozilla-central/rev/06ce1360f2ef
https://hg.mozilla.org/mozilla-central/rev/2cc3f72859cb

Comment 73

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16012 for changes under testing/web-platform/tests

Comment 74

[Rachel Andrew]

Added to release notes for 68, updated docs page for length to match spec and implementation with regard to fallback for 0.