Closed Bug 431184 Opened 16 years ago Closed 15 years ago

Composer badly handles XHTML documents

Categories

(SeaMonkey :: Composer, defect)

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

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
seamonkey2.0

People

(Reporter: sgautherie, Assigned: neil)

References

Details

(5 keywords, Whiteboard: [fixed-seamonkey1.1.10: Av1 patch only][l10n-impact])

Attachments

(2 files, 1 obsolete file)

(Av1) <ComposerCommands.js> Adds |Components.results.| [Checkin: Comment 17 & 18]
1.78 KB, patch
neil
: review+
neil
: superreview+
kairo
: approval-seamonkey1.1.10+
Details | Diff | Splinter Review
Might make save work too! [Checkin: Comment 29]
6.51 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-seamonkey2.0+
Details | Diff | Splinter Review 
[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.
*...
(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.)
Keywords: regression
[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).
Flags: blocking-seamonkey2.0a1?
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?
This was +/- discussed in that bug,
but "braking" this testcase without any warning does not seem acceptable.
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?
(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.
(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".

[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.
Attachment #318726 - Flags: superreview?(sfraser_bugs)
Attachment #318726 - Flags: review?(sfraser_bugs)
Comment on attachment 318726 [details] [diff] [review]
(Av1) <ComposerCommands.js> Adds |Components.results.|
[Checkin: Comment 17 & 18]

Sorry, I can't review at present
Attachment #318726 - Flags: superreview?(sfraser_bugs)
Attachment #318726 - Flags: superreview?
Attachment #318726 - Flags: review?(sfraser_bugs)
Attachment #318726 - Flags: review?
Attachment #318726 - Flags: superreview?(neil)
Attachment #318726 - Flags: superreview?
Attachment #318726 - Flags: review?(neil)
Attachment #318726 - Flags: review?
Ah, but you tried it with valid xhtml - I just tried it myself and it doesn't work with only html.
Attachment #318726 - Flags: superreview?(neil)
Attachment #318726 - Flags: superreview+
Attachment #318726 - Flags: review?(neil)
Attachment #318726 - Flags: review+
Attachment #318726 - Flags: approval1.9?
(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 :-/)
Summary: Composer badly handles <about.xhtml> source, (maybe all XHTML documents ?) → Composer badly handles XHTML documents
Attachment #318726 - Flags: approval-seamonkey1.1.10?
From what I read here, this bug only applies to trunk, not to branch. Why do you request 1.1.10 approval here?
(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 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.
Keywords: checkin-needed
Whiteboard: [c-n: Av1, to Trunk // Leave opened]
Attachment #318726 - Flags: approval1.9?
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.
Attachment #318726 - Flags: approval-seamonkey1.1.10? → approval-seamonkey1.1.10+
Whiteboard: [c-n: Av1, to Trunk // Leave opened] → [c-n: Av1, to Trunk and 1.8.1 branch // Leave opened]
Assignee: nobody → sgautherie.bz
Depends on: 55313
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
Status: NEW → ASSIGNED
Whiteboard: [c-n: Av1, to Trunk and 1.8.1 branch // Leave opened] → [c-n: Av1, to 1.8.1 branch // Leave opened]
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
Keywords: checkin-needed
Whiteboard: [c-n: Av1, to 1.8.1 branch // Leave opened]
Attachment #318726 - Attachment description: (Av1) <ComposerCommands.js> Adds |Components.results.| → (Av1) <ComposerCommands.js> Adds |Components.results.| [Checkin: Comment 17]
Attachment #318726 - Attachment description: (Av1) <ComposerCommands.js> Adds |Components.results.| [Checkin: Comment 17] → (Av1) <ComposerCommands.js> Adds |Components.results.| [Checkin: Comment 17 & 18]
[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
}}
Assignee: sgautherie.bz → nobody
Status: ASSIGNED → NEW
Keywords: fixed-seamonkey1.1.10
Whiteboard: [fixed-seamonkey1.1.10: Av1 patch only]
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?
(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).
Flags: blocking-seamonkey2.0a1? → blocking-seamonkey2?
Keywords: relnote
Flags: blocking-seamonkey2.0? → blocking-seamonkey2.0+
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?
Whiteboard: [fixed-seamonkey1.1.10: Av1 patch only] → [fixed-seamonkey1.1.10: Av1 patch only][l10n-impact?]
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.
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.
Attached patch Make XHTML source work, mostly (obsolete) — — Splinter Review 
* (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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #401750 - Flags: review?(iann_bugzilla)
Whiteboard: [fixed-seamonkey1.1.10: Av1 patch only][l10n-impact?] → [fixed-seamonkey1.1.10: Av1 patch only][l10n-impact]
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
"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?
Attachment #401750 - Attachment is obsolete: true
Attachment #403046 - Flags: review?(iann_bugzilla)
Attachment #401750 - Flags: review?(iann_bugzilla)
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.
Attachment #403046 - Flags: review?(iann_bugzilla) → review+
Attachment #403046 - Flags: approval-seamonkey2.0+
Blocks: 519121
Pushed changeset 4c1841261a73 to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: fixed-seamonkey2.0
Resolution: --- → FIXED
Attachment #403046 - Attachment description: Might make save work too! → Might make save work too! [Checkin: Comment 29]
(In reply to comment #28)
> Please file a followup on the XHTML filter issue.

Bug 519121.
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a1pre) Gecko/20090928 SeaMonkey/2.1a1pre] (nightly) (W2Ksp4)

V.Fixed
Status: RESOLVED → VERIFIED
Depends on: 512226
Blocks: 512226
No longer depends on: 512226
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: