Closed Bug 99554 Opened 23 years ago Closed 23 years ago

WMRB: block delimiters in style attributes causes failure

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86
Windows 2000
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.5

People

(Reporter: dunn5557, Assigned: dbaron)

References

(
URL
)

Details

(Keywords: topembed, Whiteboard: PDT+)

Attachments

(6 files)

Testcase with various levels of braces in the inline style attribute value
308 bytes, text/html
Details
patch to implement allowing no braces or single pair of braces, all modes
6.66 KB, patch
Details | Diff | Splinter Review
also change out param from |nsIStyleRule*&| to |nsIStyleRule**|
6.91 KB, patch
Details | Diff | Splinter Review
quirks mode testcase
330 bytes, text/html
Details
strict mode testcase
421 bytes, text/html
Details
above patch, changed to affect quirks mode only
7.01 KB, patch
attinasi
: superreview+
Details | Diff | Splinter Review 
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.
Keywords: topembed
Summary: block delimiters in style attributes causes failure → WMRB: block delimiters in style attributes causes failure
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.
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...
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>

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
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.
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...
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.
> 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).
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.
Reviews?
Severity: normal → major
Status: NEW → ASSIGNED
Keywords: nsbranch
Priority: -- → P1
Target Milestone: --- → mozilla0.9.5
r=hixie for both the all modes patch and the quirks-only patch.
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.

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?
nsbranch+ ing so this goes into branch.
Keywords: nsbranch → nsbranch+
Comment on attachment 49421 [details] [diff] [review]
above patch, changed to affect quirks mode only

sr=attinasi
Attachment #49421 - Flags: superreview+
Fix checked in to trunk 2001-09-17 16:59 PDT.
Assuming we're taking the quirks mode only patch, PDT+.  Please check in when
tested and ready.
Whiteboard: PDT+
Checked into branch 2001-09-25 21:47 PDT.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Verified fixed. tested with 9-26-2001 MFCEmbed
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: