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: