Closed Bug 154521 Opened 23 years ago Closed 23 years ago

In "Composer", all source I inputed will be removed by switching display mode between "<html>Source" and "Preview";

Categories

(SeaMonkey :: Composer, defect)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: alvin.duan, Assigned: harry.lu)

Details

Attachments

(1 file, 7 obsolete files)

New patch to address comment #25
7.18 KB, patch
cmanske
: review+
sfraser_bugs
: superreview+
asa
: approval+
Details | Diff | Splinter Review
[Testing Environment] Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.0rc3) Gecko/20020606 Netscape/7.0b1 SunOS offense 5.9 Generic sun4u sparc SUNW,Sun-Blade-100 [Repro Steps] 1. Open your browser and launch "Composer" component; 2. Replace all below default value with yourself input in "<html>Source" tab; <head> <title></title> <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1"> </head> <body> <br> </body> 3. Switch display mode to "Preview"; 4. And switch display mode back to "<html>Source"; [Bug] 1. Nothing displayed in "Preview" mode; 2. All your input will be disappeared when you back "<html>Source" mode; [Expect Result] All my input should be display in "Preview" mode; And if there are some errors in source, I should be given a chance to correct them in "<html>Source" mode;
-> Composer
Assignee: Matti → syd
Component: Browser-General → Editor: Composer
QA Contact: imajes-qa → sujay
It seemes when I input sth. wrong in the "<html>Source" tab, switch to "Preview" tab, then back to "<html>Source" tab, the composer will try to correct my mistake "intellligently". Why not just leave it alone and let me correct it by hand?
From the code, nsHTMLEditor::RebuildDocumentFromSource(), the user's input html source must contain <body> and <head>. Or the function will fail silently with returning NS_ERROR_FAILURE. The "Preview" tab will keep the content of last time. However, the <head> and <body> tag seem optional for a HTML document in HTML 4.01 specification. The workaround is to never remove the generated <body> and <head> tag. Keep them, you will be safe.
seek r= for this patch Now when user want to change to other tabs from HTML SOURCE tab, a check will be made to ensure the HTML source contains <head> and <body> tag. If not, a dialog will popup to alter user and user cannot move to other tabs. This can avoide accidently lost of user's input. The reporter (alvin.duan@sun.com) has agreed with this behavior.
reassign to Harry Lu
Assignee: syd → harry.lu
seek r= for above patch
I don't have a problem with the patch as such -- the code looks fine and works as advertised. But it seems very specific to look only for those two problems; couldn't there be lots of other problems that might cause RebuildDocumentFromSource to fail? Wouldn't it be better to check the return value of RebuildDocumentFromSource, and put up a dialog if it failed for any reason, and then keep the user in source mode to try and figure out the problem (which admittedly might not be obvious, since the parser probably won't tell us where the error occurred)? That said, it probably is a good idea to check specifically for these two specific errors (since we can then tell the user exactly what's wrong), so I'm marking this r=akkana anyway, even though I'd also like to see a more general solution and would like to understand why we weren't throwing an exception without Harry's patch. Kathy, Kin, other editor folk: any thoughts?
Attachment #90284 - Flags: review+
I'd like to have Robin help us with the strings added at the end of this patch.
Thank Akkana for the quick review! Yes, the nsHTMLEditor::RebuildDocumentFromSource() could fail for many reasons, but I don't think it is good to repeat all the check logic of C++ in JS again. And the exception returned by RebuildDocumentFromSoure() is too general. What's more, RebuildDocumentFromSoure() is called in JS function of SetEditMode (mode), which has no return value now and also called in some other places besides the action of TAB and MENU. Catch the exception of RebuildDocumentFromSource() in SetEditMode() will need to change some other JS code. I hesitate to make complex changes here just for this bug. The most popular reason of the failure is the lack of <head> and <body> tag in user input HTML source, so I think just catch it should be OK now. My patch is simple and more specific, though it cannot handle all situations. Anyway, it can satisfy the bug reporter :) If no other problems, could someone give it a sr=? Thanks!
Whiteboard: seeking sr=
+NoHeadTag=Cannot find <head> tag in HTML source! Please modify it. +NoBodyTag=Cannot find <body> tag in HTML source! Please modify it. Suggested text: NoHeadTag=The required <head> and </head> tags are missing from the HTML source. Please add them and try again. NoBodyTag=The required <body> and </body> tags are missing from the HTML source. Please add them and try again.
This might be a little better written as: NoHeadTag=The HTML source could not be converted back into the document because the required <head> and </head> tags are missing. Please add them. or something like that. Personally, I dislike "please try again" messages in UI. They make it sound like the software is fragile, such that the user has to keep trying things to make something work.
Your suggestions for the error message text are fine with me.
I had modified the patch according to Simon Fraser's comment #11. I also add a small check in CheckNecessaryTags() : + if (source.length == 0) //Empty document, let it go + return true; + Thus if user delete all the document content, we can give him a default content. Akkana and others, could you please give it a r= again? Thanks!
Attachment #90284 - Attachment is obsolete: true
Sorry, I just found a extra space in the alert string. Remove it.
Attachment #90715 - Attachment is obsolete: true
Whiteboard: seeking sr= → seeking r= and sr=
Sorry again. I just found the variable "source" is not declared in CheckNecessaryTags(). Now I change it to htmlSource and add a "var" before it. Please r= this new patch. Thanks!
Attachment #90716 - Attachment is obsolete: true
A few questions about the patch: * Why look for "<head" instead of "<head>"? What happens if the user accidentally types "foo" in the head tag like this: <headfoo> Does the error situation occur? * Does anything go wrong if only the end tags are omitted? If not, the error strings should be shortened since we aren't specifically checking for the end tags. Why is CheckNecessaryTags() defined in editor.js when ComposerCommands.js is the only caller? (I'm not sure if this matters or not...)
<q>Why look for "<head" instead of "<head>"?</q> <head> would not match, for example, <head lang="en">. Also, I loose everything even if head and body tags are present. ie... 1. open composer 2. switch to html source tab 3. enter random text between <body> and </body> 4. switch to any other tab 5. switch back actual result: it's back to the template document expected result: remember what I put there. It seems that anything done in html source tab doesn't change anything. composer would be really handy when you find a snippet of html and want to see what it does... copy, paste in composer, preview... voila. I'm using 2002070904.
Brade, Thanks for your comments. *The check logic is actually bought from RebuildDocumentFromSource() in C++: if (!FindInReadable(NS_LITERAL_STRING("<body"),beginbody,endbody)) return NS_ERROR_FAILURE; and if (!FindInReadable(NS_LITERAL_STRING("<head"),beginhead,endhead)) return NS_ERROR_FAILURE; And as I just tried, add sth. after the tag would not cause the Rebuild* function fail. Composer will keep it and generate a new pair of <head> </head>tag to enclose them. *Yes, the end tags can be omitted. I will modify the alert message. *As for the function location, I will move it to ComposerCommands.js. Jay Knight, Yes I can reproduce your problem with 2002071008. You can see JS error from Tools-->web development-->JavaScript console. But this seemed a newly introduced bug. Not related to this one. You can check the history log of editor.js. Akkana should had known about this.
Modify alert string according to Brade's comment. Also move CheckNecessaryTags() from editor.js to ComposerCommands.js. Please r= this patch. Thanks!
Attachment #90718 - Attachment is obsolete: true
Comment on attachment 90899 [details] [diff] [review] Modify alert string and move function location r=brade
Attachment #90899 - Flags: review+
Hrmm, not sure I would have fixed it that way. Why not make FinishHTMLSource() show the dialog, then just throw an exception if the source is malformed? It seems that would be much cleaner.
According to Simon Fraser's comment above, move the check into FinishHTMLSource(). If the check fails, popup the dialog and throw a exception sliently. Now the user will also see the dialog when they want to save, save as, exportToText, publish, publishAs, print, etc. Please r= and sr= this patch. Thanks!
Attachment #90899 - Attachment is obsolete: true
Comment on attachment 91459 [details] [diff] [review] Move the check into FinishHTMLSource() r=brade (although I still wish we checked for "<head>" or "<head " and similar for body tag)
Attachment #91459 - Flags: review+
Did you verify that in all places where FinishHTMLSource() is called, we allow the exception to propagate out far enough that we don't do anything bad?
Per Simon Fraser's comment #24, I am verifying all the places where FinishHTMLSource() is called. I have some questions here, please help. 1. In editor.js, function CheckAndSaveDocument(), it calles FinishHTMLSource (): if (gHTMLSourceChanged) FinishHTMLSource(); Since FinishHTMLSource() now might throw out a exception, we have to catch it here or the window will be closed anyway. I want to change the code to : if (gHTMLSourceChanged) { try { FinishHTMLSource(); } catch (e) { return false;} } Is this OK? 2. In ComposerCommands.js, nsRevertCommand's doCommand(): if(result == 0) { FinishHTMLSource(); window.editorShell.LoadUrl(GetDocumentUrl()); } Since we will reload the document, why do we need to call FinishHTMLSource() for current document here? I wonder if I can remove the call here. If we just want to switch to normal view, we can call SetEditMode(DisplayModeNormal) after the LoadUrl. 3. In editorApplicationOverlay.js, function editPage(url, launchWindow, delay), there are some lines: if (emptyWindow.IsInHTMLSourceMode()) emptyWindow.FinishHTMLSource(); emptyWindow.editorShell.LoadUrl(url); Save question as 2. Do we need to call emptyWindow.FinishHTMLSource() here? 4. There is a nsQuitCommand defined in ComposerCommands.js and related to "cmd_quit". But acutally it is not used in anywhere. The menu's Exit/Quit is actually comes from platformCommunicatorOverlay.xul. So I suggest to reomve the related code lines from ComposerCommands.js. Correct me if I am wrong.
I had verified all the places that call FinishHTMLSource(). This patch catch the exception in CheckAndSaveDocument() so that the window wounldn't be closed when user choose File->Exit/Quit if the tag is missing:(see my question 1 above) - if (gHTMLSourceChanged) - FinishHTMLSource(); + if (gHTMLSourceChanged) { + try { + FinishHTMLSource(); + } catch (e) { return false;} + } And I also change the call to FinishHTMLSource() to SetEditMode(PreviousNonSourceDisplayMode) in nsRevertCommand's doCommand() since no need to check the tags when reverting from saved document:(see my question 2 above): - FinishHTMLSource(); + SetEditMode(PreviousNonSourceDisplayMode); I don't change editPage(url, launchWindow, delay) since the emptyWindow has been checked with PageIsEmptyAndUntouched() before. And a empty document couldn't fail the check of tags. As for the question 4 above, it won't affect this bug. So leave it alone for now. Thanks!
Attachment #91459 - Attachment is obsolete: true
This patch addresses the 4 questions in comment #25 with the help of akkana. For question 1, change as I suggested. For question 2 and 3, change FinishHTMLSource() to SetEditMode(PreviousNonSourceDisplayMode) to avoid possible popup of dialog. For question 4, as I tested, we cannot remove all the cmd_quit related stuff from ComposerCommands.js, or the menuitem will be disabled(see 38074). Since doCommand is not used and oncommand="goQuitApplication()" is used, I just commentted the doCommand function. I had tested on Windows and Solaris. Please r= this patch.
Attachment #91618 - Attachment is obsolete: true
Whiteboard: seeking r= and sr= → seeking r= and sr=, Critical for Sun OEM 7.0 release
Comment on attachment 91749 [details] [diff] [review] New patch to address comment #25 r=cmanske Good work!
Attachment #91749 - Flags: review+
Simon Fraser, Could you please sr= my patch? £Éneed to make it into the Trunk and OEM branch in a short time. Thanks!
Comment on attachment 91749 [details] [diff] [review] New patch to address comment #25 sr=sfraser
Attachment #91749 - Flags: superreview+
Whiteboard: seeking r= and sr=, Critical for Sun OEM 7.0 release → seeking a=, Critical for Sun OEM 7.0 release
Comment on attachment 91749 [details] [diff] [review] New patch to address comment #25 a=asa (on behalf of drivers) for checkin to 1.1
Attachment #91749 - Flags: approval+
checked into trunk!
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: seeking a=, Critical for Sun OEM 7.0 release → branchOEM
Whiteboard: branchOEM
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: