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

NEW
Unassigned

Status

()

P5
normal
7 years ago
4 years ago

People

(Reporter: sebastianzartner, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++], URL)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
Created attachment 584395 [details]
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
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.
Component: General → Style System (CSS)
QA Contact: general → style-system
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.
OS: Windows 7 → All
Priority: -- → P5
Hardware: x86 → All
Summary: CSS shorthand properties get expanded, if there's another shorthand property overwriting a part of it → CSS shorthand properties get expanded if there's another shorthand property overwriting a part of it

Comment 3

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

7 years ago
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?
Possible, yes, at the cost of memory bloat and more code complexity and so forth....

Comment 7

7 years ago
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
> 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....
Oh, I see.  Comment 2.  Resummarizing accordingly.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: CSS shorthand properties get expanded if there's another shorthand property overwriting a part of it → Do a better job of serializing 'border: 1px solid silver; border-top: none;" using border-* shorthands
(Reporter)

Comment 10

7 years ago
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
(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.
Also, the background serialization code is a separate beast from the border serialization code; any issues with background should go in a separate bug.
(Reporter)

Comment 13

7 years ago
> 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
Attachment #584395 - Attachment mime type: text/plain → text/html
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.
(Reporter)

Comment 15

7 years ago
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
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
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(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
(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
Whiteboard: [lang=c++]
You need to log in before you can comment on or make changes to this bug.