Composer badly handles XHTML documents

VERIFIED FIXED in seamonkey2.0

Status

SeaMonkey
Composer
--
major
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: neil@parkwaycc.co.uk)

Tracking

(Depends on: 1 bug, 5 keywords)

Trunk
seamonkey2.0
dataloss, fixed-seamonkey1.1.10, fixed-seamonkey2.0, regression, relnote
Dependency tree / graph
Bug Flags:
blocking-seamonkey2.0 +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
[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.
*...
(Reporter)

Comment 1

9 years ago
(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
(Reporter)

Comment 2

9 years ago
[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?
(Reporter)

Comment 4

9 years ago
Sure :-< Thanks.

Correct one:
<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&sortby=Date&hours=2&date=explicit&mindate=2007-11-13+02&maxdate=2007-11-14+04&cvsroot=%2Fcvsroot>
bug 207531 !
Blocks: 207531
OS: Windows 2000 → All
Hardware: PC → All
(Reporter)

Comment 5

9 years ago
This was +/- discussed in that bug,
but "braking" this testcase without any warning does not seem acceptable.
(Assignee)

Comment 6

9 years ago
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?
(Reporter)

Comment 7

9 years ago
(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.
(Reporter)

Comment 8

9 years ago
(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".
(Reporter)

Comment 9

9 years ago
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.
Attachment #318726 - Flags: superreview?(sfraser_bugs)
Attachment #318726 - Flags: review?(sfraser_bugs)

Comment 10

9 years ago
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?
(Reporter)

Updated

9 years ago
Attachment #318726 - Flags: superreview?(neil)
Attachment #318726 - Flags: superreview?
Attachment #318726 - Flags: review?(neil)
Attachment #318726 - Flags: review?
(Assignee)

Comment 11

9 years ago
Ah, but you tried it with valid xhtml - I just tried it myself and it doesn't work with only html.
(Assignee)

Updated

9 years ago
Attachment #318726 - Flags: superreview?(neil)
Attachment #318726 - Flags: superreview+
Attachment #318726 - Flags: review?(neil)
Attachment #318726 - Flags: review+
(Reporter)

Updated

9 years ago
Attachment #318726 - Flags: approval1.9?
(Reporter)

Comment 12

9 years ago
(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
(Reporter)

Updated

9 years ago
Attachment #318726 - Flags: approval-seamonkey1.1.10?

Comment 13

9 years ago
From what I read here, this bug only applies to trunk, not to branch. Why do you request 1.1.10 approval here?
(Assignee)

Comment 14

9 years ago
(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.
(Reporter)

Comment 15

9 years ago
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.
(Reporter)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [c-n: Av1, to Trunk // Leave opened]
(Reporter)

Updated

9 years ago
Attachment #318726 - Flags: approval1.9?

Comment 16

9 years ago
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+
(Reporter)

Updated

9 years ago
Whiteboard: [c-n: Av1, to Trunk // Leave opened] → [c-n: Av1, to Trunk and 1.8.1 branch // Leave opened]

Updated

9 years ago
Assignee: nobody → sgautherie.bz
(Reporter)

Updated

9 years ago
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]
(Reporter)

Updated

9 years ago
Attachment #318726 - Attachment description: (Av1) <ComposerCommands.js> Adds |Components.results.| → (Av1) <ComposerCommands.js> Adds |Components.results.| [Checkin: Comment 17]
(Reporter)

Updated

9 years ago
Attachment #318726 - Attachment description: (Av1) <ComposerCommands.js> Adds |Components.results.| [Checkin: Comment 17] → (Av1) <ComposerCommands.js> Adds |Components.results.| [Checkin: Comment 17 & 18]
(Reporter)

Comment 19

9 years ago
[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]

Comment 20

9 years ago
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?
(Reporter)

Comment 21

9 years ago
(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?

Updated

9 years ago
Keywords: relnote

Updated

8 years ago
Flags: blocking-seamonkey2.0? → blocking-seamonkey2.0+

Comment 22

8 years ago
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?

Updated

8 years ago
Whiteboard: [fixed-seamonkey1.1.10: Av1 patch only] → [fixed-seamonkey1.1.10: Av1 patch only][l10n-impact?]
(Assignee)

Comment 23

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

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

Comment 25

8 years ago
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #401750 - Flags: review?(iann_bugzilla)

Updated

8 years ago
Whiteboard: [fixed-seamonkey1.1.10: Av1 patch only][l10n-impact?] → [fixed-seamonkey1.1.10: Av1 patch only][l10n-impact]

Comment 26

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

Comment 27

8 years ago
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?
Attachment #401750 - Attachment is obsolete: true
Attachment #403046 - Flags: review?(iann_bugzilla)
Attachment #401750 - Flags: review?(iann_bugzilla)

Comment 28

8 years ago
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+

Updated

8 years ago
Attachment #403046 - Flags: approval-seamonkey2.0+
(Assignee)

Updated

8 years ago
Blocks: 519121
(Assignee)

Comment 29

8 years ago
Pushed changeset 4c1841261a73 to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: fixed-seamonkey2.0
Resolution: --- → FIXED
(Reporter)

Updated

8 years ago
Attachment #403046 - Attachment description: Might make save work too! → Might make save work too! [Checkin: Comment 29]
(Reporter)

Comment 30

8 years ago
(In reply to comment #28)
> Please file a followup on the XHTML filter issue.

Bug 519121.
(Reporter)

Comment 31

8 years ago
[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
(Reporter)

Updated

8 years ago
Depends on: 512226
(Reporter)

Updated

8 years ago
Blocks: 512226
No longer depends on: 512226

Updated

6 years ago
Duplicate of this bug: 484719
You need to log in before you can comment on or make changes to this bug.