70918 - view source adds missing > (greater than sign/right angle bracket)

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD:  2001-03-05-05 on Linux

Load the following HTML:

<table style="width: 100; height: 100"
  <tr>
    <td>
      Some text
    </td>
  </tr>
</table>

Now view the source.  Observe that a ">" is inserted after the <table tag (on
the following line).  I would expect view source to show the actual source.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Same testcase as text/plain

Comment 3

[clayton]

Futuring.  If a test case is presented showing this can lead to serious 
misinterpretaton of content, we will happily reconsider.

Comment 4

[Jesse Ruderman]

See also bug 49030, which used to cover this bug and a discrepency in how this 
type of malformed page is displayed.  That bug now covers just the page display 
problem (not the view-source problem).

Comment 5

[Andreas M. "Clarence" Schneider]

*** This bug has been marked as a duplicate of 57724 ***

Comment 6

[Christopher Hoess (gone)]

Reopening 57724 dependencies for independent resolution.

Comment 7

[Christopher Hoess (gone)]

*** Bug 49030 has been marked as a duplicate of this bug. ***

Comment 8

[Andreas Lange]

Still present in 1.2b.

It might not be that important, but this was really annoying when I was using
Mozilla to debug html which displayed fine in moz, but not in NS4. For other
reasons I didn't use the view-source in NS4. The problem was a missing right
angle bracket. Mozilla nicely hid the problem for me until I saved the page
locally and brought the source up in emacs...

Comment 9

[timeless]

*** Bug 182420 has been marked as a duplicate of this bug. ***

Comment 10

[Jesse Ruderman]

See also bug 213383 (invalid), same bug for parsing HTML, not limited to view
source.

Comment 11

[José Jeria]

*** Bug 232893 has been marked as a duplicate of this bug. ***

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

*** Bug 172947 has been marked as a duplicate of this bug. ***

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

--> me, since I'm going to fix this.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

Finally fix this bug. I think this should catch all missing > signs. If any
others crop up, file new bugs on them.

Also note: this bug contains most of the gruntwork for bug 263083. All that
remains for that bug is to actually color the erroneous tags.

I also made CMarkupDeclTokens not get lost when they are unclosed (at this
rate, bug 70828 is going to just go away :-) [not very likely]).

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

>Index: src/nsViewSourceHTML.cpp
>+  //Tokens are set in error if their ending > is not there, so don't output the
>+  //after-text

First, put spaces after the "//".   This isn't parser code style-wise.	;)

Second, a lot of other things can set a tag in error... Do they all mean that
the after-text shouldn't be shown?  I guess most things don't have
after-text...

>     nsAutoString afterText;
>     afterText.AssignWithConversion(kAfterText[aTagType]);
>     theContext.mITextToken.SetIndirectString(afterText);

While you're here, want to change that to:

  theContext.mITextToken.
     SetIndirectString(NS_ConvertASCIItoUTF16(kAfterText[aTagType]));

?


>+        if (!aToken->IsInError()) {
>+          theStr.AppendLiteral(">");
>+        }
>+        result=WriteTag(mCDATATag,theStr,0,aToken->IsInError());

Wouldn't that be more consistently handled via settin after-text for CDATA?  Or
is the after-text not highlighted the right color?

>+        if (!aToken->IsInError()) {
>+          theStr.AppendLiteral(">");
>+        }
>+        result=WriteTag(mMarkupDeclaration,theStr,0,aToken->IsInError());

Again.

r=bzbarsky with those changes as needed.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #15)

> First, put spaces after the "//".   This isn't parser code style-wise.	;)
> 

Ack, sorry. Actually, I was thinking about changing parser to have more
whitespace in general, and then I go and do this... Fixed.

> Second, a lot of other things can set a tag in error... Do they all mean that
> the after-text shouldn't be shown?  I guess most things don't have
> after-text...

Right, the reason most things don't have after-text is for that reason. It makes
sense (in my mind anyway), because it makes it more clear what the tag name is.
The code is sort of backwards because the special cases (start and end tags) are
handled by what seems to be the main line of code.

> 
> While you're here, want to change that to:
> 
>   theContext.mITextToken.
>      SetIndirectString(NS_ConvertASCIItoUTF16(kAfterText[aTagType]));
> 

Sure (I also am changing the like code from kBeforeText).

> Wouldn't that be more consistently handled via settin after-text for CDATA?  Or
> is the after-text not highlighted the right color?
> 

Basically, see my comments above. The short of this is that we want to color the
closing '>' for these tokens, and after-text isn't highlighted.

I'll have a new patch up soon.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

Updated to review comments.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

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

rbs, could you give this sr=, please?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

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

Since I've been asking rbs for lots of sr=s lately, shifting this one to dmose.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

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

sr=dmose

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 22

[Boris Zbarsky [:bzbarsky]]

This caused crash bug 264917 because my review comments in comment 15 were bogus
(was that last patch tested?).

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #22)
> This caused crash bug 264917 because my review comments in comment 15 were bogus
> (was that last patch tested?).

I was still fiddling with the .error style, so I did test it. I'm not sure how I didn't crash (bad luck maybe?).

Comment 24

[PikeUK]

*** Bug 273889 has been marked as a duplicate of this bug. ***

Comment 25

[Matthias Versen [:Matti]]

*** Bug 298623 has been marked as a duplicate of this bug. ***

Comment 26

[Jesse Ruderman]

*** Bug 309195 has been marked as a duplicate of this bug. ***