Closed Bug 95062 Opened 23 years ago Closed 22 years ago

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

Categories

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

Product:
Core
Component:
DOM: CSS Object Model
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: glazou, Assigned: glazou)

References

Details

(Keywords: css1, dom2, Whiteboard: correctness, dom2, css1)

Attachments

(2 files, 4 obsolete files)

test case showing bug
1.07 KB, text/html
Details
patch 2.0
3.14 KB, patch
peterv
: review+
jst
: superreview+
Details | Diff | Splinter Review 
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
Assignee: jst → harishd
setting mozilla0.9.4 + keywords
Whiteboard: correctness,dom2,css1
Target Milestone: --- → mozilla0.9.4
--> 0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.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 ?-)
Assignee: harishd → glazman
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.
Keywords: css1, dom2, patch
Whiteboard: correctness,dom2,css1 → correctness, dom2, css1
Blocks: 42417
Target Milestone: mozilla0.9.5 → mozilla0.9.6
bulk milestone change
Target Milestone: mozilla0.9.6 → mozilla0.9.7
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
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Attached patch patch 2.0 — — Splinter Review 
new patch for this bug ; must be in before CSS in Composer.
Pierre,Boris: please review
Attachment #47187 - Attachment is obsolete: true
Attachment #47188 - Attachment is obsolete: true
Attachment #47197 - Attachment is obsolete: true
Attachment #47206 - Attachment is obsolete: true
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.
Attachment #62337 - Flags: review+
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
Attachment #62337 - Flags: superreview+
checked in trunk
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
<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
You need to log in before you can comment on or make changes to this bug.