107923 - nsCSSDeclaration::GetCssText flubs !important rules

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Boris Zbarsky [:bzbarsky]]

All the shorthand property handling in nsCSSDeclaration::GetCssText has major
issues with handling of !important.  Specifically, it does not distinguish
between the following sets of property/value pairs:

1) border: 1px solid red;
2) border: 1px solid red !important;
3) border: 1px red; border-style: solid !important;

The logic for deciding when to combine things and when not to really should take
this into account.

I have a simple change to AppendValueToString which could fix the fact that
!important properties are not displayed, but it just adds "!important" after the
property value, which is no good if multiple properties values get appended in a
row as the shorthand code does.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing some of the problems

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Very good catch, Boris. Accepting bug.

Comment 3

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

098

Comment 4

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

=> 099

Comment 5

[Kevin McCluskey (gone)]

Moving to Moz1.1. Engineers are overloaded with higher priority bugs.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I should note that this has an easy fix if it were not for the design of our
shorthand system.   Could we please back that out, fix this bug, and then do the
shorthands when we can do them something resembling right?  As it is, we're
violating the spec here for questionable benefit...

Comment 7

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

NO !!! This change is not a questionable benefit for CSS in Composer but
an absolutely necessary change. DON'T DO THAT !

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  So could we please not break CSS use by web authors for the benefit of CSS
in composer in the future?  Unlike web authors composer _could_ be using
internal APIs for this if it needed to...

Comment 9

[Johnny Stenback (:jst)]

If this broke real webpages only in favor of making CSS in composer work
correctly, then I'm afraid I'll haveto go with bz here. Regressing stuff that
shows up on real web pages is not acceptable unless the gain in some other are
is *really* *really* high. Can we do something about this?

Comment 10

[Boris Zbarsky [:bzbarsky]]

I should qualify my comments in glazou's favor.  The shorthands did not break
anything that was not broken -- we have always flubbed the !important rules. 
The shorthand stuff just makes fixing the !important issue much harder...

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Fix almost in hand. Should be ok for 099.

Comment 12

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.0

with this patch, cssText now correctly output "! important" declarations and
I fixed some holes in my previous code (for instance border-style shorthand
not handled). Test case attached OK, and even with a better factorization
that the original css rules.

Reviews please ?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> (!isBgPositionXImportant  == !isBgPositionYImportant))

You do this all over...  Just use 
(isBgPositionXImportant == isBgPositionYImportant) -- these are PRBools...

I think it would make sense to add nsCSSDeclaration::AllPropertiesSameImportance
and call that first thing from nsCSSDeclaration::AllPropertiesSameValue.  That
would be much more reusable, in my opinion, for other shorthands too...

Change AppendImportanceToStringIfNeeded and AppendDeclarationSeparator to return
nsAString& instead of void.  Then you can do things like:

aString.Append(ImportanceIfNeeded(GetValueIsImportant(aProperty)) +
DeclarationSeparator());

reducing string copies.  You could even inline those functions.  As far as that
goes, I'd make the declaration separator a define....  [#define
kDeclarationSeparator NS_LITERAL_STRING("; ") ]

Your new code could fail to create a background shorthand in the following
scenario:

background: url() center center no-repeat green;
background-attachment: center right !important;

Since in this case you will reset numberOfSameImportance to 1.  You probably
only want to do that for cases when numberOfSameImportance has not gotten bigger
than 1 yet...  (this last issue is pretty minor and I'd be happy to see that
spun off into a separate bug).

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

boris : no, you cannot use (a == b) if a and b are PRBools. We had a discussion
in #mozilla with brendan and shaver ; PRBool is an int ; it *should* be 0 or 1
but that's not for sure. Using (!a == !b) guarantees that you are comparing values
both equal to 0 or 1.

About shorthand factorization : I studied it a while and I think that factorization
in all cases is extremely difficult, or even impossible. Because border-*
shorthands can be grouped by size or by type, there are cases where you just cannot
decide the factorization user wants. Implementation has to make a choice.

Comment 15

[Johnny Stenback (:jst)]

!a == !b seems just wrong to me. Why not do the right thing and ensure that you
never assign a "true" value other than 1 into PRBool's every time you assign
into them? Then you could do the obvious and natural a == b check w/o confusing
the code with the negation operators all over.

Comment 16

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v2.0

Comment 17

[Boris Zbarsky [:bzbarsky]]

1) I still think we could use nsCSSDeclaration::AllPropertiesSameImportance....
   Could be used in at least two places in your code, and likely in future
   shorthands.  If you decide not to do that, I think it would be nice to change
   the code in nsCSSDeclaration::AllPropertiesSameValue to be as lazy about
   getting values as possible (getting values can be expensive).

   So something like:

PRBool isSecondImportant =
    GetValueIsImportant((nsCSSProperty)mOrder->ValueAt(aSecond));
if (isFirstImportant != isSecondImportant) {
  return PR_FALSE;
}
GetValue((nsCSSProperty)mOrder->ValueAt(aFirst), firstValue);
GetValue((nsCSSProperty)mOrder->ValueAt(aSecond), otherValue);
if (firstValue != otherValue) {
  return PR_FALSE;
}

2)  

> +#define CSS_IMPORTANCE_STRING(importance)   (importance ? ...

Please parenthesize "(importance)" in the macro body.

> +#define kDeclarationSeparation              NS_LITERAL_STRING(" ; ")

Why the space before the ';'?  Also, I'd call it "separator" not "separation".

Other than that, looks good...  Thanks for doing this, glazou.

Comment 18

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 70514 [details] [diff] [review]
patch v2.0

I am merging the work done on this bug and the one done on bug 142019. Please
see bug 142019 for new patches.

Comment 19

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Forgot to say a few things :

  1. new patch follows Hixie's suggestions
  2. I also fixed a bug in background shorthand output

Comment 20

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

This bug is fixed by fix for 142019, checked in just a minute ago => closing