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).

reassign to Harish

setting mozilla0.9.4 + keywords

--> 0.9.5

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 ?-)

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

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

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

Good catch Pierre, thanks! new patch attached

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.

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

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).

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.)

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.

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?

Fixing keywords.

bulk milestone change

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).

098

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

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 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

checked in trunk

<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>

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