1. When using DOMI to inspect computed values I frequently see: WARNING: Double check the unit, file nsComputedDOMStyle.cpp, line 1411 on the console. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/style/src/nsComputedDOMStyle.cpp&rev=1.134&root=/cvsroot&mark=1403#1376 2. CSS3 UI, http://www.w3.org/TR/css3-ui/#outline-offset says: Name: outline-offset Value: <length> | inherit but Mozilla accepts '-moz-outline-offset:medium' for example. Is this intentional? I have a fix for 1 but can fix 2 at the same time if you confirm it's a bug.

I guess outline-offset will going to be like the properties are defined in CSS 2.1. So as <border-width>: <http://w3.org/TR/CSS21/box.html#value-def-border-width>.

That does not seem appropriate to me, given that this is an *offset* and not a *width*.

I also would prefer NS_ASSERTION instead of NS_WARNING in all the places where we get an unexpected unit in this file, because that should not happen for any input/operation.

Unless Robert and Aaron had really good reasons for doing outline-offset this way (and I don't see any in bug 251498), it looks like the patch for bug 251498 was just wrong; it implemented nothing remotely resembling the spec it linked to... See also bug 251498 comment 12 for the list of things that'd need fixing if we fix this to follow the spec.

There are more bugs I'm afraid... There is code to handle border-width and -moz-outline-width values of type eStyleUnit_Integer, eStyleUnit_Proportional and eStyleUnit_Chars. Integer/Proportional cannot occur but for eStyleUnit_Chars it does not seem correct to interpret it as an enum value. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/style/src/nsComputedDOMStyle.cpp&rev=1.134&root=/cvsroot&mark=3355,3376-3377,3379#3355 Can someone explain why we have this non-CSS unit? It seems like it's just a synonym for 'em'? http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/style/src/nsRuleNode.cpp&rev=3.24&root=/cvsroot&mark=156,168,169#156 (It's accepted by the parser for *all* <length> type properties AFAICT) I would appreciate any guidance on how to handle this unit. Regarding '-moz-outline-style' the spec also says that 'auto' should be accepted (unlike 'border-style'): http://www.w3.org/TR/css3-ui/#outline-style We don't do that... Also, we should not accept 'hidden', which we currently do. Regarding '-moz-outline-width' we currently accept negative values, this does not seem right (it triggers warnings during reflow). Also, I wonder if the computed value should be zero when the '-moz-outline-style' is 'none' - like computed 'border-width'? (The spec does not say anything regarding this difference so I wonder if it is an oversight in the spec?) And there are bugs in '-moz-outline-radius' also... more on that later or perhaps in a different bug...

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/style/src/nsComputedDOMStyle.cpp&rev=1.134&root=/cvsroot&mark=1316,1330,1331,1333#1315

AAs long as outline-offset still accepts negative values. That's extremely useful.

It accepts whatever the spec says it accepts. If you don't like that, bring it up on the appropriate mailing list, not here.

Do you have a preference on how '-moz-outline-style:auto' should be rendered? This is what the spec says: "The 'auto' value permits the user agent to render a custom outline style, typically a style which is either a user interface default for the platform, or perhaps a style that is richer than can be described in detail in CSS, e.g. a rounded edge outline with semi-translucent outer pixels that appears to glow. As such, this specification does not define how the 'outline-color' is incorporated or used (if at all) when rendering 'auto' style outlines. User agents may treat 'auto' as 'solid'."

> Can someone explain why we have this non-CSS unit? I was just wondering that, the other day.. it's not supported for anything but widths in HTML, really... File a separate bug on that? The outline-style bugs sound like they need fixing (more broken copy-paste). > Regarding '-moz-outline-width' we currently accept negative values I don't see any exclusions of those in the spec, but that seems like a bug in the spec. Let's fix that in Gecko and raise the issue in www-style@w3.org? > Also, I wonder if the computed value should be zero Again, www-style issue. > Do you have a preference on how '-moz-outline-style:auto' should be rendered? I'd render as 'solid' for now unless we have strong reasons not to (eg on Mac? I recall that coming up).

> > Regarding '-moz-outline-width' we currently accept negative values > > I don't see any exclusions of those in the spec, but that seems like a bug in > the spec. Let's fix that in Gecko and raise the issue in www-style@w3.org? I think it's clear that negative values are not accepted: http://www.w3.org/TR/css3-ui/#outline-width says "Value: <border-width> | inherit" where <border-width> is a link to CSS2.1 definition which says: http://www.w3.org/TR/CSS21/box.html#value-def-border-width "<length> The border's thickness has an explicit value. Explicit border widths cannot be negative." And for '-moz-outline-offset' the definition is: http://www.w3.org/TR/css3-ui/#outline-offset "Value: <length> | inherit" and there is no text to exclude negative values there.

Ah, I missed it saying <border-width>. Yes, we should exclude negative values there; in fact, we should just parse it just like we do borde-width, sounds like.

(In reply to comment #11) > > Can someone explain why we have this non-CSS unit? > > ... it's not supported for anything but > widths in HTML, really... File a separate bug on that? Filed bug 282126

Fixes the outline-specific issues raised here (so far)

He, I was just about to attach a patch for this...

I like my patch better ;-) I think your patch will still assert on the 'ch' unit. Also, I tried hard to avoid adding 'auto' into the enum values. I think it makes a difference but maybe not...

I have asked www-style@w3.org what the computed value for 'offset-width' should be when 'offset-style' is 'none'. In my patch I assumed the answer is zero...

(In reply to comment #18) > I think your patch will still assert on the 'ch' unit. Yeah, I was hoping you'd fix all those in your other bug :-). > Also, I tried hard to avoid adding 'auto' into the enum values. I think it > makes a difference but maybe not... I don't see the point of avoiding adding 'auto' to kOutlineStyleKTable. Actually I would like to have NS_STYLE_BORDER_STYLE_AUTO, because then we can work towards making it draw a native focus ring on Mac and elsewhere, like Safari does.

Comment on attachment 174256 [details] [diff] [review] fix We generally avoid "auto" in the tables because it'd bloat a whole bunch of them. Just parse the outline style as VARIANT_HOK | VARIANT_AUTO as you do. Note that if you do that, having "auto" in the keyword table won't do you a bit of good, since the VARIANT_AUTO check causes "auto" to SetAutoValue() on the CSSValue and return. Then in the rulenode you need to deal with "auto" (map it into the desired value in the style struct). >Index: layout/style/nsRuleNode.cpp >+ } else { >+ outline->mOutlineOffset = 0; >+ } That seems wrong... What if the outline offset is just not specified in this rule? Should we really be nuking the value in the computed style then? Mats and Robert, sort out between the two of you who'll fix whihc part, ok? ;)

Since Mats likes his patch better, I'll let him do it :-) As long as he turns 'auto' into NS_STYLE_BORDER_STYLE_AUTO the way my patch does.

(In reply to comment #11) > > Do you have a preference on how '-moz-outline-style:auto' should be rendered? > > I'd render as 'solid' for now unless we have strong reasons not to (eg on Mac? I > recall that coming up). I'm confused, does outline-style:auto apply to the 'style' part only (e.g. solid) or to outline* (e.g. solid 1px red)?

What's the status of this? It would be good to fix these for 1.8, and if the support is good enough, maybe even remove the -moz-.

Comment on attachment 174261 [details] [diff] [review] Another fix I'll add a new patch tomorrow.

I got no respons from www-style regarding the computed outline-width. Meanwhile, a new CSS3 "Backgrounds and Borders" draft was published, which changes the computed value for 'border-width', it says: http://www.w3.org/TR/2005/WD-css3-background-20050216/#the-border-width "Computed value: specified value" without an exception for 'border-style:none' as CSS2.1 have: http://www.w3.org/TR/CSS21/box.html#value-def-border-width "Computed value: absolute length; '0' if the border style is 'none' or 'hidden'" So I changed both 'border-width' and 'outline-width' to use the CSS3 definition. 'auto' is now mapped into NS_STYLE_BORDER_STYLE_AUTO as per roc's request, but without adding it to 'kOutlineStyleKTable'. AFAICT, -moz-outline-color/style/width/offset are now implemented according to spec so I dropped the -moz- prefix. (I kept the prefix for outline-radius). I added aliases though for backward compat, the same for DOM. (IMO, we can drop the aliases post-1.8) Changed files: editor/composer/src/res/EditorOverride.css toolkit/themes/qute/communicator/formatting.css toolkit/themes/qute/global/formatting.css toolkit/themes/qute/global/tabbox.css toolkit/themes/qute/mozapps/extensions/extensions.css toolkit/themes/winstripe/global/formatting.css toolkit/themes/winstripe/global/tabbox.css toolkit/themes/winstripe/mozapps/extensions/extensions.css toolkit/themes/pinstripe/global/formatting.css toolkit/themes/pinstripe/mozapps/extensions/extensions.css layout/forms/resources/content/xbl-forms.css layout/mathml/tests/various.xml themes/modern/communicator/formatting.css themes/classic/communicator/formatting.css themes/classic/global/win/tabbox.css mail/themes/qute/mail/messageBody.css mail/themes/pinstripe/mail/messageBody.css layout/style/ua.css layout/style/forms.css layout/style/html.css the above is just -moz-outline* => outline* property changes dom/public/idl/css/nsIDOMCSS2Properties.idl layout/base/nsCSSRendering.cpp layout/base/nsPresShell.cpp layout/base/nsStyleConsts.h layout/style/nsCSSDeclaration.cpp layout/style/nsCSSParser.cpp layout/style/nsCSSPropList.h layout/style/nsCSSProps.cpp layout/style/nsCSSProps.h layout/style/nsCSSStruct.cpp layout/style/nsComputedDOMStyle.cpp layout/style/nsDOMCSSDeclaration.cpp layout/style/nsRuleNode.cpp layout/style/nsStyleStruct.cpp layout/style/nsStyleStruct.h

Comment on attachment 178676 [details] [diff] [review] Patch rev. 2 Assume CSS2.1 is correct and css3 is wrong unless told otherwise. CSS2.1 has had much more careful review.

That said, what nsComputedDOMStyle should be using is the CSS2.1 "used value", which corresponds to the CSS2.0 "computed value". See http://www.w3.org/TR/2004/CR-CSS21-20040225/cascade.html#used-value . It's important for interface compatibility, since people rely on layout-dependent lengths to be converted to absolute lengths in getComputedStyle. However, that's not relevant to 'border-width' or 'outline-width' -- they should definitely compute to zero when the relevant '*-style' is 'none'.

Ok, I reverted that particular change for 'border-width' and made 'outline-width' behave the same. >That said, what nsComputedDOMStyle should be using is the CSS2.1 "used value", >which corresponds to the CSS2.0 "computed value". Hmm, so the nsComputedDOMStyle methods returning enum widths are wrong then?

*** Bug 6647 has been marked as a duplicate of this bug. ***

Comment on attachment 178681 [details] [diff] [review] Patch rev. 3 >Index: dom/public/idl/css/nsIDOMCSS2Properties.idl >@@ -271,16 +271,19 @@ interface nsIDOMCSS2Properties : nsISupp Don't add to nsIDOMCSS2Properties. That's a frozen DOM interface. Add to nsIDOMNSCSS2Properties (in the CSS3 properties section), and rev its IID.

Comment on attachment 178681 [details] [diff] [review] Patch rev. 3 >Index: layout/base/nsCSSRendering.cpp Put a case (with an assertion) at the end of DrawTableBorderSegment as well. >Index: layout/style/nsCSSParser.cpp >-#define ENABLE_OUTLINE // un-comment this to enable the outline properties (bug 9816) >- // XXX un-commenting for temporary fix for nsbeta3+ Bug 48973 >- // so we can use "mozoutline >+#define ENABLE_OUTLINE_RADIUS // comment this to disable the -moz-outline-radius properties Don't bother. Just remove the ifdefs. >Index: layout/style/nsCSSProps.cpp >+// Note that 'auto' is also a valid value and is handled in nsRuleNode.cpp. Don't say that, since it's true for tons of other properties. If somebody wants to find that out, they look in nsCSSParser.cpp or nsRuleNode.cpp. (And so is 'none', which you didn't mention.) >Index: layout/style/nsCSSStruct.cpp up to here

Comment on attachment 178681 [details] [diff] [review] Patch rev. 3 >Index: layout/style/nsStyleStruct.cpp >@@ -298,16 +299,19 @@ static nscoord CalcCoord(const nsStyleCo >+ case eStyleUnit_Chars: >+ // XXX we need a frame and a rendering context to calculate this, bug 281972, bug 282126. >+ return 0; > default: > NS_ERROR("bad unit type"); > break; This should probably still assert (NS_NOTYETIMPLEMENTED), no?

Comment on attachment 178681 [details] [diff] [review] Patch rev. 3 r+sr=dbaron given the changes in comment 31, comment 32, and comment 33

comment 31, 32: Fixed comment 33: Yes, added NS_NOTYETIMPLEMENTED here and in nsStyleOutline::GetOutlineOffset().

Checked in 2005-03-27 03:36 PDT -> FIXED