Support CSS3 'ch' unit for border and outline

RESOLVED FIXED in mozilla1.9.1a1

Status

()

defect
P2
minor
RESOLVED FIXED
13 years ago
7 years ago

People

(Reporter: mats, Assigned: zwol)

Tracking

(Blocks 1 bug, {assertion, css3, testcase})

Trunk
mozilla1.9.1a1
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

Reporter

Description

13 years ago
Support CSS3 'ch' unit for border and outline

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

Comment 1

13 years ago
Posted file Testcase #1
Assignee: dbaron → nobody
QA Contact: ian → style-system

Comment 2

12 years ago
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

Comment 3

11 years ago
The testcase also seems to have infinite scrollbars.
Assignee

Comment 4

11 years ago
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?
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.
Assignee

Comment 6

11 years ago
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.
Assignee

Comment 8

11 years ago
Posted patch kill eStyleUnit_Chars (obsolete) — Splinter Review
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Attachment #323317 - Flags: review?(dbaron)

Comment 9

11 years ago
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)
Assignee

Updated

11 years ago
Flags: wanted1.9.1?
Priority: -- → P2
Assignee

Comment 10

11 years ago
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 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+
Assignee

Comment 12

11 years ago
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.
Assignee

Comment 15

11 years ago
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)
Assignee

Comment 16

11 years ago
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?
Assignee

Comment 17

11 years ago
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.
Assignee

Comment 20

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

Comment 21

11 years ago
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)
Assignee

Updated

11 years ago
Attachment #323795 - Flags: review? → review?(pavlov)
Assignee

Updated

11 years ago
Blocks: 437335
Assignee

Comment 22

11 years ago
(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)
wanted1.9.1+
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+
Assignee

Comment 26

11 years ago
(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).  */
Assignee

Comment 27

11 years ago
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:
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.
Status: ASSIGNED → RESOLVED
Closed: 11 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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 33

11 years ago
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.
Status: REOPENED → ASSIGNED
Assignee

Comment 34

11 years ago
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
Assignee

Comment 35

11 years ago
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
Assignee

Comment 36

11 years ago
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
Assignee

Updated

11 years ago
Attachment #323795 - Attachment is obsolete: true
Assignee

Comment 37

11 years ago
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).
Assignee

Comment 38

11 years ago
... ok, now 391979 *is* failing for me.  Stay tuned.
Assignee

Comment 39

11 years ago
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.
Relanded:
http://hg.mozilla.org/index.cgi/mozilla-central/rev/0b1995eab10f
http://hg.mozilla.org/index.cgi/mozilla-central/rev/0b57ada5d4b2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 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.)
Status: RESOLVED → REOPENED
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?)
Assignee

Comment 46

11 years ago
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.
Assignee

Comment 51

11 years ago
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 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.
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.
Assignee

Comment 61

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

Updated

7 years ago
Blocks: css-values-3
You need to log in before you can comment on or make changes to this bug.