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

RESOLVED FIXED in Future

Status

()

Core
HTML: Parser
RESOLVED FIXED
17 years ago
12 years ago

People

(Reporter: bz, Assigned: mrbkap)

Tracking

({testcase})

Trunk
Future
x86
Linux
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

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.
Created attachment 26788 [details]
Testcase
Created attachment 26789 [details]
Same testcase as text/plain

Updated

17 years ago
QA Contact: janc → bsharma

Comment 3

17 years ago
Futuring.  If a test case is presented showing this can lead to serious 
misinterpretaton of content, we will happily reconsider.
Target Milestone: --- → Future

Comment 4

17 years ago
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).

*** This bug has been marked as a duplicate of 57724 ***
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → DUPLICATE

Updated

16 years ago
QA Contact: bsharma → moied
Reopening 57724 dependencies for independent resolution.
Status: RESOLVED → REOPENED
Keywords: testcase
Resolution: DUPLICATE → ---
Depends on: 57724
*** Bug 49030 has been marked as a duplicate of this bug. ***

Comment 8

15 years ago
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

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

Comment 10

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

Comment 11

14 years ago
*** Bug 232893 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 12

13 years ago
*** Bug 172947 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 13

13 years ago
--> me, since I'm going to fix this.
Assignee: harishd → mrbkap
Status: REOPENED → NEW
(Assignee)

Comment 14

13 years ago
Created attachment 161828 [details] [diff] [review]
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]).
(Assignee)

Updated

13 years ago
Attachment #161828 - Flags: review?(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.
Attachment #161828 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 16

13 years ago
(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.
(Assignee)

Comment 17

13 years ago
Created attachment 162332 [details] [diff] [review]
patch v2

Updated to review comments.
(Assignee)

Comment 18

13 years ago
Comment on attachment 162332 [details] [diff] [review]
patch v2

rbs, could you give this sr=, please?
Attachment #162332 - Flags: superreview?(rbs)
(Assignee)

Comment 19

13 years ago
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.
Attachment #162332 - Flags: superreview?(rbs) → superreview?(dmose)
Comment on attachment 161828 [details] [diff] [review]
patch v1

sr=dmose
Attachment #161828 - Flags: superreview+
(Assignee)

Comment 21

13 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Attachment #162332 - Flags: superreview?(dmose)
This caused crash bug 264917 because my review comments in comment 15 were bogus
(was that last patch tested?).
Depends on: 264917
(Assignee)

Comment 23

13 years ago
(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

13 years ago
*** Bug 273889 has been marked as a duplicate of this bug. ***
*** Bug 298623 has been marked as a duplicate of this bug. ***

Comment 26

12 years ago
*** Bug 309195 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.