Closed Bug 263083 Opened 18 years ago Closed 18 years ago

View source tokenizer error reporting

Categories

(Core :: DOM: HTML Parser, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(2 files)

I have been working towards this for quite some time now (slowly but surely). I
have the easy part of the fix for this in my tree, but need to finish writing
the code that outputs these erroneous tags.

Before I write this code, I need to know if we should have a pref for this
(i.e., only show errors if the pref is set) and if you have any preferences for
style issues. Please CC anybody who isn't/may be interested in this.

Note: the types of errors that will show up are things like: "<b <i>" which
according to SGML is not an error, but in actual practice is probably a mistake
in HTML.

Also note that some of the code for this will be checked in as a part of bug 70918.
(hm, I wonder whether bug 76412 relates in any way...)

duplicate of bug 6211, maybe?
I don't think we need a pref.

Note that this is for tokenizer errors, whereas bug 6211 is much more broad in
scope (eg containment errors, etc).
Bug 76412 is referring to code that is no longer functional (and should be taken
out, as is indicated by comments 9, 10, 11 in that bug) so it should remain a
separate bug.

This probably blocks bug 6211 (or has some sort of relationship).
Currently, I have a mostly working patch in my tree. There is one problem with
it: when I output an attribute that is in error, the view-source source (phew!)
looks like:
<span class="error"><span class="attribute-key">key</span></span><span
class="error"><span class="attribute-value">value</span></span>

This isn't ideal, as I output the "error" tag twice. Is this something I should
wait and try to fix, or is this behavior OK?
It's probably ok for a first cut (though it would be nice to fix it in a followup).
Attached patch patch v1Splinter Review
This patch has the bug that I was talking about before, but otherwise works
quite well. I don't really understand all of the CSharedVSContext foo, so I
just followed the example of what we currently do with syntax highlighting. I'm
also open to suggestions on how to color error tags. I'm against <blink> since
one quote can be in error, and if it's blinking, it may be hard to see.
Comment on attachment 162386 [details] [diff] [review]
patch v1

rbs, could you give this r=?
Attachment #162386 - Flags: review?(rbs)
Comment on attachment 162386 [details] [diff] [review]
patch v1

r=rbs

+	   // The attribute is only in error if its parent is NOT in error.
+	   PRBool attributeInError = 
+	     theAttrToken->IsInError() && !aParentInError;

What is the |parent| of an attribute? Use |tag| or |owner|.

+    mSink->CloseContainer(eHTMLTag_span);  //emit </starttag>...

You meant: emit </endtag>...

Do well to attach (or point to) a testcase. People who follow bugs of interest
to them often download the next build after the fix, and go back to bug to
experiment and enjoy the new features as demonstrated by the initial testcases.

---
Did you experiment a background-color rather than making the whole thing bold?
Just curious. I don't mind what you have.
Attachment #162386 - Flags: review?(rbs) → review+
(In reply to comment #8)
> (From update of attachment 162386 [details] [diff] [review])
> What is the |parent| of an attribute? Use |tag| or |owner|.

Fixed.

> You meant: emit </endtag>...

Fixed.

> 
> Do well to attach (or point to) a testcase. 

I will a bit later this week.

> Did you experiment a background-color rather than making the whole thing bold?
> Just curious. I don't mind what you have.

The only problem with background colors is (of course) attributes. Given:
<a     href="unterminated string literal>

The key of |href| holds all that whitespace, so we would show
<a<error>     href="..."</error>>, which is ugly (I tried this with underlining
too). Unfortunately, this is extremely non-trivial to solve (we'd have to
generate whitespace tokens inside start-tags or something). If it ever becomes
an issue, I'll work on this.
Comment on attachment 162386 [details] [diff] [review]
patch v1

dmose, could you give this sr=?
Attachment #162386 - Flags: superreview?(dmose)
Attached file testcases
This is a testcase that shows most of the features of this patch. Note that
some errors can only be displayed once per document (such as the CDATA not
being properly terminated.)
Comment on attachment 162386 [details] [diff] [review]
patch v1

>@@ -953,16 +953,20 @@ nsresult CViewSourceHTML::WriteAttribute
>           mTokenizer->PopToken(); //pop it for real...
>           theContext.mTokenNode.AddAttribute(theToken);  //and add it to the node.
> 
>           CAttributeToken* theAttrToken = (CAttributeToken*)theToken;
>           const nsAString& theKey = theAttrToken->GetKey();
>+          
>+          // The attribute is only in error if its parent is NOT in error.
>+          PRBool attributeInError = 
>+            theAttrToken->IsInError() && !aParentInError;

This could be |const|, right?

Also, if you swap the ordering of the conditions, you'll avoid a method call in
some cases.

With those tweaks, sr=dmose.
Attachment #162386 - Flags: superreview?(dmose) → superreview+
Updated patch checked in.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
I can see it as a Seamonkey-only feature. Cause it doesn't work on FF trunk
(20041025 Win32). Am I wrong?
It shouldn't work anywhere because I forgot to check in viewsource.css! It
should be fixed (for real) now. Sorry!
Verifying to work with latest tinderbox FF (beast) trunk build although I would
prefer a different (colored) text background for the tags in error.
Summary: View source error reporting → View source tokenizer error reporting
You need to log in before you can comment on or make changes to this bug.