713613 - Do a better job of serializing 'border: 1px solid silver; border-top: none;" using border-* shorthands

Status: NEW

(Core :: CSS Parsing and Computation, defect, P5)

Description

[Sebastian Zartner [:sebo]]

Attachment: border style is expanded when there's a border-top style defined

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

See http://code.google.com/p/fbug/issues/detail?id=5070. I attached its test case here.


Actual results:

In the test case the different border styles are listed like shown in the screenshot attached to the issue.


Expected results:

The styles should be exposed as defined, so just "border" and "border-top" will be seen in Firebug.

Sebastian

Comment 1

[Boris Zbarsky [:bzbarsky]]

Shorthand properties are always expanded internally.

When serializing the declaration, we recollapse them if we can figure out how to do it in a _single_ shorthand.  In this case we can't, so we don't.

I suspect this is wontfix, but dbaron should make that call.

Comment 2

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

It's probably easily fixable in a bunch of cases, but this is one of the hard ones.  I'm not sure this is immediately WONTFIX, though.

Comment 3

[Leon Sorokin]

i think a related issue is indicating whether each property is implicit (computed) vs explicitly defined in css. i dunno if you guys expose this in the api (probably, since there's a "computed" tab) but in fbug shorthands expand from eg:

div {
    background: none;
}

to

div {
    background: none repeat scroll 0 0 transparent;
}

if this is unavoidable, it would be helpful to at least make the computed properties show as a different color (silver maybe). i'm not sure if i should open an fbug issue or an FF issue, i'm thinking fbug.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> i dunno if you guys expose this in the api

We don't.  "background: none" is expanded to "background: none repeat scroll 0 0 transparent" at parse time, and only the latter form is stored.  So at that point we don't know whether it was originally the full form, or "background: none" or "background: transparent" or some other version of the thing.  They're all stored exactly the same way.  This is distinct from computed value; the parse-time expansion affects specified values.

Comment 5

[Leon Sorokin]

hmmm, makes debugging harder. no me gusta :(

would it be possible to store some kind of bitmask alongside the expanded versions that would indicate which were actually parsed and which were defaulted?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Possible, yes, at the cost of memory bloat and more code complexity and so forth....

Comment 7

[Leon Sorokin]

i'd think the introspection benefits outweigh the overhead by a good margin, though. while to the end user it doesn't matter whether a rule is "computed" or "default". but the current situation is that firebug presents defaults as if they were explicit.

this discussion is bordering on hijacking. i opened Bug 713760 for this specific concern. is there a way to transfer over relevant comments?

thx

Comment 8

[Boris Zbarsky [:bzbarsky]]

> i'd think the introspection benefits outweigh the overhead by a good margin,

For the small minority of users using a debugger.  For the rest, the overhead is pure loss.

> is there a way to transfer over relevant comments?

Copy and paste.

At this point, I'm not clear on what this but is about; it seems to be based on a false premise....

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  Comment 2.  Resummarizing accordingly.

Comment 10

[Sebastian Zartner [:sebo]]

This is not limited to the "border" shorthand property, but also to "background". E.g. if I define a CSS rule like the following it doesn't get serialized:

body {
  background: blue;
  background-size: 100px 50px;
}

According to the CSS3 definition at http://www.w3.org/TR/css3-background/#background it should be serialized to something like "background: none 0 0 / 100px 50px repeat scroll border-box padding-box blue".

And if you then add "background-position" and delete "background-size" like this:

body {
  background: blue;
  background-position: 20px 30px;
}

Then the styles also won't be serialized.

Sebastian

Comment 11

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

(In reply to Sebastian Zartner from comment #10)
> This is not limited to the "border" shorthand property, but also to
> "background". E.g. if I define a CSS rule like the following it doesn't get
> serialized:
> 
> body {
>   background: blue;
>   background-size: 100px 50px;
> }
> 
> According to the CSS3 definition at
> http://www.w3.org/TR/css3-background/#background it should be serialized to
> something like "background: none 0 0 / 100px 50px repeat scroll border-box
> padding-box blue".
> 
> And if you then add "background-position" and delete "background-size" like
> this:
> 
> body {
>   background: blue;
>   background-position: 20px 30px;
> }
> 
> Then the styles also won't be serialized.

I'm not sure what you mean by 'delete "background-size"', but if you use removeProperty to do it, you will *not* end up with a rule equivalent to what you write above.  You'll instead end up with a rule that does not override a background-size declaration on an earlier rule, unlike the rule you write above which does override an earlier background-size declaration.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Also, the background serialization code is a separate beast from the border serialization code; any issues with background should go in a separate bug.

Comment 13

[Sebastian Zartner [:sebo]]

> I'm not sure what you mean by 'delete "background-size"', but if you use
> removeProperty to do it, you will *not* end up with a rule equivalent to
> what you write above.  You'll instead end up with a rule that does not
> override a background-size declaration on an earlier rule, unlike the rule
> you write above which does override an earlier background-size declaration.

Sorry for the misunderstanding. I just used Firebug to reproduce the issue, so I used its terminology. Yes, internally Firebug calls removeProperty.
The second rule (including "background" and "background-position") is a transformation of the first rule (including "background" and "background-size") by calling removeProperty("background-size") followed by a setProperty("background-position", "20px 30px", "").

> any issues with background should go in a separate bug.
I will create one with a proper test case.

But first let's celebrate the new Year! :-)

Sebastian

Comment 14

[Kang-Hao (Kenny) Lu [:kennyluck]]

I had trouble reproducing this bug because the the steps to reproduce as referenced in comment 0 are in another link and it requires Firebug installed. Unfortunately this particular Firefox profile I am using doesn't have it. (It also seems like a Firebug bug if described that way.) 

Here is the easier steps to reproduce:

1. Click attachment 584395 [details].
2. Tools -> Web Development -> Web Console
3. Enter "alert(document.styleSheets[0].cssRules[0].cssText)"

Actual results (formatted):

div { 
  border-right: 1px solid silver; 
  border-width: medium 1px 1px; 
  border-style: none solid solid; 
  border-color: -moz-use-text-color silver silver; 
  -moz-border-top-colors: none; 
  -moz-border-right-colors: none; 
  -moz-border-bottom-colors: none; 
  -moz-border-left-colors: none;
  -moz-border-image: none;
}

Expected results (formatted):

div { 
  border-right: 1px solid silver; 
  border-top: none;
}

(In reply to Boris Zbarsky (:bz) from comment #8)
> > i'd think the introspection benefits outweigh the overhead by a good margin,
> 
> For the small minority of users using a debugger.  For the rest, the
> overhead is pure loss.

What about reparsing the stylesheet everytime cssText is called on a CSSRule? Does WYGIWYS editors use cssText on CSSRule often (or equivalently does making it slow affect normal users)?

Code complexity remains, though. Asking Firebug to do the parsing seems like an approach too.

Comment 15

[Sebastian Zartner [:sebo]]

Thanks Kenny for creating an easier test case.

> Code complexity remains, though. Asking Firebug to do the parsing seems like an 
> approach too.
It's not just Firebug that displays the CSS rules. Also the web dev tools do that and probably some other tools, too.
So it wouldn't make sense to move the responsibility for serializing the CSS to extensions.

Sebastian

Comment 16

[Sebastian Zartner [:sebo]]

I assume the implementation probably isn't too complicated, so is it ok to add [good first bug] to the whiteboard?

(In reply to Kang-Hao (Kenny) Lu [:kennyluck] from comment #14)
> Expected results (formatted):
> 
> div { 
>   border-right: 1px solid silver; 
>   border-top: none;
> }

This is actually wrong. The expected results are:

div { 
  border: 1px solid silver; 
  border-top: none;
}

The 'border-right' is incorrectly added to the serialization.

> (In reply to Boris Zbarsky (:bz) from comment #8)
> > > i'd think the introspection benefits outweigh the overhead by a good margin,
> > 
> > For the small minority of users using a debugger.  For the rest, the
> > overhead is pure loss.
> 
> What about reparsing the stylesheet everytime cssText is called on a
> CSSRule? Does WYGIWYS editors use cssText on CSSRule often (or equivalently
> does making it slow affect normal users)?

Looks like you asked Boris, so I'm requesting info from him.
Maybe some usage statistics could be acquired to be able to answer this question?

Sebastian

Comment 17

[Boris Zbarsky [:bzbarsky]]

> What about reparsing the stylesheet everytime cssText is called on a CSSRule?

We currently don't store the stylesheet text.  We could keep it around, but in the common case it's just a waste of memory, and we're already pretty memory-constrained.  Just storing a bitmask would be a better solution memory-wise.

In any case, that's not what the spec says to do for .cssText.

Comment 18

[Sebastian Zartner [:sebo]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> > i dunno if you guys expose this in the api
> 
> We don't.  "background: none" is expanded to "background: none repeat scroll
> 0 0 transparent" at parse time, and only the latter form is stored.

(In reply to Boris Zbarsky [:bz] from comment #17)
> > What about reparsing the stylesheet everytime cssText is called on a CSSRule?
> In any case, that's not what the spec says to do for .cssText.

Though when I'm interpreting the spec correctly, it says[1] that longhands with initial value that are not required should not be added to the serialized value.
That is also indicated by the examples[1], which need to be updated, though.

Test case:
data:text/html,<!DOCTYPE html><style>p{border:none;}</style><p>Test</p><script>document.write(document.styleSheets[0].cssRules[0].cssText);</script>

The expected output is:
p { border: none; }

Sebastian

[1] http://dev.w3.org/csswg/cssom/#serializing-css-values
[2] http://dev.w3.org/csswg/cssom/#serializing-css-values-examples

Comment 19

[Sebastian Zartner [:sebo]]

(In reply to Sebastian Zartner from comment #13)
> > any issues with background should go in a separate bug.
> I will create one with a proper test case.

Looks like I missed to do that. Did that now in bug 1134171.

Sebastian