125070 - nsWebBrowserPersist adds base tag

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Kathleen :Brade]

After using Composer to publish/save, the html source will show a base tag that
wasn't present before.  nsWebBrowserPersist should remove the base tag if it
wasn't there previously.

Comment 1

[Adam Lock]

Persist is putting a BASE tag in to ensure relative links work when you save to
file.

I am guessing that this is not the correct behaviour when saving remotely.

Comment 2

[Kathleen :Brade]

Actually the issue is that it is inserting it into the actual dom but the other
changes are happening on a copy of the DOM so we are a bit out of synch.  

It seems to me that nsWebBrowserPersist should remove the base node when it is
done (if it inserted it) or change it back to the original value (if it was
present).  I believe it tries to do the latter but not the former.

Comment 3

[Kathleen :Brade]

oops!  clarification:
I don't think it has to do with local vs remote.  I think we see the base tag
get added in every case where the base tag wasn't initially there.

Also, note that the actual output does not show the base tag so there isn't a
problem if the user closes the composer window and reopens it.

Comment 4

[Charles Manske]

I think the output DOES show the base tag that is added by nsIWebBrowserPersist.
We realize that it is a good idea to add the base tag, but for Composer it should
be added before the file is uploaded and have the correct href for the new 
location. What I have noticed in debugging is that the newly-added base tag 
has the href of the source file, not the new destination url. So that is the
real problem. It is not a trivial problem, however, since setting the href url
to an "ftp://username:[e-mail]/filename" would also not be correct 
in the case that the user is actually wants the "http://www.foo.com/filename" 
version. 
Composer is supposed to have code to fix that href in its OnLocationChange
callback, but that isn't working correctly (bug 125085).

Comment 5

[Shmuel Popper]

Composer should not always set a BASE tag.  What if there isn't ftp or http
upload available?  I'm editing local html, and using scp to copy the file. 
Since Composer doesn't know about the remote filename, it will add a base href
to a file on my local computer that won't work for anyone else on the web.  At
least, please make adding a base tag to be configurable.

If you can't make it configurable, at least let me edit it in the HTML source,
and don't add it back in when I save!!

Comment 6

[sujay]

*** Bug 126783 has been marked as a duplicate of this bug. ***

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 127566 has been marked as a duplicate of this bug. ***

Comment 8

[R.K.Aa.]

i filed the same issue in bug 126851, which was resolved dup of bug 122227

Regarding comment 3:
>I think we see the base tag get added in every case where the base tag wasn't
>initially there.

This is wrong. Composer currently *modifies* existing base tag, adding full
local path.

Comment 9

[Kathleen :Brade]

Attachment: add flag to not do base tag manipulations (needed for Composer)

Comment 10

[Adam Lock]

Comment on attachment 71354 [details] [diff] [review]
add flag to not do base tag manipulations (needed for Composer)

r=adamlock

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 71354 [details] [diff] [review]
add flag to not do base tag manipulations (needed for Composer)

>+    persistObj.persistFlags = persistObj.persistFlags | webPersist.PERSIST_FLAGS_NO_BASE_TAG_MODIFICATIONS

Are you missing a ; here? Fix that and sr=sfraser

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 71354 [details] [diff] [review]
add flag to not do base tag manipulations (needed for Composer)

a=roc+moz for 0.9.9

Comment 13

[Kathleen :Brade]

fix checked in

Comment 14

[Bill Law]

The fix checked in to ComposerCommands.js for this bug (which doesn't match the
patch, BTW), caused a serious regression bug 128538.

I've backed out this fix on the branch, and will do likewise on the trunk.

Hopefully it will be a simple matter to rectify things.

Comment 15

[Boris Zbarsky [:bzbarsky]]

The reason this caused the regression is that the persistence object starts life
in an inconsistent state.  Initially we have:

mReplaceExisting(PR_TRUE),
mPersistFlags(kDefaultPersistFlags),

where const PRUint32 kDefaultPersistFlags =
    nsIWebBrowserPersist::PERSIST_FLAGS_NO_CONVERSION;

Now when we execute the js in this patch:

persistObj.persistFlags = persistObj.persistFlags |
                          webPersist.PERSIST_FLAGS_NO_BASE_TAG_MODIFICATIONS

[sic, without semicolon at the end]

WebBrowserPersist does:

mReplaceExisting = (mPersistFlags & PERSIST_FLAGS_REPLACE_EXISTING_FILES) ?
                    PR_TRUE : PR_FALSE;

And the state of the flags becomes synced with mReplaceExisting.

Since this is likely to bite people other than the composer, I suggest that we
make the default flags include PERSIST_FLAGS_REPLACE_EXISTING_FILES, since the
initial state acts as if that flag is set.

Comment 16

[sujay]

*** Bug 128740 has been marked as a duplicate of this bug. ***

Comment 17

[Kathleen :Brade]

I have filed bug 128900 to track Boris' comments.
I will attach a patch (which technically will cover more than the bug described
here; it'll cover some other bugs on my list as well).  This patch will always
set the replace flag for Composer since there really isn't a case when we
wouldn't want to replace the file.

Comment 18

[Kathleen :Brade]

Attachment: patch to fix 125070 and 126672

Composer should always set replace flag.  The only change between what I
checked in Friday and this patch always sets the replace flag and has a comment
explaining that.

Here is a rundown on the whole patch (which covers several bugs):
  add const for web persist component for readability
  add flag variable for "isLocalFile" and set appropriately
  always set crlf for non-local file locations (4.x parity)
  set persist flags (which is what introduced the regression)
  user persist flags for encoding instead of hard-coded values
  add a comment after one line where we are doing bad things (needs its own
bug)

Comment 19

[Adam Lock]

Comment on attachment 72457 [details] [diff] [review]
patch to fix 125070 and 126672

r=adamlock

Comment 20

[Simon Fraser [no longer active]]

Comment on attachment 72457 [details] [diff] [review]
patch to fix 125070 and 126672

sr=sfraser. Please file a bug on the parentDir == root issue.

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 72457 [details] [diff] [review]
patch to fix 125070 and 126672

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 22

[Kathleen :Brade]

Attachment: proposed diff for branch (don't bother with constant cleanup)

This patch is reduced (as requested) for the 0.9.9 branch.  It does not do the
conversion to webbrowser persist constants but simply does the flag setting for
persist.

Comment 23

[Kathleen :Brade]

fix checked into trunk; waiting to resolve after determination of whether some
of this fix will land on the 0.9.9 branch.

Comment 24

[Kathleen :Brade]

bug 129054 filed on the null parent issue

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 72592 [details] [diff] [review]
proposed diff for branch (don't bother with constant cleanup)

a=asa (on behalf of drivers) for checkin to the 0.9.9 branch

Comment 26

[Kathleen :Brade]

fix checked into branch too
This should be verified in both places.

Comment 27

[michael wendell]

Verified on 03-06 trunk and branch builds on Win XP and 03-06 trunk build on 
Mac OSX.

If anyone is still seeing this problem, feel free to reopen this bug.

Comment 28

[R.K.Aa.]

*** Bug 131600 has been marked as a duplicate of this bug. ***