Closed Bug 216609 Opened 21 years ago Closed 19 years ago

Error - required <head> tag is missing

Categories

(SeaMonkey :: Composer, defect)

Product:
SeaMonkey
Component:
Composer
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: d_king, Assigned: d_king)

Details

(Keywords: regression, Whiteboard: Workaround: ensure your HTML uses lowercase "head" and "body" elements.)

Attachments

(1 file, 3 obsolete files)

regexp patch
2.09 KB, patch
glazou
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030718
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030718

I was testing Bug #25141 by entering the HTML code from the example into
Composer in "<HTML> Source" mode. When I select File-SaveAs I get the error "The
HTML source could not be converted back into the document because the required
<head> tag is missing. Please add it."

The HTML I entered is as follows :-
<HTML>
<HEAD>
<TITLE>Test</TITLE>
</HEAD>
</html>

Note that <HEAD> and </HEAD> both exist.

Reproducible: Always

Steps to Reproduce:
1. From browser select File-New-ComposerPage
2. Enter HTML in "<HTML> Source" mode
3. Select File-SaveAs from same mode.

Actual Results:  
Error message.

Expected Results:  
Prompt for filename to save as.

I've never accessed Composer this way. Up to now I've used Window-Composer.

Selecting "Major" as can't use File-SaveAs, and error message is hopelessly wrong.
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5b) Gecko/20030728

Looks like the uppercase in <HEAD> or <BODY> is critical:

1.Paste this into a empty composer document (in "source" mode) - for me it
doesn't seem to matter how I start Composer. 

<html>
<head>
<title>Test</title>
</head>
<body>
</body>
</html>

2. Changing <head> to <HEAD> or <body> to <BODY> will cause an error message
while trying to save the file (Composer thinks one of the required tags are
missing, ie <head> or <body>)

Nothing happens if <html>, <title> or any end tag are written in uppercase
letters - saving works fine then.
Changing Hardware and OS to ALL
OS: Windows XP → All
Hardware: PC → All
Yup, and I found the code that does it.
http://lxr.mozilla.org/seamonkey/source/editor/ui/composer/content/editor.js#1848
is a function FinishHTMLSource() that includes the edit 

var beginHead = htmlSource.indexOf("<head");

but doesn't have an edit for the uppercase version, or maybe htmlSource is case
sensitive when it shouldn't be (for this instance anyway).
The workaround is (of course) to always write the <head> and <body> tags in
lowercase...

So, adding a check for uppercase <HEAD> and <BODY> in FinishHTMLSource() will
fix the problem? I wonder if there's a point in not allowing uppercases? CC:ing
Harry Lu, who wrote the patch for bug 154521.
I'm currently testing some possible fixes for uppercase <BODY>. My first attempt
using toLowerCase didn't work (probably due to my limited JS skills) so now I'm
testing a brute force expansion of the If statement.

As for bug #154521. It doesn't look like anyone considered uppercase as being an
issue, mainly due to it being a quick fix for one particular problem.

Hmmm, uppercase tags are legit aren't they? Oh well, I suppose I'd better check
the spec to be sure.
Since I'm having a go at a patch for this bug, assigning to myself.

From reading the W3 spec for HTML 4.01, I couldn't find any comment about what
case <HEAD> should be, although all examples are in upper case.

Also adding "regression" keyword as the fix for Bug #154521 caused this problem.
That bug also fixed other problems, so I decided not to reopen it.
Assignee: composer → d_king
Keywords: regression
Attached patch Brute force patch (obsolete) — — Splinter Review 
I'm not 100% happy with this patch, but it will fix the bug at hand. I'd rather
use toLowerCase, but until I sort out how to use it, this is the solution.
Comment on attachment 130156 [details] [diff] [review]
Brute force patch

Requesting r, if anyone is listening at composer@editor.bugs
Attachment #130156 - Flags: review?(composer)
Attachment #130156 - Flags: superreview?(daniel)
Comment on attachment 130156 [details] [diff] [review]
Brute force patch

I am not super-reviewer. If you want me to r= this patch, please resubmit a
request through the attachment manager, thanks.
Attachment #130156 - Flags: superreview?(daniel) → superreview-
Comment on attachment 130156 [details] [diff] [review]
Brute force patch

Do you know who the right person to do a sr is? Bugzilla just tells me that
"composer@editor.bugs" is the owner!
Attachment #130156 - Flags: review?(composer) → review?(daniel)
Please download a more recent build and try again to hit the bug you
describe here. I am using a WinXP 20030823 debug build and I can't reproduce.

Comment on attachment 130156 [details] [diff] [review]
Brute force patch

>-      var beginHead = htmlSource.indexOf("<head");
>-      if (beginHead == -1)
>+      if (htmlSource.indexOf("<head") == -1 && htmlSource.indexOf("<HEAD") == -1 && htmlSource.indexOf("<Head") == -1)

Urgh. Sorry, this is definitely too expensive and suboptimal. Please use
a RegExp matching with case insensitivity.

>-      var beginBody = htmlSource.indexOf("<body");
>-      if (beginBody == -1)
>+      if (htmlSource.indexOf("<body") == -1 && htmlSource.indexOf("<BODY") == -1 && htmlSource.indexOf("<Body") == -1)

Same thing here.
See
http://devedge.netscape.com/library/manuals/2000/javascript/1.3/reference/regex
p.html#1193136
Attachment #130156 - Flags: review?(daniel) → review-
I've just duplicated the problem with the released Mozilla 1.5 beta (2003082704).

As for the code being suboptimal, you are being too kind. In my mind, what I
wrote was horrible, horrendous, and I should be first against the wall when the
revolution comes (apologies to Douglas Adams).

Many thanks for the link, I'll investigate that. There is an existing function
to convert case, but I couldn't get it to work, hence the horrible thing I put
up for review.

        Attachment #130156 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 130712 [details] [diff] [review]
Convert to lowercase before compare.

A much better patch. Convert the source to lowercase before doing the compare.
I used "toLowerCase" as it's used all over the place in editor.js.

        Attachment #130712 -
        Flags: review?(daniel)

    
  

        Attachment #130712 -
        Flags: review?(daniel)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Even better patch (obsolete)
        — 
          — Splinter Review
      

    


  
Don't convert zero-length strings to lower case.

        Attachment #130712 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 130713 [details] [diff] [review]
Even better patch

This one should be better. Seeking "r".

        Attachment #130713 -
        Flags: review?(daniel)

    
  

        Attachment #130713 -
        Flags: superreview?(sfraser)

        Attachment #130713 -
        Flags: review?(kaie)

        Attachment #130713 -
        Flags: review?(daniel)

    
    

    
  
Comment on attachment 130713 [details] [diff] [review]
Even better patch

>     var htmlSource = gSourceTextEditor.outputToString(kTextMimeType, 1024); // OutputLFLineBreak
>     if (htmlSource.length > 0)
>     {
>+      htmlSource = htmlSource.toLowerCase();
>       var beginHead = htmlSource.indexOf("<head");
>       if (beginHead == -1)
>       {


This is still suboptimal. You're converting the entire source (which may be
several MB of text) to lowercase just to do compares. Please follow up on
Daniel Glazman's suggestions above of using a regex.

        Attachment #130713 -
        Flags: superreview?(sfraser) → superreview-

    
    

    
  
Hmmm, that's a point, htmlSource could be rather large.

New patch coming....

    
    

    
  
Comment on attachment 130713 [details] [diff] [review]
Even better patch

Cancelling review request.

        Attachment #130713 -
        Attachment is obsolete: true

        Attachment #130713 -
        Flags: review?(kaie)

    
    

    
  
Added info to whiteboard for those creating HTML 4.x or below web pages which
accept uppercase elements (unlike XHTML).

I'm still fighting with regexp. I have the code, but it's being difficult about
working properly.
Status: NEW → ASSIGNED
Whiteboard: Workaround: ensure your HTML uses lowercase "head" and "body" elements.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        regexp patch
        — 
          — Splinter Review
      

    


  
OK, after using Venkman to confirm my pattern matching, and the correct method
of using the "i" flag, I have this patch.

Tested on Mozilla with <HEAD>, <Head> and <head>.

Also, I found another copy of editor.js that hides in
mozilla/composer/base/content which appears to be an exact duplicate of the one
in mozilla/editor/ui/composer/content.

Both copies are in this patch.

    
    

    
  
Comment on attachment 137488 [details] [diff] [review]
regexp patch

Asking for r and sr.

        Attachment #137488 -
        Flags: superreview?(sfraser)

        Attachment #137488 -
        Flags: review?(kaie)

    
    

    
  
Please ask another editor person for review.
I think I should no longer be a peer of editor, I don't know enough about it to
do reviews.


    
    

    
  
Comment on attachment 137488 [details] [diff] [review]
regexp patch

As per request, changing reviewer.

        Attachment #137488 -
        Flags: review?(kaie) → review?(neil.parkwaycc.co.uk)

    
    

    
  
glazou: you need to use File/Save As to trigger the bug, clicking on Normal mode
will simply ignore any document without <html>, <head> or <body> tags.

    
    

    
  
Comment on attachment 137488 [details] [diff] [review]
regexp patch

Changing reviewer.

        Attachment #137488 -
        Flags: review?(neil.parkwaycc.co.uk) → review?(cmanske)

    
    

    
  
Comment on attachment 137488 [details] [diff] [review]
regexp patch

.

        Attachment #137488 -
        Flags: review?(cmanske) → review?(daniel)
Product: Browser → Seamonkey

    
  

        Attachment #137488 -
        Flags: review?(daniel) → review?(composer)

    
    

    
  
Simple fix, just needs minimal attention to review and check-in.
Flags: blocking1.8b?
Flags: blocking-aviary1.1?

    
    

    
  
Too late for 1.8b1, moving nomination to 1.8b2, but the real trick is to get
reviews on the patch and get it in.
Flags: blocking1.8b?
Flags: blocking1.8b2?
Flags: blocking1.8b-

    
  

        Attachment #137488 -
        Flags: review?(composer) → review?(daniel)

    
  

        Attachment #137488 -
        Flags: superreview?(sfraser_bugs) → superreview?(composer)

    
    

    
  
This just needs a "sr" and it's ready to be checked in. Very very minor fix.

    
  
Flags: blocking-seamonkey1.0a?

    
    

    
  
Comment on attachment 137488 [details] [diff] [review]
regexp patch

you should request sr of a specific person...

    
    

    
  
I just used the information at
https://bugzilla.mozilla.org/describecomponents.cgi?product=Mozilla%20Application%20Suite
which I got to by following the "Component" link.

    
  

        Attachment #137488 -
        Flags: superreview?(composer) → superreview?(daniel)

    
    

    
  
If we were able to ship stable releases with this, we won't hold an alpha of the
new suite for that. Please request sr= from a real super reviewer though (there
should be a list of them on mozilla.org - a tip: look who did sr on patches
checked in in the last couple of days/weeks)
Flags: blocking-seamonkey1.0a? → blocking-seamonkey1.0a-

    
    

    
  
(In reply to comment #34)
> I just used the information at
>
https://bugzilla.mozilla.org/describecomponents.cgi?product=Mozilla%20Application%20Suite
> which I got to by following the "Component" link.

That list is unrelated to reviewers and super-reviewers. see
http://mozilla.org/hacking/reviewers.html for the list of super-reviewers.

    
  

        Attachment #137488 -
        Flags: superreview?(daniel) → superreview?(neil.parkwaycc.co.uk)

    
    

    
  
(In reply to comment #35)
> If we were able to ship stable releases with this, we won't hold an alpha of the
> new suite for that.

I'm not sure why Asa set the blocking-seamonkey1.0a to ?.

    
    

    
  
(In reply to comment #36)
> That list is unrelated to reviewers and super-reviewers. see
> http://mozilla.org/hacking/reviewers.html for the list of super-reviewers.

According to that link, Neil is the only super-reviewer for composer. Changed
the flag to ask him for a sr.


    
    

    
  
Since attachment 150948 [details] [diff] [review] and its follow-up fix were checked in last July,
most of FinishHTMLSource should no longer be necessary.

    
    

    
  
Hmm... although I filed bug 252423 I forgot to follow up on it :-[

    
    

    
  
(In reply to comment #37)
> I'm not sure why Asa set the blocking-seamonkey1.0a to ?.

he moved blocking-1.8b2? requests to blocking-seamonkey1.0a? in the moz. app
suite product.

    
    

    
  
OK, so part of the fix for Bug #192557 may have fixed this problem as well.
Working for over a year to get a r and sr only to find that it was probably
fixed elsewhere.....

Oh well, I suppose I should double check the fix.

    
    

    
  
Fixed by part of checkin to Bug #192557.
Bug #252423 will remove the code that this bug patches.

Fixed.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 137488 [details] [diff] [review]
regexp patch

Removing obsolete request.

        Attachment #137488 -
        Flags: superreview?(neil.parkwaycc.co.uk)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: