Closed
Bug 281972
Opened 20 years ago
Closed 20 years ago
A couple of -moz-outline-offset problems
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: testcase)
Attachments
(5 files, 3 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•20 years ago
|
||
Comment 2•20 years ago
|
||
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>.
Assignee | ||
Comment 3•20 years ago
|
||
That does not seem appropriate to me, given that this is an *offset* and not
a *width*.
Assignee | ||
Comment 4•20 years ago
|
||
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•20 years ago
|
||
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.
Assignee | ||
Comment 6•20 years ago
|
||
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...
Assignee | ||
Comment 7•20 years ago
|
||
Comment 8•20 years ago
|
||
AAs long as outline-offset still accepts negative values. That's extremely useful.
Comment 9•20 years ago
|
||
It accepts whatever the spec says it accepts. If you don't like that, bring it
up on the appropriate mailing list, not here.
Assignee | ||
Comment 10•20 years ago
|
||
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•20 years ago
|
||
> 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).
Assignee | ||
Comment 12•20 years ago
|
||
> > 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.
Comment 13•20 years ago
|
||
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.
Assignee | ||
Comment 14•20 years ago
|
||
(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)
Attachment #174256 -
Flags: superreview?(bzbarsky)
Attachment #174256 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 17•20 years ago
|
||
He, I was just about to attach a patch for this...
Assignee | ||
Comment 18•20 years ago
|
||
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...
Assignee | ||
Comment 19•20 years ago
|
||
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 21•20 years ago
|
||
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? ;)
Attachment #174256 -
Flags: superreview?(bzbarsky)
Attachment #174256 -
Flags: superreview-
Attachment #174256 -
Flags: review?(bzbarsky)
Attachment #174256 -
Flags: review-
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•20 years ago
|
||
(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•20 years ago
|
||
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-.
Assignee | ||
Comment 25•20 years ago
|
||
Comment on attachment 174261 [details] [diff] [review]
Another fix
I'll add a new patch tomorrow.
Attachment #174261 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #174256 -
Attachment is obsolete: true
Assignee | ||
Comment 26•20 years ago
|
||
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
Assignee | ||
Updated•20 years ago
|
Attachment #178676 -
Flags: superreview?(dbaron)
Attachment #178676 -
Flags: review?(dbaron)
Comment 27•20 years ago
|
||
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.
Attachment #178676 -
Flags: superreview?(dbaron)
Attachment #178676 -
Flags: superreview-
Attachment #178676 -
Flags: review?(dbaron)
Attachment #178676 -
Flags: review-
Comment 28•20 years ago
|
||
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'.
Assignee | ||
Comment 29•20 years ago
|
||
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?
Attachment #178676 -
Attachment is obsolete: true
Attachment #178681 -
Flags: superreview?(dbaron)
Attachment #178681 -
Flags: review?(dbaron)
*** Bug 6647 has been marked as a duplicate of this bug. ***
Comment 31•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
Comment on attachment 178681 [details] [diff] [review]
Patch rev. 3
r+sr=dbaron given the changes in comment 31, comment 32, and comment 33
Attachment #178681 -
Flags: superreview?(dbaron)
Attachment #178681 -
Flags: superreview+
Attachment #178681 -
Flags: review?(dbaron)
Attachment #178681 -
Flags: review+
Assignee | ||
Comment 35•20 years ago
|
||
comment 31, 32: Fixed
comment 33:
Yes, added NS_NOTYETIMPLEMENTED here and in nsStyleOutline::GetOutlineOffset().
Assignee | ||
Comment 36•20 years ago
|
||
Checked in 2005-03-27 03:36 PDT
-> FIXED
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•