Closed Bug 116867 Opened 23 years ago Closed 21 years ago

background image left locally

Categories

(MailNews Core :: Composition, defect)

Product:
MailNews Core
Component:
Composition
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: peterthaus, Assigned: mscott)

References

Details

Attachments

(1 file, 1 obsolete file)

complete patch that fixes editor, mail compose and libmime
11.41 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review 
Selecting an image as background in a Mail, this is shown as a background in the
editor. But this mail is only kept locally at its original place on the
harddisk. It is not sent with the mail and also the link in the mail keeps local.

This is an excerpt from a HTML-mail source-code:

<BODY =
background=3Dfile:///P:/Internet/Pictures/Background/Sky/CLOUDS2.GIF=20
bgColor=3D#ffffff>
<DIV><FONT face=3DArial>Ciao&nbsp;Peter,</FONT></DIV>
composition
Assignee: sspitzer → ducarroz
Status: UNCONFIRMED → NEW
Component: Mail Window Front End → Composition
Ever confirmed: true
QA Contact: esther → sheelar
Status: NEW → ASSIGNED
QA Contact: sheelar → esther
This issue was originally implemented in Bug #127077.

I believe it is broken because the following line in nsHTMLEditor.cpp
never returns true even when we do have a background image on the body:


if (NS_SUCCEEDED(element->HasAttribute(NS_LITERAL_STRING("background"),
&hasBackground)))
Assignee: ducarroz → scott
Status: ASSIGNED → NEW
This code in nsHtmlEditor really needs to check the computed style for the body
element to see if we have a style="background-image url('some url')" attribute
installed on it before checking for a background='some url' attribute.

If I knew how to do that then we could fix this.
Severity: minor → normal
Status: NEW → ASSIGNED
this fixes the problem in editor where we now return the body element if the
background image was set via computed style.

However we need code in nsMsgSend which can read out the url when set as a
style attribute too.
Making this work again took us all over the map.

1) nsHTMLEditor::GetEmbeddedObjects needed to stop looking for the old
background property, instead looking for a background-image inline style
attribute

2) Add a bunch of code to mail compose to stop trying to replace the old
background attribute with a cid url referring to the message part. Instead, dig
down into the compute style rules looking for the background-image: url(...)
attribute and replace it with our cid notation

3) Finally, libmime was not trying to replace cid urls with the actual part url
during message display. Libmime expects all cid urls to be of the form
someAttribute="cid:..." instead of url(cid:...). Add a hack in here to handle
the case of an inline style rule for a image url.
Attachment #129644 - Attachment is obsolete: true
*** Bug 214849 has been marked as a duplicate of this bug. ***
Comment on attachment 129765 [details] [diff] [review]
complete patch that fixes editor, mail compose and libmime

David, can you review this for me? some time ago, composer stopped setting
background="some url" on the body element for background images. Instead, we
now set the background url as an inline style attribute:

style="background-image: url('some url'); texcolor = blue"

We have code in mail compose which looks for background="some file url" and
replaces it with an inline part url (cid:...) before sending the message.

Unfortunately it is a lot harder to dig down into the computed style to extract
this attribute and then replace it. That's what the code in nsMsgCompUtils.cpp
does

Finally in libmime, we have code which drugs through the message looking for
cid: urls of the form src="cid:" and replacing them with mailbox / imap urls
for the actual part. I had to tweak that nasty code to make it also work for
url(cid:);
Attachment #129765 - Flags: superreview?(bienvenu)
Comment on attachment 129765 [details] [diff] [review]
complete patch that fixes editor, mail compose and libmime

ugh. The code that's changed pretty much only deals with background images,
right? So, it's not going to affect other more common stuff, right?
Attachment #129765 - Flags: superreview?(bienvenu) → superreview+
*** Bug 216149 has been marked as a duplicate of this bug. ***
right the new code only gets executed in the case of background image handling.
Flags: blocking1.5b?
Blocks: 170504
Comment on attachment 129765 [details] [diff] [review]
complete patch that fixes editor, mail compose and libmime

a few comments / questions:

1)

can you get someone on editor (glazman or jfrancis, who are still active) to
review the editor/libeditor/html/nsHTMLEditor.cpp change?

Index: editor/libeditor/html/nsHTMLEditor.cpp
===================================================================
RCS file: /cvsroot/mozilla/editor/libeditor/html/nsHTMLEditor.cpp,v
retrieving revision 1.489
diff -u -w -r1.489 nsHTMLEditor.cpp
--- editor/libeditor/html/nsHTMLEditor.cpp	28 Jul 2003 21:35:21 -0000     
1.489
+++ editor/libeditor/html/nsHTMLEditor.cpp	13 Aug 2003 22:34:33 -0000
@@ -3992,9 +3992,10 @@
	   nsCOMPtr<nsIDOMElement> element = do_QueryInterface(node);
	   if (element)
	   {
-	     PRBool hasBackground = PR_FALSE;
-	     if
(NS_SUCCEEDED(element->HasAttribute(NS_LITERAL_STRING("background"),
&hasBackground)))
-	       if (hasBackground)
+	     nsAutoString bgImageStr;
+
+	     mHTMLCSSUtils->GetComputedProperty(element,
nsEditProperty::cssBackgroundImage, bgImageStr);
+	     if (!bgImageStr.Equals(NS_LITERAL_STRING("none"))) 
		 (*aNodeList)->AppendElement(node);

what about "NONE", do you have to worry about caps here?

2)

+nsresult GetBackgroundImageUrl(nsIDOMElement * aElement, const nsAString&
aPropertyName, nsAString& aUrl)
+{
+  NS_ENSURE_ARG(aElement);
+  
+  nsresult rv = NS_OK;

rv declared, but never used.  maybe this helper function should just return
NS_OK?


+  nsCOMPtr<nsIDOMCSSPrimitiveValue> primCSSValue;
+
+  nsCOMPtr<nsIDOMDocument> domDocument; 
+  aElement->GetOwnerDocument(getter_AddRefs(domDocument));
+  nsCOMPtr<nsIDocument> document = do_QueryInterface(domDocument);
+  if (document)
+  {	
+    nsCOMPtr<nsIScriptGlobalObject> global;
+    document->GetScriptGlobalObject(getter_AddRefs(global));
+    nsCOMPtr<nsIDOMViewCSS> viewCSS(do_QueryInterface(global));
+    if (viewCSS)
+    {
+      nsCOMPtr<nsIDOMCSSStyleDeclaration> cssDecl;
+      nsAutoString empty;
+      viewCSS->GetComputedStyle(aElement, empty, getter_AddRefs(cssDecl));
+      if (cssDecl)
+      {
+	 nsAutoString value;
+	 nsCOMPtr<nsIDOMCSSValue> cssValue; 
+	 cssDecl->GetPropertyCSSValue(aPropertyName, getter_AddRefs(cssValue)); 
+	 primCSSValue = do_QueryInterface(cssValue);
+
+	 if (primCSSValue)
+	   primCSSValue->GetStringValue(aUrl);	  
+
+      }
+    }
+  }
+
+  return rv;
+}

3)

what about "BACKGROUND-IMAGE"?	does your helper method need to worry about
caps, or does GetPropertyCSSValue() work with lower case, and internally,
everything is normalized?

+	 nsAutoString backgroundUrl;
+	 if (NS_SUCCEEDED(GetBackgroundImageUrl(domElement,
NS_LITERAL_STRING("background-image"), backgroundUrl)) &&
!backgroundUrl.IsEmpty())
+	   *_retval = nsIMsgCompConvertible::No; // There is a background image
	 else if
(NS_SUCCEEDED(domElement->HasAttribute(NS_LITERAL_STRING("text"),
&hasAttribute)) &&
		  hasAttribute &&
		 
NS_SUCCEEDED(domElement->GetAttribute(NS_LITERAL_STRING("text"), color)) &&
4)

+nsresult nsMsgComposeAndSend::ChangeBackgroundImageUrl(nsIDOMElement *
aBodyElement, const nsAString& aCurrentUrl, const nsAString& aNewUrl)
+{
+  nsString styleValue;

Why a heap based string here, for styleValue, and not a stack baced one?

+  aBodyElement->GetAttribute(NS_ConvertASCIItoUCS2("style"), styleValue);

shouldn't that be NS_LITERAL_STRING("style")?

+
+  PRInt32 startOfUrl = styleValue.Find(PromiseFlatString(aCurrentUrl).get());
+  if (startOfUrl != kNotFound)
+  {
+    // cut out the current url
+    nsAutoString before; 
+    styleValue.Left(before, startOfUrl);
+    nsAutoString after;  
+    styleValue.Mid(after, startOfUrl + aCurrentUrl.Length(),
styleValue.Length() - (startOfUrl + aCurrentUrl.Length()) );
+    before.Append(aNewUrl);
+    before.Append(after);
+
+    styleValue = before;
+  }
+
+  // styleValue.ReplaceSubstring(aCurrentUrl, aNewUrl);
+
+  aBodyElement->SetAttribute(NS_ConvertASCIItoUCS2("style"), styleValue);

here too, shouldn't that be NS_LITERAL_STRING("style")?
fixed with seth's comments.

btw the attribute names are normalized in the css code so no need to check
different cases.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
> 1) can you get someone on editor (glazman or jfrancis, who are still active) to
>    review the editor/libeditor/html/nsHTMLEditor.cpp change?

Sorry Scott, I have my module owner hat on here: you SHOULD get an editor peer's
review for *ALL* changes in mozilla/editor/libeditor .
Flags: blocking1.5b?
ugh! I've been trying to cleanup this code and more junk is getting in! ugh!

btw, isn't the real problem that css attributes are appearing?  That is bug
216753 (I assume).
brade, I'm not sure I understood this comment:

>> ugh! I've been trying to cleanup this code and more junk is getting in! ugh!

This was a mailnews fix in mail compose to handle the fact that editor was no
longer setting the background attribute on the body for us but was instead
setting inline style attributes for the body. How is this more junk getting in?
Are you saying editor is not supposed to be using inline style so I didn't have
to convert mail compose to handle it?

I presume that this is a basic philosophical difference that I see here.
Let me assure you that there are thousands..yes thousands of Moz and ex-netscape
users that have not been active in Html/CSS mail/news because of the attitude
that I see here. We don't want less HTML/CSS editing capability in Moz, we need
more. From what I see since 1.4 the trend is certainly towards plain text mail.
You don't have to 'like' the fact the some folks use mail/news in this way,
but I don't think Mozilla can afford to reject the HTML mail user because of some
pre-determined idea of what the media should be.
Moz will loose a lot of folks very quickly if the trend continues.
Joe
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: