Closed Bug 363706 Opened 14 years ago Closed 12 years ago

Support CSS3 'ch' unit for border and outline


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






(Reporter: mats, Assigned: zwol)


(Blocks 1 open bug)


(Keywords: assertion, css3, testcase)


(5 files, 5 obsolete files)

Support CSS3 'ch' unit for border and outline
Attached file Testcase #1
Assignee: dbaron → nobody
QA Contact: ian → style-system
Loading the testcase gives me:

###!!! ASSERTION: GetOutlineWidth had no cached outline width: 'result', file /Users/jruderman/trunk/mozilla/layout/generic/nsFrame.cpp, line 3739
Keywords: assertion
The testcase also seems to have infinite scrollbars.
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".
      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?
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.
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.
Attached patch kill eStyleUnit_Chars (obsolete) — Splinter Review
Assignee: nobody → zweinberg
Attachment #323317 - Flags: review?(dbaron)
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)
Flags: wanted1.9.1?
Priority: -- → P2
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


without needing a rendering context.  (and then convert to layout units, but that's easy)
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...
Attachment #323317 - Flags: superreview+
Attachment #323317 - Flags: review?(dbaron)
Attachment #323317 - Flags: review+
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?
Attachment #323317 - Attachment is obsolete: true
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?
We should get the first patch in before worrying about the second; it's better not to change behavior twice.
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?)
Attachment #323316 - Attachment is obsolete: true
Attachment #323794 - Flags: superreview?
Attachment #323794 - Flags: review?
Attachment #323316 - Flags: review?(dbaron)
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.
Attachment #323794 - Flags: superreview?(dbaron)
Attachment #323794 - Flags: superreview?
Attachment #323794 - Flags: review?(pavlov)
Attachment #323794 - Flags: review?
Whups, crossed wires.  Shall I just merge the patches together?
Merging them is fine too (for me, anyway).
That said, probably easier for stuart to review just the one.
I'll leave it till stuart gets a chance to look at it, and we can merge them for commit.
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.
Attachment #323794 - Attachment is obsolete: true
Attachment #323795 - Flags: superreview?(dbaron)
Attachment #323795 - Flags: review?
Attachment #323794 - Flags: superreview?(dbaron)
Attachment #323794 - Flags: review?(pavlov)
Attachment #323795 - Flags: review? → review?(pavlov)
Blocks: 437335
(In reply to comment #11)
> We should have a followup bug on getting rid of
> nsLayoutUtils::GetAbsoluteCoord.

Done; bug 437335.
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?
Attachment #323795 - Flags: review?(pavlov) → review?(roc)
Flags: wanted1.9.1? → wanted1.9.1+
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.
Attachment #323795 - Flags: review?(roc) → review+
(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).  */
also, could someone please positively confirm that I am using nsCOMPtr correctly, or else tell me what I got wrong?
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...
Attachment #323795 - Flags: superreview?(dbaron) → superreview+
(I have this queued up ready to land.)
Pushed to mozilla-central:

I reordered things back to the original order and added a simple reftest.
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1
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).
Resolution: FIXED → ---
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.
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.
Attachment #323792 - Attachment is obsolete: true
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.
Attachment #325920 - Attachment is obsolete: true
Comment on attachment 325920 [details] [diff] [review]
re-revised patch to remove eStyleUnit_Chars

marked the wrong patch obsolete, oops
Attachment #325920 - Attachment is obsolete: false
Attachment #323795 - Attachment is obsolete: true
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).
... ok, now 391979 *is* failing for me.  Stay tuned.
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.
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.
I have this queued up for relanding, and reftests pass locally.
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.1a1
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.)
Resolution: FIXED → ---
(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?)
I'll look at this again after I get done with @font-face.
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.
So the failures seen on tinderbox appear random; I can't reproduce them on Linux or Windows.
So, how to proceed?  Mark those tests as random?
No, fix them.  (Perhaps by figuring out how to reproduce them, or perhaps not.)
(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.)
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.
I didn't see any failures after a run of the entire reftest suite on Linux.
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.
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
real URL is
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= 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.
So I suspect a lot of the confusion was due to bug 442823.  I wish that had been publicized more broadly.
Looks like the landing stuck this time, finally.
Thanks for seeing this through.  I don't think I would ever have figured out what had gone wrong on the tinderboxes.
Blocks: css-values-3
You need to log in before you can comment on or make changes to this bug.