95062 - getComputedStyle answers "" for text-decoration if value is 'none' or if more than one value

Status: RESOLVED FIXED

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

Description

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

Call to getComputedStyle answers "" for 'text-decoration' property if its value is 
'none' or if its has more than one value. For example if

    text-decoration : underline overline

See test case attached for more information.

This bug blocks the CSSization of Composer (bug 77705).

Comment 1

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

Attachment: test case showing bug

Comment 2

[Boris Zbarsky [:bzbarsky]]

reassign to Harish

Comment 3

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

setting mozilla0.9.4 + keywords

Comment 4

[harishd]

--> 0.9.5

Comment 5

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

Here is what happens here : the implementation of GetTextDecoration cannot
work properly because it forgets that the property can accept a list of values.
The actual implementation only looks for one value.

Reassigning to myself, since I have a patch in hand for this bug and it works
absolutely fine. Keeping 0.9.5 target. Harish, is that ok for you ?-)

Comment 6

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

Attachment: patch v1.0

Comment 7

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

Patch added, waiting for reviews. Extensively tested and working fine.

Comment 8

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

Attachment: patch v1.1

Comment 9

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

Oopps, added a wrong version of the patch ; I have attached now the correct
version, sorry for the spam.

Comment 10

[Pierre Saslawsky]

I did not run the patch but... does 'none' work?  The string is not in 
nsCSSProps::kTextDecorationKTable.

Comment 11

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

Attachment: patch v1.2

Comment 12

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

Good catch Pierre, thanks! new patch attached

Comment 13

[Hixie (not reading bugmail)]

If the stylesheet says "text-decoration: underline;" then getComputedStyle will
return "underline " with that implementation. Is that intentional?

Also, what's the impact of adding the |eCSSKeyword_none| value to the 
|nsCSSProps::kTextDecorationKTable[]| enum? Why did everything work fine before
without it? I'm concerned that this will break something.

Comment 14

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

Hixie: (a) the 'none' keyword being common to a lot of properties, its parsing
is shared in nsCSSParser.cpp ; no other code uses the table; adding none to the
table should have no impact.
(b) ok for the ' ', working on it

Comment 15

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

Attachment: patch v1.onemore

Comment 16

[Pierre Saslawsky]

As Ian pointed out, I don't like the 'eCSSKeyword_none' in 
'kTextDecorationKTable' either.  In the Parser, it may turn a VARIANT_NONE to 
VARIANT_KEYWORD and change the resulting value to eCSSUnit_Enumerated instead of 
eCSSUnit_None.  Appparently it doesn't because ParseVariant() checks for 
VARIANT_NONE before checking for VARIANT_KEYWORD, but I think it should be 
avoided.

You don't need to put the keyword into the table anyhow.  When 
(NS_STYLE_TEXT_DECORATION_NONE == text->mTextDecoration), you can call 
nsCSSKeywords::GetStringValue(eCSSKeyword_none) or something like that instead of 
SearchKeywordTable(NS_STYLE_TEXT_DECORATION_NONE).

Comment 17

[Hixie (not reading bugmail)]

For that matter, why search the table each time anyway? Isn't that a bit of a
waste of time?

(BTW, I would be interested to hear from a C++ compiler expert about whether

   if (multipleValues) decorationString.Append(PRUnichar(' '));
   multipleValues = PR_TRUE;

...generates more or less code than

   if (multipleValues) {
      decorationString.Append(PRUnichar(' '));
   } else {
      multipleValues = PR_TRUE;
   }

In this case it's not very important, but it's a common pattern and I've often 
wondered which would be best.)

Comment 18

[Pierre Saslawsky]

In terms of code size, the first solution is more efficient. In term of 
performance, the question is: "Is the jump generated by the 'else' more efficient 
than the assignment of the boolean?"  In the good old times, the assignment would 
usually prevail because the variable would have been assigned to a fast-access 
internal register. With modern processors, I guess the two solutions are 
equivalent and would be executed in less than a single tick.

Brendan has been known to give very thorough explanations about similar code 
optimizations, which makes me smile given the architecture of the product.

Comment 19

[Dylan Schiemann]

I have found that at a minimum, the following css properties return "" using
getComputedStyle when no non-default value is explicilty set for them:

background-image
border-*-style
font-style
font-variant
text-decoration
visibility
text-align

The correct value is returned if there is an explicit style sheet declaration. 
This bug seems to be addressing this problem for text-decoration, so should I
file another bug on this, or is this really the same as this bug?

Comment 20

[Christopher Hoess (gone)]

Fixing keywords.

Comment 21

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

bulk milestone change

Comment 22

[Boris Zbarsky [:bzbarsky]]

glazou, are you working on this?  If  not, would you like me to take it?  (If I
do, it's not happening till January, as a warning).

Comment 23

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

098

Comment 24

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

Attachment: patch 2.0

new patch for this bug ; must be in before CSS in Composer.
Pierre,Boris: please review

Comment 25

[Peter Van der Beken [:peterv]]

Comment on attachment 62337 [details] [diff] [review]
patch 2.0

>+        if (multipleValues) decorationString.Append(PRUnichar(' '));

I prefer

+	 if (multipleValues)
+	   decorationString.Append(PRUnichar(' '));

but r=peterv any way.

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 62337 [details] [diff] [review]
patch 2.0

What peterv said, and you could loose the multipleValues local, use
!decorationString.IsEmpty() as the condition in the if checks in stead.

sr=jst

Comment 27

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

checked in trunk

Comment 28

[Jerome Lacoste]

<spam>

I was just reading the patch that was just checked. I was wondering how you guys
feel about using macros or inline functions to avoid repeating code like this one:

+      if (text->mTextDecoration & NS_STYLE_TEXT_DECORATION_BLINK) {
+        if (multipleValues) decorationString.Append(PRUnichar(' '));
+        const nsAFlatCString& decoration=
+          nsCSSProps::SearchKeywordTable(NS_STYLE_TEXT_DECORATION_BLINK,
+                                         nsCSSProps::kTextDecorationKTable);
+        decorationString.AppendWithConversion(decoration.get());
+      }

This block is used 4 times, the only difference beeing the flag (in this case
NS_STYLE_TEXT_DECORATION_BLINK).
In this case, templatising this code is not perhaps very important, but I have
seen cases where the same code is repeated 20 times. Then one has to modify it.

A macros or an inline fonction would have helped to avoid some errors, but as I
don't see this used often, I was wondering if it was considered as a bad
practise. Perhaps does it have negative effects on performance, or it cannot be
compiled on some architectures?

I 've already asked that directly to some developers but without answer. As I
often spend time per day reviewing patches and code, I would be happy to get
some feedback. Answer me by email if you want. Any links appreciated. Thanks.

</spam>

Comment 29

[Brendan Eich [:brendan]]

peterv is right, put then statements on separate lines, if they do anything more
than return rv (and maybe even then) -- otherwise you can't breakpoint the then
part -- all debuggers I know of will breakpoint the if condition before it is
evaluated.

/be