413958 - More detailed error information needed for CSS errors from dynamically generated content

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Hubert Kauker]

User-Agent:       Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0; TBA-USER; .NET CLR 1.1.4322; .NET CLR 2.0.50727)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11

The following error message gets reported in the error console:
Error in parsing value for property '...'.  Declaration dropped.

I get this regularly in my pages for properties 'top' and 'left'.
As further information only the page url is given.
There is NO indication that this is a CSS problem, but I assume it is.





Reproducible: Always

Steps to Reproduce:
1.
2.
3.


Expected Results:  
[CSS] Error in parsing value '...' for property '...'.  Declaration dropped.
Selector: '...'. Stylesheet: id='...'.

or:

Object: ... id='...'



Examples:

[CSS] Error in parsing value '100' for property 'top'.  Declaration dropped.
Selector: '.TestClass'. Stylesheet: id='MyCss'.

[CSS] Error in parsing value '400' for property 'left'.  Declaration dropped.
Object: [object DOMElement] id='MyDiv'.

In other words: we should see the offending string which aborts the parse.
And some more context information as to where the parsing took place.

Thank you.

Comment 1

[Serge Gautherie (:sgautherie)]

Could you do the same "test" with a current Trunk nightly ?
Could you provide steps / code example ?

Comment 2

[Jo Hermans]

There a CSS syntax error on that page. A unit should be specified with top or
left, like 'em' or 'px'. See
<http://www.w3.org/TR/CSS21/syndata.html#value-def-length>

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The CSS parser is passing through the information that this is a CSS error; it's the error console that doesn't display it.  (Making it localizable might be a bit interesting, though.)

Comment 4

[Philip Chee]

Jo:
The original reporter knows that. His complaint is that the error message isn't detailed enough.

David:
All that the Error Console does is to print out what ever the nsIConsoleService throws at it. If the detail isn't forthcoming from nsIConsoleService then there is nothing the Error Console can do. I think that this bug should be moved back to Core::Style System (CSS)

On to the actual problem.

On trunk this is what I see:

Warning: Error in parsing value for property 'width'.  Declaration dropped.
Source File: http://i.l.cnn.net/cnn/.element/css/2.0/main.css
Line: 267

So you have the file (main.css) which is obviously a css file and the line number in the file with the error (267).

Clicking on the URL opens a view-source window focused on line 267 which in this case is:

{width: expression(document.documentElement.clientWidth < 1002? "1002px": "100%" );}

> In other words: we should see the offending string which aborts the parse.

{width: expression(document.documentElement.clientWidth < 1002? "1002px": "100%" );}

> And some more context information as to where the parsing took place.

Source File: http://i.l.cnn.net/cnn/.element/css/2.0/main.css
Line: 267

Hubert Kauker: All the information you have requested is available, except for:

> There is NO indication that this is a CSS problem, but I assume it is.

The category is reported [CSS] by the console service but I agree that there is no visual indication in the Error Console that the error is coming from CSS. The Console² extension <http://console2.mozdev.org/> which is a replacement for the standard Error Console does give a visual indication via a "CSS" icon on the the row.

Comment 5

[Hubert Kauker]

Philip:

Yes, thank you.
True, the error console reports quite well, when the parsing error happens in a css file which is included into the html document via a LINK tag.
By consulting the line number, it is usually quite obvious what is wrong.

But when I am using *script* to dynamically generate styles, 
what I very often do,
then only the html document url is given as a reference, which is not exactly useful.

In that case it would be a great help so get the offending syntax which
is rejected by the parser and the goal for which it is parsing.

I have created a demo at http://www.travelbasys.de/bug-413958.html.
A script contains the line:
    document.getElementById( "myDiv2" ).innerHTML = 
        "<p style='border: solid 1px red; width: 200'>This was generated</p>";

An error is duely reported in the console by there is no useful help about its cause. 
I would be glad to see the cause 'width: 200' in the error message
maybe extended by the context 'border: solid 1px red; width: 200'.

[ I shall have to try out the console extension you mentioned. ]

Comment 6

[Philip Chee]

What happens if, instead of .innerHTML you use something like:

var p = document.createElement('p');
p.setAttribute("style", "border: solid 1px red; width: 200");
p.appendChild(document.createTextNode("This was generated");
document.getElementById( "myDiv2" ).appendChild(p);

Comment 7

[Hubert Kauker]

I added the DOM test case to the test page.
Error console reporting is identical.

Installed console2 in the meantime.
Except for the css icon it reports the same as the classical console.

Comment 8

[Philip Chee]

Attachment: Test Case as attachment

Saving the remote test case as a bugzilla attachment.

Comment 9

[Philip Chee]

> Except for the css icon it reports the same as the classical console.

That makes sense. Both just report whatever bubbles up from the nsIConsoleService. If the underlying service doesn't provide the necessary information there is nothing any Error Console UI can do.

Comment 10

[Philip Chee]

Changing summary text and making this depend on Bug 228205.

Comment 11

[Philip Chee]

SeaMonkey::Error Console -> Toolkit::Error Console
Branch -> Trunk

1.8 branch is closed for enhancements.

Comment 12

[Zack Weinberg (:zwol)]

At its root this is a very simple style system problem: The CSS parser does not provide the text of the construct that provoked the error to the error console.  I have changes in progress that will fix this.

Comment 13

[Zack Weinberg (:zwol)]

Assigning to self.

Comment 14

[Zack Weinberg (:zwol)]

Attachment: patch v1

Dusting this off now we're finally in a position to land bug 663291 and therefore to test CSS error reporting in detail.  I wrote this patch a long time ago, but I never actually posted it.

The error console's pointer to the offending token can be off by a character or two in either direction; this will be fixed by the scanner rewrite in bug 543151.  I think this is a worthwhile change even if that never actually lands.

Functional dependency on bug 663291 for tests. Textual dependency on bug 516091, bug 455839, bug 229827 (in that order).

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 661644 [details] [diff] [review]
patch v1

I'm a little worried about memory usage here, given that minified CSS, often entirely on one line, is pretty common.

I think the least we could do is ensure that when there are multiple errors in succession on the same line, we end up using the same (shared) string buffer for all of them.  I don't think this patch does that, since it reassigns mErrorLine from an nsDependentString for each new error.  I think you need to ensure that we reuse the same buffer for multiple errors on the same line.

Otherwise this looks good, though.

Comment 16

[Zack Weinberg (:zwol)]

(In reply to David Baron [:dbaron] from comment #15)
> I think the least we could do is ensure that when there are multiple errors
> in succession on the same line, we end up using the same (shared) string
> buffer for all of them.  I don't think this patch does that, since it
> reassigns mErrorLine from an nsDependentString for each new error.  I think
> you need to ensure that we reuse the same buffer for multiple errors on the
> same line.

I'll have to think a bit about how to achieve that, but I'll make it happen.

Comment 17

[Zack Weinberg (:zwol)]

Attachment: patch v2

Here's a revised patch which caches the current line in the ErrorReporter until the scanner reports that the line number has changed.  If I correctly understand how nsString works, this should have the effect of sharing the text among all errors reported on that line.

I thought about trimming the line maybe 80 characters on either side of the syntax error, but then I'd have to lie about the column number in the actual source, which would cause problems downstream, e.g. for clicking on something in the error console and having it jump to the text in view-source, which is a thing that *almost* works now (we highlight the whole line and don't scroll horizontally to the position of the error).

Comment 18

[Philip Chee]

> I thought about trimming the line maybe 80 characters on either side of the syntax
> error,
The front end Error Console code already trims long lines. See:
Bug 796179 - Don't store full source URI in an attribute in the error console, and display it in abbreviated form to the user.

Comment 19

[Zack Weinberg (:zwol)]

(In reply to Philip Chee from comment #18)
> The front end Error Console code already trims long lines. See:
> Bug 796179 - Don't store full source URI in an attribute in the error
> console, and display it in abbreviated form to the user.

Does that work at both ends?  Imagine if I have a minified CSS file, maybe ten thousand characters all on one line, and there's a syntax error a couple thousand characters along.  With the code in this latest patch, we'll make one copy of the entire line into an nsIScriptError object and then that will get sent to the console.  I have observed the console put an ellipsis *after* the point where the error is marked, but is it capable of collapsing a great deal of text *before* the error mark into an ellipsis as well?

Comment 20

[Philip Chee]

Looking at the patches in Bug 796179, the code just truncates the line to the first 200 characters.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 687264 [details] [diff] [review]
patch v2

I don't see why ou need both mErrorLine and mErrorLineCache -- though you do need one that you don't clear in nsScanner::ClearError.  Can you make it just have mErrorLine, or am I missing something?

I'm assuming the change of the column by 1 was intentional (in nsCSSScanner::GetColumnNumber), and it improves things.  If not, you shouldn't change it, though.

And could you add a comment explaining why we want to ensure that we use the same string when we report multiple errors on the same line (i.e., to benefit from sharing)?

r=dbaron with that

sorry for the delay getting to this

Comment 22

[Zack Weinberg (:zwol)]

(In reply to David Baron [:dbaron] from comment #21)
> Comment on attachment 687264 [details] [diff] [review]
> patch v2
> 
> I don't see why ou need both mErrorLine and mErrorLineCache -- though you do
> need one that you don't clear in nsScanner::ClearError.  Can you make it
> just have mErrorLine, or am I missing something?

I just didn't think of that.  I looked at it carefully and I agree, it should be enough to preserve and reuse mErrorLine until the line number changes.

> I'm assuming the change of the column by 1 was intentional (in
> nsCSSScanner::GetColumnNumber), and it improves things.  If not, you
> shouldn't change it, though.

Yes, that was intentional: most of the time, when GetColumnNumber is called, mOffset is one past the end of the token that provoked the error message, so adding 1 makes the error console point *two* past the end of the token.  It's still not what you would call *precise*, but as I said, that should be fixed by bug 543151 (and any remaining problems will be easier to address with that in place).

> And could you add a comment explaining why we want to ensure that we use the
> same string when we report multiple errors on the same line (i.e., to
> benefit from sharing)?

Will do.

Comment 23

[Zack Weinberg (:zwol)]

Attachment: patch v3 (landed)

https://hg.mozilla.org/integration/mozilla-inbound/rev/028ce4b36b4e

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/028ce4b36b4e