281972 - A couple of -moz-outline-offset problems

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mats Palmgren (inactive)]

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.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 2

[Anne (:annevk)]

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>.

Comment 3

[Mats Palmgren (inactive)]

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

Comment 4

[Mats Palmgren (inactive)]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Testcase #2

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...

Comment 7

[Mats Palmgren (inactive)]

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

Comment 8

[Aaron Leventhal]

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

Comment 9

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

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

Comment 10

[Mats Palmgren (inactive)]

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'."

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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 [e-mail]?

> 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).

Comment 12

[Mats Palmgren (inactive)]

> > 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 [e-mail]?

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Mats Palmgren (inactive)]

(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

Comment 15

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

Attachment: fix

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

Comment 16

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

Attachment: testcase

Comment 17

[Mats Palmgren (inactive)]

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

Comment 18

[Mats Palmgren (inactive)]

Attachment: Another fix

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...

Comment 19

[Mats Palmgren (inactive)]

I have asked [e-mail] what the computed value for 'offset-width' should
be when 'offset-style' is 'none'. In my patch I assumed the answer is zero...

Comment 20

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

(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 21

[Boris Zbarsky [:bzbarsky]]

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?  ;)

Comment 22

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

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.

Comment 23

[Asaf Romano (gone)]

(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)?

Comment 24

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

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 25

[Mats Palmgren (inactive)]

Comment on attachment 174261 [details] [diff] [review]
Another fix

I'll add a new patch tomorrow.

Comment 26

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

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 27

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

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.

Comment 28

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

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'.

Comment 29

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

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?

Comment 30

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

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

Comment 31

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

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 32

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

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 33

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

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 34

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

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 35

[Mats Palmgren (inactive)]

Attachment: Final patch that was checked in (rev.3 + comments)

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

Comment 36

[Mats Palmgren (inactive)]

Checked in 2005-03-27 03:36 PDT

-> FIXED