363706 - Support CSS3 'ch' unit for border and outline

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

Support CSS3 'ch' unit for border and outline

http://www.w3.org/TR/css3-values/#relative0

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 2

[Jesse Ruderman]

Loading the testcase gives me:

###!!! ASSERTION: GetOutlineWidth had no cached outline width: 'result', file /Users/jruderman/trunk/mozilla/layout/generic/nsFrame.cpp, line 3739

Comment 3

[Jesse Ruderman]

The testcase also seems to have infinite scrollbars.

Comment 4

[Zack Weinberg (:zwol)]

I had a look at this; it shouldn't be hard to implement - just need to tweak nsRuleNode.cpp::CalcLengthWidth() something like 

  switch (unit) {
    case eCSSUnit_Char: {
      nsFont font = aStyleFont->mFont;
      nscoord charwidth;
      nsCOMPtr<nsIFontMetrics> fm = aPresContext->GetMetricsFor(font);
      if (/* there is a zero digit in this font */) {
        fm->GetWidth("0", 1, charwidth, /* rendering context */);
      } else {
        // if there is no zero digit in this font, return the "average
        // character width".
        fm->GetAveCharWidth(charwidth);
      }
      return NSToCoordRound(aValue.GetFloatValue() * charwidth);
    }

However, note the /* */ comments.  I am not at all sure how to find out whether the font has a zero digit, this code hasn't got access to a rendering context (only a presentation context), and worst of all, ->GetWidth() is a method only of nsIThebesFontMetrics, which is private to gfx/src/thebes.

It is tempting to add a ->GetZeroCharWidth() to nsIFontMetrics.  Thoughts?

Tangentially, should this code maybe be using ->GetMaxAscent() for eCSSUnit_CapHeight?

Comment 5

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

See also nsLayoutUtils::CharsToCoord (which currently uses 'M' but should probably be changed to '0'), and its callers.

It would be really nice if we could get font metrics with a rendering context rather than a device context.

Comment 6

[Zack Weinberg (:zwol)]

Ok, I have a pair of patches for this bug.  Patch #2 depends on patch #1, and both are necessary; I split them up solely to ease review.

Patch #1 introduces a new gfxFont::Metrics field zeroOrAveCharWidth, which has exactly the definition CSS3 specifies for 'ch' - the width of the zero, or if there is no zero, the same value as .aveCharWidth.  This is then propagated upward as nsIFontMetrics:GetZeroOrAveCharWidth(), and CalcLengthWidth() is made to use it for eCSSUnit_Char.  Note that I cannot test the changes to any Thebes font backend except gfxPangoFonts.cpp, and have no fonts with no zero digit (that I know of) so the fallback to .aveCharWidth is also untested.

Patch #1 also adds GetMetricsFor() methods to nsIRenderingContext.  They just delegate to the associated device context.  CharsToCoord is made to use them.

Patch #2 then changes SetCoord() to not special case eCSSUnit_Char; it's treated like any other unit for which .IsLengthUnit() is true.  This renders *every single use* of eStyleUnit_Chars unnecessary, and in particular allows me to remove the NS_WARNING("Border unit set in chars; we don't handle that") from nsRuleNode::ComputeBorderData.  CharsToCoord can be deleted altogether (which means that the nsIRenderingContext GetMetricsFor() methods added in #1 are not used, but it sounds like they would be useful elsewhere).  GetAbsoluteCoord() becomes sufficiently simple that I made it an inline in nsLayoutUtils.h.  (It might make sense to strip all its now-unused parameters, but I wasn't sure if we mightn't ever want to put them back.)

Mats' testcase passes with these two patches applied.  Advice on how to write reftests for this would be appreciated.

This should also address bug 282126.

Comment 7

[Zack Weinberg (:zwol)]

Attachment: patch #1: add zeroOrAveCharWidth and nsIRenderingContext::GetMetricsFor

Comment 8

[Zack Weinberg (:zwol)]

Attachment: kill eStyleUnit_Chars

Comment 9

[Stuart Parmenter]

Comment on attachment 323316 [details] [diff] [review]
patch #1: add zeroOrAveCharWidth and nsIRenderingContext::GetMetricsFor

we shouldn't add new APIs to deprecated/old GFX apis.  new code written should be using thebes APIs.  there is an accessor on nsIThebesFontMetrics you can use to get the font group.  I don't see much purpose in caching the zero char advance for something that doesn't seem like it would get used very often.  We're probably better off just measuring it for now when needed (using the font group)

Comment 10

[Zack Weinberg (:zwol)]

I don't think I can measure '0' from the place that needs the value.  As far as I can tell, the way to do that would be (most easily) to use the ->GetWidth() method of nsIThebesFontMetrics, or (less easily) do what that method does internally, i.e. create an AutoTextRun from the string "0" and the font metrics, then call ->GetAdvanceWidth() on it.  Either approach requires a rendering context.  But the place that needs the value is nsRuleNode.cpp/CalcLengthWith(), which only has access to a style context and a presentation context - *not* a rendering context.

If instead we do cache the value in gfxFont::Metrics, then I could do

  tfm->GetFontGroup()->GetFontAt(0)->GetMetrics().zeroWidth;

without needing a rendering context.  (and then convert to layout units, but that's easy)

Comment 11

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

Comment on attachment 323317 [details] [diff] [review]
kill eStyleUnit_Chars

r+sr=dbaron (though this depends on the previous patch)

We should have a followup bug on getting rid of nsLayoutUtils::GetAbsoluteCoord.  It doesn't need all those parameters anymore.  The tests of its result can just be replaced with the GetUnit() == eStyleUnit_Coord check, and then places its out parameter was used can just use GetCoordValue().  That will probably actually simplify the callers...

Comment 12

[Zack Weinberg (:zwol)]

Attachment: revised patch to remove eStyleUnit_Chars

Here's a revised patch for the removal of eStyleUnit_Chars which doesn't depend on the other patch.  (It does mess with CalcLengthWith()'s handling of eCSSUnit_Char, but only to make it use average character width instead of the same number as 'em'.)  How do I get it checked in?

Comment 13

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

Given that the spec says to fall back to the average char width if there's no glyph for '0', it seems like we should put this in the metrics rather than trying to use the measurement API.  Or is there a good way to determine whether the first font has a 0?

Comment 14

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

We should get the first patch in before worrying about the second; it's better not to change behavior twice.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: revised patch to expose width of '0' to GetLengthWith()

And here's a revised patch to expose the width of '0', which does not mess with nsIFontMetrics.  It does still add a field to gfxFont::Metrics, for the reasons explained in comment 10.  Also I'm not sure I'm using nsCOMPtr correctly or in the most efficient way, and I still don't know how to write a test case for this.  (And, heck, am I using the r/sr flags correctly?)

Comment 16

[Zack Weinberg (:zwol)]

Comment on attachment 323794 [details] [diff] [review]
revised patch to expose width of '0' to GetLengthWith()

fixing r/sr fields.

And I should have mentioned that this patch now depends on the removal of eStyleUnit_Chars.

Comment 17

[Zack Weinberg (:zwol)]

Whups, crossed wires.  Shall I just merge the patches together?

Comment 18

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

Merging them is fine too (for me, anyway).

Comment 19

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

That said, probably easier for stuart to review just the one.

Comment 20

[Zack Weinberg (:zwol)]

I'll leave it till stuart gets a chance to look at it, and we can merge them for commit.

Comment 21

[Zack Weinberg (:zwol)]

Attachment: Re-revised patch to expose width of '0'

Crud, I forgot to 'hg qrefresh' before posting.  This patch is functionally the same as attachment #323794 [details] [diff] [review], but unlike that one, it compiles.

Comment 22

[Zack Weinberg (:zwol)]

(In reply to comment #11)
> We should have a followup bug on getting rid of
> nsLayoutUtils::GetAbsoluteCoord.

Done; bug 437335.

Comment 23

[Damon Sicore (:damons)]

Comment on attachment 323795 [details] [diff] [review]
Re-revised patch to expose width of '0'

Stuart is out for three weeks, so can you do this review, roc?

Comment 24

[Damon Sicore (:damons)]

wanted1.9.1+

Comment 25

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

Comment on attachment 323795 [details] [diff] [review]
Re-revised patch to expose width of '0'

+        gfxFloat zeroOrAveCharWidth;

The name is a little ambiguous so please document this.

Comment 26

[Zack Weinberg (:zwol)]

(In reply to comment #25)
> +        gfxFloat zeroOrAveCharWidth;
> 
> The name is a little ambiguous so please document this.

Will this do for docs?

  /* Width of U+0030 DIGIT ZERO, or if this font does not contain that
     character, the average character width (same as .aveCharWidth).  */

Comment 27

[Zack Weinberg (:zwol)]

also, could someone please positively confirm that I am using nsCOMPtr correctly, or else tell me what I got wrong?

Comment 28

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

Comment on attachment 323795 [details] [diff] [review]
Re-revised patch to expose width of '0'

>+      nsCOMPtr<nsIThebesFontMetrics> tfm(do_QueryInterface(fm));

this looks fine

>+      gfxFloat zeroWidth = tfm->GetThebesFontGroup()->GetFontAt(0)->GetMetrics().zeroOrAveCharWidth;

but I'd wrap this at less than 80 characters.

sr=dbaron with roc's comments addressed.

Hopefully I'll be able to land this for you later today...

Comment 29

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

(I have this queued up ready to land.)

Comment 30

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

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/f6b609b0af6e
http://hg.mozilla.org/mozilla-central/index.cgi/rev/68362ba98c23
http://hg.mozilla.org/mozilla-central/index.cgi/rev/b3578627a8c4

I reordered things back to the original order and added a simple reftest.

Comment 31

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

Also this to fix bustage:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/1c2c0b023699

Comment 32

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

This caused two reftest failures across platforms:

REFTEST UNEXPECTED FAIL: file:///builds/slave/darwin/build/layout/reftests/bugs/371043-1.html
REFTEST UNEXPECTED FAIL: file:///builds/slave/darwin/build/layout/reftests/bugs/391979.html

I didn't have time to investigate after everything cycled an hour and a half after I checked in, so I backed the whole thing out (except for the added test, which I marked failing).

Comment 33

[Zack Weinberg (:zwol)]

Both of the failing tests involve 'ch' units in some form.

371043 does not specify a fixed-width font but assumes 'ch' is the width of 'M'.  If I change 371043-1-ref.html to use 0 instead of M it passes.

391979 isn't failing for me and I don't see anything obviously wrong with it.

Revised patches coming shortly.

Comment 34

[Zack Weinberg (:zwol)]

Attachment: re-revised patch to remove eStyleUnit_Chars

Iteration three, patch 1/2: this version differs from the previous only in not touching nsRuleNode.cpp at all.  All it does is eliminate all uses of eStyleUnit_Chars.

Comment 35

[Zack Weinberg (:zwol)]

Attachment: Revised*3 patch to expose width of '0'

Iteration three, 2/2: incorporates David's fix to gfxWindowsFonts.cpp (you would think I could manage to cut and paste code correctly, but it seems not) and a correction to the reftest for bug 371043, per discussion above.  Also, I found a somewhat simpler way to describe the meaning of .zeroOrAveCharWidth.

I threw the combination of these patches at the try servers, and will see what happens.

Comment 36

[Zack Weinberg (:zwol)]

Comment on attachment 325920 [details] [diff] [review]
re-revised patch to remove eStyleUnit_Chars

marked the wrong patch obsolete, oops

Comment 37

[Zack Weinberg (:zwol)]

The try servers appear to run by default against CVS HEAD instead of moz-central, so they failed to apply my revised patch (which has a dependency on the test case that David added).

Comment 38

[Zack Weinberg (:zwol)]

... ok, now 391979 *is* failing for me.  Stay tuned.

Comment 39

[Zack Weinberg (:zwol)]

The 391979 reftest fails for me only with the additional patch in bug 437335.  I need to rev that patch anyway, but I'm now 99% sure there shouldn't be anything wrong with 391979 with the latest rev of the patches in *this* bug.

Comment 40

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

Another followup we can do is replace a number of nsStyleCoord in the style structs (e.g., mOutlineOffset, nsTextShadowItem::m{X,Y}Offset, and probably some others) with nscoord, since they only reason they used the union type was to carry the coord/chars distinction.

Comment 41

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

I filed comment 40 as bug 443057.

Comment 42

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

I have this queued up for relanding, and reftests pass locally.

Comment 43

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

Relanded:
http://hg.mozilla.org/index.cgi/mozilla-central/rev/0b1995eab10f
http://hg.mozilla.org/index.cgi/mozilla-central/rev/0b57ada5d4b2

Comment 44

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

Backed out again; the following tests failed on Windows:

REFTEST UNEXPECTED FAIL: file:///C:/builds/slave/win2k3_03/build/layout/reftests/bugs/363706-1.html
REFTEST UNEXPECTED FAIL: file:///C:/builds/slave/win2k3_03/build/layout/reftests/bugs/371043-1.html

Not sure if anything failed on non-Windows platforms.  (I haven't lined up pushlog times with tinderbox; you can do that tomorrow if you want.)

Comment 45

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

(And if you revise the patches, could you repost with the commit comments that I used, which note bug numbers and reviewers, so that it's easier for somebody else to reland the patches?)

Comment 46

[Zack Weinberg (:zwol)]

I'll look at this again after I get done with @font-face.

Comment 47

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

Looks like there were no Mac or Linux cycles that picked up the changes (hooray for slow unit test boxes).  But the tests did pass for me on Linux.

Comment 48

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

Attachment: first set of reftest failures from Windows unit test box

Comment 49

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

Attachment: second set of reftest failures from Windows unit test box

Comment 50

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

So the failures seen on tinderbox appear random; I can't reproduce them on Linux or Windows.

Comment 51

[Zack Weinberg (:zwol)]

So, how to proceed?  Mark those tests as random?

Comment 52

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

No, fix them.  (Perhaps by figuring out how to reproduce them, or perhaps not.)

Comment 53

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

(If necessary, we can commit printfs and see what they say on the actual test machines when the tests fail... or maybe we have unit test try servers by now, but I don't think so.)

Comment 54

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

That said, when I ran the entire reftest suite on Windows, I got the 391979.html failure (just like attachment 329593 [details]).  (I didn't see any failures when I ran just the tests showing the problems.)  So it seems like it could be related to long-term caching (e.g., the font cache making assumptions about the size of the metrics object?) or something.

Comment 55

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

I didn't see any failures after a run of the entire reftest suite on Linux.

Comment 56

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

Though, actually, I think the 391979.html failure is a bug in the test; it's making assumptions about how big a 'ch' unit is that are causing the bottoms of some of the 'g' characters to get lopped off.

Comment 57

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

I think I see what happened.  The tinderbox log that had the two failures that weren't caused by the broken reftest that I have fixed locally in my tree has this in the log:

pulling from http://hg.mozilla.org/mozilla-central
real URL is http://hg.mozilla.org/mozilla-central/
searching for changes
adding changesets
adding manifests
adding file changes
added 2 changesets with 21 changes to 20 files
(run 'hg update' to get a working copy)
8 files updated, 0 files merged, 0 files removed, 0 files unresolved
        1 file(s) copied.

The "8 files updated" suggests that it's trying to test only the first of the two changesets that I pushed.  This is the log of "WINNT 5.2 mozilla-central qm-win2k3-03 dep unit test on 2008/07/01 20:51:36  ".

I saw this behavior elsewhere... for example, on "MacOSX Darwin 9.2.2 mozilla-central qm-moz2mini01 dep unit test on 2008/07/01 22:10:36  " I see "TinderboxPrint: <a href=http://hg.mozilla.org/mozilla-central/index.cgi/rev/4f1b1553985c title="Built from revision 4f1b1553985c">rev:4f1b1553985c</a>" which is a changeset that was pushed along with a merge changeset on top of it.

(The windows tinderbox above didn't print what changeset it was testing.)


Furthermore, the following chunk:
-  else if (((aMask & SETCOORD_LENGTH) != 0) && 
-           (aValue.GetUnit() == eCSSUnit_Char)) {
-    aCoord.SetIntValue(NSToIntFloor(aValue.GetFloatValue()), eStyleUnit_Chars);
-  } 
is in the second patch instead of the first.  The lack of this chunk would be expected to cause the tests to fail.

So I think I'm going to reland, with:
 * a prior landing of a fix for 391979.html (which previously assumed that text was *never taller* than 2ch; I'll change that assumption to 4ch; ch are really width-ish units)
 * moving that chunk I mention from the second patch to the first.

Comment 58

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

Relanded:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/61f90cca8c3c
http://hg.mozilla.org/mozilla-central/index.cgi/rev/c0eaa1f9d904
http://hg.mozilla.org/mozilla-central/index.cgi/rev/572afe6e63fc

Comment 59

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

So I suspect a lot of the confusion was due to bug 442823.  I wish that had been publicized more broadly.

Comment 60

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

Looks like the landing stuck this time, finally.

Comment 61

[Zack Weinberg (:zwol)]

Thanks for seeing this through.  I don't think I would ever have figured out what had gone wrong on the tinderboxes.