Last Comment Bug 99554 - WMRB: block delimiters in style attributes causes failure
: WMRB: block delimiters in style attributes causes failure
Status: VERIFIED FIXED
PDT+
: topembed
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Windows 2000
: P1 major (vote)
: mozilla0.9.5
Assigned To: David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
: Hixie (not reading bugmail)
:
Mentors:
http://www.lanaconline.com
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-09-13 17:09 PDT by Michael Dunn
Modified: 2001-09-26 16:42 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase with various levels of braces in the inline style attribute value (308 bytes, text/html)
2001-09-13 17:29 PDT, Marc Attinasi
no flags Details
patch to implement allowing no braces or single pair of braces, all modes (6.66 KB, patch)
2001-09-13 18:20 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
also change out param from |nsIStyleRule*&| to |nsIStyleRule**| (6.91 KB, patch)
2001-09-13 18:40 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
quirks mode testcase (330 bytes, text/html)
2001-09-14 19:58 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details
strict mode testcase (421 bytes, text/html)
2001-09-14 19:59 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details
above patch, changed to affect quirks mode only (7.01 KB, patch)
2001-09-14 19:59 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
attinasi: superreview+
Details | Diff | Splinter Review

Description Michael Dunn 2001-09-13 17:09:30 PDT
The Sale text renders BLACK in NSCP (CS) but renders RED in IE.
Failure to render is due to invalid curly brackets in tag attribute:<span
style="{color: 990000}">. If the curly brackets are removed then the color
renders as expected
---> We have a choice here: we can create an bug to make the inclusion of curly
brackets benign in this type of instance or request the web site to conform to
standard. Its seems to me that it is easy to determine intent and a parser
change would be nice.

(per jesse@netscape.com)
We could fix the color problem by allowing "style" attributes to
contain declaration block delimiters ('{' and '}') in quirks mode.
Comment 1 Marc Attinasi 2001-09-13 17:25:47 PDT
CC'ing Daniel Glazman since he has some experience extending the CSS parser too,
and Pierre as well.

I think that this should be pretty easy to do, and in Quirks mode, I see no
reason not to support it, especially considering that IE allows it.
Comment 2 Marc Attinasi 2001-09-13 17:29:11 PDT
Created attachment 49280 [details]
Testcase with various levels of braces in the inline style attribute value
Comment 3 Marc Attinasi 2001-09-13 17:30:47 PDT
I attached a simple testcase showing that IE pretty much ignores the curly
braces, no matter how many, or if they are balanced. I'm not sure how much we
care to emulate their behavour in this respect...
Comment 4 Marc Attinasi 2001-09-13 17:34:02 PDT
Wow, IE even handles declarations outside of the braces, as in:

<html>
<body>
<p style="{{{{color: red;}font-size: larger;">Unbalanced set of braces</p>
</body>
</html>

They do not seem to allow the braces inside of a declaration, fortunately, as in:

<p style="border:{1px solid black;}">
Unbalanced set of braces</p>
Comment 5 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-09-13 17:51:37 PDT
I don't think we should ignore all braces, but I think it would be reasonable to
accept a single pair of braces -- especially considering the formal statements
in the CSS specs have actually been ambiguous (some said there should be no
braces, some said, I think, that braces are required -- the original intent was
that there be no braces, though).  Also, in CSS3, I suspect the braces are
likely to be allowed: http://www.w3.org/TR/css-style-attr
Comment 6 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-09-13 18:20:41 PDT
Created attachment 49292 [details] [diff] [review]
patch to implement allowing no braces or single pair of braces, all modes
Comment 7 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-09-13 18:40:20 PDT
Created attachment 49295 [details] [diff] [review]
also change out param from |nsIStyleRule*&| to |nsIStyleRule**|
Comment 8 Daniel Glazman (:glazou) 2001-09-14 01:57:03 PDT
The standard is now very clear even if it was not. At the time IE added that
to their parser, ie in the HTML4.0 timeframe, they were totally right.

Excerpt from the only valid spec now : HTML4.01, section 14.2.2

  The syntax of the value of the style attribute is determined by the default
  style sheet language . For example, for CSS2 inline style, use the
  declaration block syntax described in section 4.1.8 (without curly brace  
  delimiters).

Please also follow threads in www-style@w3.org (1st in chrono order) and
w3c-css-wg@w3.org (open to W3C members only) :

  http://lists.w3.org/Archives/Public/www-style/1999Sep/0056.html

  http://lists.w3.org/Archives/Member/w3c-css-wg/1999JulSep/0173.html
  http://lists.w3.org/Archives/Member/w3c-css-wg/1999OctDec/0115.html

The result of the discussions is in :

  http://www.w3.org/TR/css-style-attr

dbaron : if we had the patch to Quirks, what is the side effect on the
query of the value of a STYLE attribute containing curly braces ? Since the
value is parsed and somehow regenerated, we are going to loose the curly braces,
right ? Just wondered if it is a problem or not. Will review your patch today.
Comment 9 Marc Attinasi 2001-09-14 10:17:26 PDT
The testcases I attached showed that IE allows the curley braces in various
places, but it looks like your patch will only allow the curley braces if the
first token in the attribute's value is a '{'. I saw your comment about not
wanting to ignore curley braces everywhere, but if we restrict this to Quirks
mode, we probably want to try and match the prevailing quirks, right?

I think that this needs to be supported in Quirks mode only.

Would it maybe be simpler to just strip all curley braces from the string in
Quirks mode before we parse the declaration block? Just a thought...
Comment 10 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-09-14 11:03:43 PDT
Is there any sign that real web pages use anything other than one pair of curly
braces?  I'd still like our behavior in quirks mode to be as logical as possible
-- after all, most web pages will be using quirks mode, so our level of
standards compliance there does matter.
Comment 11 Marc Attinasi 2001-09-14 11:50:02 PDT
> Is there any sign that real web pages use anything other than one pair of 
> curly braces?  
Not that I know of.

> I'd still like our behavior in quirks mode to be as logical as possible
Sure, that is reasonable.

> -- after all, most web pages will be using quirks mode, so our level of
> standards compliance there does matter.
OK, but we are equally non-compliant whether we allow a simple set of
surrounding braces, two sets, arbitraty braces - what is the difference WRT the
standards?

Given your 
first and second arguments, I'm happy to go along with this patch provided it is
restricted to Quirks mode (or, you can try to convince me that it is OK to do
this in standard mode too).
Comment 12 Hixie (not reading bugmail) 2001-09-14 12:35:35 PDT
Depending on what happens with http://www.w3.org/TR/css-style-attr the braces
may become part of the syntax.

I agree with dbaron that we should keep our handling as logical as possible,
even in quirks mode.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-09-14 19:58:51 PDT
Created attachment 49419 [details]
quirks mode testcase
Comment 14 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-09-14 19:59:06 PDT
Created attachment 49420 [details]
strict mode testcase
Comment 15 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-09-14 19:59:42 PDT
Created attachment 49421 [details] [diff] [review]
above patch, changed to affect quirks mode only
Comment 16 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-09-14 20:00:50 PDT
Reviews?
Comment 17 Hixie (not reading bugmail) 2001-09-15 08:48:38 PDT
r=hixie for both the all modes patch and the quirks-only patch.
Comment 18 Daniel Glazman (:glazou) 2001-09-17 01:40:52 PDT
r=glazman but can you make your modifications coherent with file's coding
style. For instance

  }
  else {

instead of

  } else {

I also prefer :

  haveBraces = (eCSSToken_Symbol == mToken.mType) && ('{' == mToken.mSymbol);

instead of

  haveBraces = eCSSToken_Symbol == mToken.mType && '{' == mToken.mSymbol;

Thanks. It helps readability, so it helps us.
Comment 19 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-09-17 07:23:26 PDT
Ugh.  OK, I'll make the first of the two changes, but I'd rather leave the
second one as is -- over-bracing disables compiler warnings that tell you about
things like = instead of ==.  Is it ok if I break it into two lines at the &&
instead?
Comment 20 Suresh Duddi (gone) 2001-09-17 09:41:04 PDT
nsbranch+ ing so this goes into branch.
Comment 21 Marc Attinasi 2001-09-17 13:58:41 PDT
Comment on attachment 49421 [details] [diff] [review]
above patch, changed to affect quirks mode only

sr=attinasi
Comment 22 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-09-17 18:14:36 PDT
Fix checked in to trunk 2001-09-17 16:59 PDT.
Comment 23 selmer (gone) 2001-09-25 14:23:13 PDT
Assuming we're taking the quirks mode only patch, PDT+.  Please check in when
tested and ready.
Comment 24 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-09-25 22:01:17 PDT
Checked into branch 2001-09-25 21:47 PDT.
Comment 25 Michael Dunn 2001-09-26 16:42:44 PDT
Verified fixed. tested with 9-26-2001 MFCEmbed

Note You need to log in before you can comment on or make changes to this bug.