Last Comment Bug 431184 - Composer badly handles XHTML documents
: Composer badly handles XHTML documents
Status: VERIFIED FIXED
[fixed-seamonkey1.1.10: Av1 patch onl...
: dataloss, fixed-seamonkey1.1.10, fixed-seamonkey2.0, regression, relnote
Product: SeaMonkey
Classification: Client Software
Component: Composer (show other bugs)
: Trunk
: All All
: -- major (vote)
: seamonkey2.0
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
: 484719 (view as bug list)
Depends on: 55313
Blocks: 207531 512226 519121
  Show dependency treegraph
 
Reported: 2008-04-28 10:19 PDT by Serge Gautherie (:sgautherie)
Modified: 2011-09-18 03:03 PDT (History)
7 users (show)
iann_bugzilla: blocking‑seamonkey2.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
(Av1) <ComposerCommands.js> Adds |Components.results.| [Checkin: Comment 17 & 18] (1.78 KB, patch)
2008-04-30 17:44 PDT, Serge Gautherie (:sgautherie)
neil: review+
neil: superreview+
kairo: approval‑seamonkey1.1.10+
Details | Diff | Review
Make XHTML source work, mostly (5.82 KB, patch)
2009-09-20 14:58 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Review
Might make save work too! [Checkin: Comment 29] (6.51 KB, patch)
2009-09-26 11:59 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
iann_bugzilla: approval‑seamonkey2.0+
Details | Diff | Review

Description Serge Gautherie (:sgautherie) 2008-04-28 10:19:08 PDT
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008042801 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

(example) Steps:
1. Load <about:> in browser.
2. Ctrl+E to start editor.
3. Go to Source view.
4. Add a space somewhere.
5. Switch to Normal view.
5r.
(The new source is not taken into account, and:)
Error Console reports
{{
Error: mismatched tag. Expected: </link>.
Source File: 
Line: 7, Column: 3
Source Code:
</a></div></div></body></html>
}}
triggered by the following line
{{
<link type="text/css" href="chrome://global/skin/about.css"
rel="stylesheet">
}}

The root cause seems to be that this XHTML document was "wrongly" converted to an HTML document, then cannot be revalidate (as XHTML).

In the editor view source, you get:
{{
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">


<html xmlns="http://www.w3.org/1999/xhtml">
<head>

<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<title>About:</title>
<link type="text/css" href="chrome://global/skin/about.css"
rel="stylesheet">
</head>
}}
which you can compare with a View Source from the browser
or
<http://mxr.mozilla.org/seamonkey/source/toolkit/content/about.xhtml>

Ideas:
*Do not convert source to HTML in the first place, = support XHTML.
*Validate HTML converted source as HTML, not XHTML.
*Warn the user that XHTML is converted to HTML, or that XHTML is not supported/editable.
*...
Comment 1 Serge Gautherie (:sgautherie) 2008-04-28 10:42:49 PDT
(In reply to comment #0)
> *Warn the user that [...] XHTML is not supported/editable.

This used to be the case:


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.13) Gecko/20060414] (release) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.12) Gecko/20070510 SeaMonkey/1.0.9] (release) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.13) Gecko/20080313 SeaMonkey/1.1.9] (release) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/20070515 SeaMonkey/1.5a] (nightly) (W2Ksp4)

Get an alert dialog at step 2:
{{
Alert
This type of page can't be edited.
}}

This code is at
<http://mxr.mozilla.org/seamonkey/search?string=CantEditMimeTypeMsg&case=on&tree=seamonkey>
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/ui/composer/content/editor.js&rev=1.249&mark=371-373,384-387#365>


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008022601 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008042801 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Has this bug.

(I'll try to narrow down the timeframe.)
Comment 2 Serge Gautherie (:sgautherie) 2008-04-28 11:32:44 PDT
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a6pre) Gecko/2007062802 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007082503 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007093003 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007101611 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007102302 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Good.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007102903 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

(Ctrl+E crashes.)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b2pre) Gecko/2007111202 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b2pre) Gecko/2007111302 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Good.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b2pre) Gecko/2007111403 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b2pre) Gecko/2007111503 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b2pre) Gecko/2007111903 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2007123003 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Bad.

Then:
<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=MOZILLA_1_8_BRANCH&branchtype=match&sortby=Date&hours=2&date=explicit&mindate=2007-11-13+02&maxdate=2007-11-14+04&cvsroot=%2Fcvsroot>
Only 5 checkins :-)
Guessing: bug 402649, bug 402460, (bug 403331).
Comment 3 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2008-04-28 12:16:01 PDT
I'm confused by your bonsai link. It seems to give a regression range for branch, while you only posted trunk build id's. That bonsai link is incorrect, isn't it?
Comment 5 Serge Gautherie (:sgautherie) 2008-04-28 12:41:39 PDT
This was +/- discussed in that bug,
but "braking" this testcase without any warning does not seem acceptable.
Comment 6 neil@parkwaycc.co.uk 2008-04-30 16:19:59 PDT
Interesting... so you can start with about: and edit it normally and save it as HTML and even view the source but then rebuilding it fails? I suspect this is because it always uses an HTML serialiser but uses a parser appropriate to the type of document originally loaded.

I'm not sure what should happen if you try to edit invalid XHTML. (Invalid HTML simply gets "fixed" by the parser.)

Does the "Insert HTML" function also fail if you edit an XHTML document?
Comment 7 Serge Gautherie (:sgautherie) 2008-04-30 17:21:23 PDT
(In reply to comment #6)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008022601 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Testing with
*<about:>
*Bug 428739 attachment 315327 [details] (trivial XHTML document).

> Interesting... so you can start with about: and edit it normally and save it as
> HTML and even view the source but then rebuilding it fails? I suspect this is

I can't save the file:
"Save" Toolbar button, and "File > Save As..." menu item, are active, but don't work; [2nd bug !]

Error Console reports
{{
Error: NS_ERROR_NOT_IMPLEMENTED is not defined
Source File: chrome://editor/content/ComposerCommands.js
Line: 1676
}}
See
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/ui/composer/content/ComposerCommands.js&rev=1.206&mark=1674-1676#1654>
[3rd bug !]

(timeless, how "Save" could have been working in bug 207531 comment 0 ?)

> because it always uses an HTML serialiser but uses a parser appropriate to the
> type of document originally loaded.

That is what I was guessing...

> I'm not sure what should happen if you try to edit invalid XHTML. (Invalid HTML
> simply gets "fixed" by the parser.)

(We'll see when valid XHTML "works" ;->)

> Does the "Insert HTML" function also fail if you edit an XHTML document?

Yes, "Insert > HTML... > <i>Hello</i>" works fine.
Comment 8 Serge Gautherie (:sgautherie) 2008-04-30 17:23:56 PDT
(In reply to comment #7)
> > Does the "Insert HTML" function also fail if you edit an XHTML document?
> Yes, "Insert > HTML... > <i>Hello</i>" works fine.

== "No, it does not fail; it works".
Comment 9 Serge Gautherie (:sgautherie) 2008-04-30 17:44:42 PDT
Created attachment 318726 [details] [diff] [review]
(Av1) <ComposerCommands.js> Adds |Components.results.|
[Checkin: Comment 17 & 18]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008022601 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Fix error noticed in comment 7.
Comment 10 Simon Fraser 2008-04-30 19:35:54 PDT
Comment on attachment 318726 [details] [diff] [review]
(Av1) <ComposerCommands.js> Adds |Components.results.|
[Checkin: Comment 17 & 18]

Sorry, I can't review at present
Comment 11 neil@parkwaycc.co.uk 2008-05-01 13:54:52 PDT
Ah, but you tried it with valid xhtml - I just tried it myself and it doesn't work with only html.
Comment 12 Serge Gautherie (:sgautherie) 2008-05-01 15:57:44 PDT
(In reply to comment #11)
> Ah, but you tried it with valid xhtml - I just tried it myself and it doesn't
> work with only html.

(I'm not sure what your "it" refers to :-/)
Comment 13 Robert Kaiser (not working on stability any more) 2008-05-01 16:17:49 PDT
From what I read here, this bug only applies to trunk, not to branch. Why do you request 1.1.10 approval here?
Comment 14 neil@parkwaycc.co.uk 2008-05-01 16:48:11 PDT
(In reply to comment #13)
> From what I read here, this bug only applies to trunk, not to branch. Why do
> you request 1.1.10 approval here?
The patch in question fixes a bug in the Composer save UI that happens to be easily exposed by this bug. (I don't know how easily it might be reproducible on the branch.)

Note that I don't think Firefox uses this file so it won't need 1.9 approval.
Comment 15 Serge Gautherie (:sgautherie) 2008-05-01 16:52:21 PDT
Comment on attachment 318726 [details] [diff] [review]
(Av1) <ComposerCommands.js> Adds |Components.results.|
[Checkin: Comment 17 & 18]

(In reply to comment #13)
> From what I read here, this bug only applies to trunk, not to branch. Why do
> you request 1.1.10 approval here?

Av1 patch is not directly related to this bug:
I simply noticed it while testing here.
It fixes "non-working" code which branch has too.

As comment 9 says, "Fix error noticed in comment 7".
As far as I saw (with Trunk), it does not actually fix anything else.
Comment 16 Robert Kaiser (not working on stability any more) 2008-05-02 05:52:47 PDT
Comment on attachment 318726 [details] [diff] [review]
(Av1) <ComposerCommands.js> Adds |Components.results.|
[Checkin: Comment 17 & 18]

I still don't know what would fail on branch that we fix, but from the explanation Neil gave, it seems to be the right thing to do and should not be risky.
Comment 17 Reed Loden [:reed] (use needinfo?) 2008-05-07 03:17:54 PDT
Checking in editor/ui/composer/content/ComposerCommands.js;
/cvsroot/mozilla/editor/ui/composer/content/ComposerCommands.js,v  <--  ComposerCommands.js
new revision: 1.208; previous revision: 1.207
done
Comment 18 Reed Loden [:reed] (use needinfo?) 2008-05-07 03:43:50 PDT
MOZILLA_1_8_BRANCH:

Checking in editor/ui/composer/content/ComposerCommands.js;
/cvsroot/mozilla/editor/ui/composer/content/ComposerCommands.js,v  <--  ComposerCommands.js
new revision: 1.201.4.4; previous revision: 1.201.4.3
done
Comment 19 Serge Gautherie (:sgautherie) 2008-05-07 09:35:29 PDT
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008050708 SeaMonkey/2.0a1pre] (SEA-WIN32-TBOX-trunk) (W2Ksp4)

V.Fixed, for the Av1 patch part.

New "expected" error is:
{{
Error: [Exception... "'Method not implemented' when calling method: [nsIControllerCommand::doCommand]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 86"  data: no]
Source File: chrome://global/content/globalOverlay.js
Line: 86
}}
Comment 20 Robert Kaiser (not working on stability any more) 2008-09-09 07:05:45 PDT
So, is this bug a regression of trunk vs. branch, i.e. it works on 1.1.x but breaks in 2.0a1pre?
If so, how common is this to happen? I wonder if this really needs to block Alpha 1 or if it can be pushed out to a later milestone, final or whatever.
If it should block, who can work on it?
Comment 21 Serge Gautherie (:sgautherie) 2008-09-09 07:15:58 PDT
(In reply to comment #20)
> So, is this bug a regression of trunk vs. branch, i.e. it works on 1.1.x but
> breaks in 2.0a1pre?

Yes, see comment 1.

> if it can be pushed out to a later milestone, final or whatever.

I swapped the flags.
I would think this may be worth a relnote, no more (for the 2.0a1).
Comment 22 Robert Kaiser (not working on stability any more) 2009-09-18 15:39:43 PDT
Neil, I don't know anyone but you who knows anything about Composer code - what can we do to get some resolution here, even if it might just be getting back the warning that 1.x had?
Comment 23 neil@parkwaycc.co.uk 2009-09-20 09:52:10 PDT
So, it turns out that RebuildDocumentFromSource actually assumes that we're using an HTML parser, since even if you happen to feed it valid XHTML for an XHTML editor it chops it up into two invalid substrings.

Maybe we could get away with disabling source mode for XHTML.
Comment 24 Robert Kaiser (not working on stability any more) 2009-09-20 14:06:38 PDT
I think disabling the source mode for xhtml is bad, but good enough for a temporary solution for 2.0 if a followup is filed to make it better in the future.
Comment 25 neil@parkwaycc.co.uk 2009-09-20 14:58:47 PDT
Created attachment 401750 [details] [diff] [review]
Make XHTML source work, mostly

* (Untested!) Saves the document using its real MIME type
* Converts the document to source using its real MIME type
* Manually rebuilds the document from XHTML source
(rebuildDocumentFromSource is smart, but HTML-dependent)
* Manually deletes the body so that the editor notices
* Checks for valid XHTML when automatically switching out of source view
* Disables object resizing in all tags mode

I don't know why, but the validity check does not happen when you manually switch out of source view; I use Save and Change Character Encoding to trigger it. The validity check isn't necessary for HTML as rebuildDocumentFromSource was enhanced to handle almost anything message compose could throw at it.
Comment 26 Ian Neal 2009-09-26 10:36:15 PDT
Comment on attachment 401750 [details] [diff] [review]
Make XHTML source work, mostly

At the moment you cannot save a page with type "application/xhtml+xml", probably need to add that type to kSupportedTextMimeTypes at http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/ComposerCommands.js#1630
Comment 27 neil@parkwaycc.co.uk 2009-09-26 11:59:58 PDT
Created attachment 403046 [details] [diff] [review]
Might make save work too!
[Checkin: Comment 29]

"Save As" now offers to save .xhtml but displays HTML files... not quite right, but we don't have an obvious way of showing XHTML files. Maybe a followup bug?
Comment 28 Ian Neal 2009-09-27 11:03:34 PDT
Comment on attachment 403046 [details] [diff] [review]
Might make save work too!
[Checkin: Comment 29]

r=me
Please file a followup on the XHTML filter issue.
Comment 29 neil@parkwaycc.co.uk 2009-09-27 11:55:09 PDT
Pushed changeset 4c1841261a73 to comm-central.
Comment 30 Serge Gautherie (:sgautherie) 2009-09-28 09:56:14 PDT
(In reply to comment #28)
> Please file a followup on the XHTML filter issue.

Bug 519121.
Comment 31 Serge Gautherie (:sgautherie) 2009-09-28 09:57:05 PDT
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a1pre) Gecko/20090928 SeaMonkey/2.1a1pre] (nightly) (W2Ksp4)

V.Fixed
Comment 32 Ian Neal 2011-09-18 03:03:59 PDT
*** Bug 484719 has been marked as a duplicate of this bug. ***

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