472195 - support css3 root em ('rem' or 're') units

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P4)

Description

[K R]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2008121622 Ubuntu/8.10 (intrepid) Firefox/3.0.5
Build Identifier: 

CSS3 defines "rem" -- root em -- units as the font size of the root element. This will be very useful for style sheet authors. I have a patch implementing rem units; I'll attach it momentarily.

Reproducible: Always

Comment 1

[K R]

Attachment: Simple test case for rem support

Comment 2

[K R]

Attachment: CSS3 rem units support

This patch is against hg head. Let me know if anything is wrong and I'll try to fix it.

Comment 3

[K R]

Attachment: CSS3 rem units support (cleanup)

This is a cleaned up version of the previous patch. It has exactly the same functionality; I just removed some commented out code.

Comment 4

[Tyler Downer [He/Him]]

Comment on attachment 355459 [details] [diff] [review]
CSS3 rem units support (cleanup)

David is the right guy for this.

Comment 5

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

Comment on attachment 355459 [details] [diff] [review]
CSS3 rem units support (cleanup)

Thanks for working on this.  Other than one issue below, the patch looks good codewise.  However, we don't want to commit this until the spec is stable enough for us to ship an implementation of it.  (And the unit probably needs to be renamed; see below.)

>+    case eCSSUnit_REM: {
>+      nsRefPtr<nsStyleContext> rootStyle;
>+      nsIContent* docElement = aPresContext->Document()->GetRootContent();
>+      rootStyle = aPresContext->StyleSet()->ResolveStyleFor(docElement, nsnull);
>+      if (rootStyle) {
>+        aStyleFont = rootStyle->GetStyleFont();
>+        aFontSize = aStyleFont->mFont.size;
>+      }
>+    }
>+    // Fall through...


So this bit has a few problems.

The worst of them is that I think if you specify a length in 'rem' on the root element, particularly for the 'font' property (or any other property in the nsStyleFont struct, if any others accept lengths), I think this will go into an infinite loop.

I'm also prefer avoiding code that changes function parameters.  And in this case since it's easy to avoid falling through cases, I'd prefer avoiding that as well.  (You could factor out the NSToCoordRoundWithClamp(aValue.GetFloatValue() * float(aFontSize)) into its own inline function used for this unit, 'em', 'ex', and 'ch'.)

I was a little surprised by the approach of resolving a *new* style context for the root element, but I think it's actually probably the best way to go about things here.


The other issue here is whether the spec is currently stable enough to be implemented.  Given that the unit has been renamed in the current editor's draft:
http://dev.w3.org/csswg/css3-values/#the-re
which also clarifies what the correct behavior is in the case above, I sort of suspect it's not quite ready.  (I'd like it to be, though.)


Another issue where the spec could use some clarification (although where I tend to think what you implemented is the correct behavior) is the question of which document's root element should be used.  For example, in SVG, things can be pulled in from other resource documents; in those cases, the root element doesn't even necessarily have a font size, or it could have more than one if it's used in different places.  So I think we really do want the root of the root document in that case (which is what you've done), although I think the spec could be clearer.  (This is different from frames, where we should, and your code does, use the root of the document inside the frame/iframe, not the root of the root document.  I think, anyway.)


The final issue is tests.  New features should have tests added to our test suites (either mochitest or reftest; in this case either would work) to test that they work correctly and that future changes don't break them.  All of the cases I mentioned above (rem on the root element, rem on a non-root element where it's different from 'em', rem on a document in an iframe in a way that tests which root element is used, rem in an SVG resource document in a way that tests which root element is used).  For more information on mochitest and reftest, see https://developer.mozilla.org/En/Mozilla_automated_testing

Comment 6

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

The mailing list for spec discussion is http://lists.w3.org/Archives/Public/www-style/

Comment 7

[K R]

Thanks for this amazingly detailed help. I'll do all the changes you asked for and post a new patch in the next day or two.

As for whether the spec is ready or not, I'll go ahead and ask in www-style. Either way, I'm happy to tend this patch until you feel it's safe to include.

Comment 8

[K R]

For those not reading www-style...

Håkon Wium Lie wrote:
> The CSS WG discussed the issue today and decided to keep the
> three-letter "rem" name.

So I'll keep that name in this patch.

I also asked about the interpretation of "root element" and again about spec stability in general.

Comment 9

[K R]

Attachment: CSS3 rem units support (work in progress)

This doesn't have tests yet (I'm having trouble getting "firefox -reftest ..." to run without segfaulting), but otherwise I think it's complete and correct. I'm just posting it now as a preview in case there's more feedback on my work.

What I changed since the first patch:

 * Removed the GK_ATOM(rem_, "rem") line from nsGkAtomList.h, since it seemed to be redundant with the line I added. (Or should I just have made no change to that file at all?)

 * Renamed eCSSUnit_REM to eCSSUnit_RootEM to be more consistent with the other spelled-out names.

 * Factored out the NSToCoordRoundWithClamp() calls from CalcLengthWith().

 * Made the root em case not modify the params.

 * Made the root em case not fall through. FWIW, I only had it fall through in the first place so the generated code would be (a tiny bit) smaller; I didn't realize mozilla's prevailing style was not to fall through.

 * Introduced a new parameter to CalcLengthWith(). That function needs to know if the value it's computing is for the root element, in which case it can just use the provided aFontSize. This information is passed in via a new parameter to SetFontSize() and SetFont(). I don't know if there's a better way to do it. (Maybe one of the existing params to CalcLengthWith() already carries the necessary information... It doesn't seem so.)

Still to come:

 * Tests!

Comment 10

[K R]

Attachment: CSS3 rem units support with tests

Changes since previous patch:

 * Fixed my bug that was causing the segfault. I had incorrect logic to decide if the current element was the root element, causing the same infinite recursion from my first patch.

 * Wrote tests.

Comment 11

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

Thanks for updating the patch so quickly, and sorry I didn't get to it equally quickly.  I just took a brief look at it, and I'll try to get through the rest tomorrow.  Things I've noticed so far:

I think the tests should probably use a non-default font-size on the root element (and then a different non-default font size on body), so that they're really testing that the font size is coming from the correct element.

The way you set |atRoot| should probably be based on whether the context has a null parent context rather than by comparing font to parentFont (etc.), since the latter difference is actually quite complicated (and depends on what properties in the font struct were specified and what values they had).

In the places where atRoot is a function parameters, it should probably be aAtRoot to match the existing style of naming function parameters with "a" (for argument), (members with m, globals with g or s, constants with k, etc.).


There are two messier issues that I need to look into tomorrow (and possibly write testcases explaining what the problem is, if in fact they are problems):
 * Do we need to set aInherited in CalcLengthWith? (and if so, when)
 * Do we need to change CheckFontCallback?
(My gut feeling is that the answer to the first is yes and the second is no, but I need to investigate further.)

I also need to look at the MathML stuff a little more closely.

Comment 12

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

(In reply to comment #11)
> The way you set |atRoot| should probably be based on whether the context has a
> null parent context rather than by comparing font to parentFont (etc.), since
> the latter difference is actually quite complicated (and depends on what
> properties in the font struct were specified and what values they had).

And this means that you also don't need to pass atRoot to SetFont and SetGenericFont, since they have the context and you can figure it out there.


It's late, and I still haven't looked at the other two issues...

Comment 13

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

Comment on attachment 356340 [details] [diff] [review]
CSS3 rem units support with tests

(In reply to comment #11)
>  * Do we need to set aInherited in CalcLengthWith? (and if so, when)

Er, never mind this one; CalcLengthWith already sets aInherited at exactly the right point.

>  * Do we need to change CheckFontCallback?

No, since it already tests IsRelativeLengthUnit(), which covers RootEM correctly.

> I also need to look at the MathML stuff a little more closely.

That looks fine, modulo the other changes (setting atRoot differently).


Sorry it took so long to finish reviewing this.  In any case, I think it should be ready to go given the changes in comment 11 and comment 12.  (The consensus on www-style was that this is ok to implement, right?  In any case, it'll be a little while before we ship 1.9.2 anyway...)

Comment 14

[K R]

No problem. Yes, it's okay to implement according to www-style. I'll have a new patch soon.

Comment 15

[K R]

Attachment: CSS3 rem units support with tests

Changes since previous patch:

 * Set atRoot from aContext->GetParent()

 * Removed atRoot parameter from SetFont and SetGenericFont

 * Renamed atRoot to aAtRoot in SetFontSize

 * Simplified the tests a little by splitting one of the reference pages into
   three.

 * Set different non-default font sizes on the root and body elements in the
   tests.

 * Changed the tests to use px everywhere except for the value under test,
   which uses rem.

 * Added my name to the Contributors list of nsRuleNode.cpp. I hope that's
   okay.

Comment 16

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

I applied the patch in my tree and then reviewed it there, so I have it ready to land for the next time I push stuff.

I made the following changes in my tree:

 * merged with the patch from bug 473892, which also added a parameter to CalcLengthWith

 * changed the name of the parameter from useProvidedRootEmSize to aUseProvidedRootEmSize to match local style

 * changed nsRuleNode::CalcLengthWithInitialFont to pass PR_TRUE for aUseProvidedRootEmSize, since the point of CalcLengthWithInitialFont is to calculate a length with the default values of the font properties (i.e., ignoring styles on the root element).  Otherwise there would be an infinite loop bug similar to bug 473892 with media queries and the 'rem' unit, I think.  (I should probably add a test for that before I land, and confirm that that's the case.)

 * Added a second reference for unit-rem-root-fontsize.html, since 'em' on the root could potentially break at the same time.  (The second reference uses 'em' on body.)  This is in addition to the first reference.

 * Fixed unit-rem.svg to refer to unit-rem-resource.svg#rect rather than unit-rem-ref-resource.svg#rect .  Without this change, the test and reference are identical.

 * Fixed unit-rem-resource.svg to avoid using rem units in an SVG attribute (since otherwise the previous change would make the test fail), since we haven't added rem support to nsSVGLength (although maybe we should).  In other words, I changed:
  width="10rem"
to:
  style="font-size: 1rem" width="10em"
and added a comment explaining it.


Do all those changes make sense to you?



I'd also note that in the future, it would be good if you added to the [diff] section of your ~/.hgrc the lines:
git = 1
showfunc = 1
(in addition to the unified = 8 that I think you already have).

Comment 17

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

Attachment: CSS3 rem units support with tests

Here's the merged patch per the previous comment.

Comment 18

[K R]

Yes, that all makes sense. Again, thanks. Next time I'll be sure not to make those mistakes.

Comment 19

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

(In reply to comment #18)
> Yes, that all makes sense. Again, thanks. Next time I'll be sure not to make
> those mistakes.

Well, it's why we have review.  It's pretty good for a first patch.  The best way to learn what the expectations for code in a particular project are is through experience.

I landed your patch (with the revisions):
http://hg.mozilla.org/mozilla-central/rev/23d1cadf9e8d
and my additional tests:
http://hg.mozilla.org/mozilla-central/rev/adf44ec71ce9
in mozilla-central, so this is fixed for 1.9.2a1.

Perhaps we can interest you in working on something else?

Comment 20

[Eric Shepherd [:sheppy]]

There's a little mention of this on the Firefox 3.6 for developers page, but we need to properly document it.