image insert doesn't function

VERIFIED FIXED in mozilla0.8

Status

()

Core
Editor
--
critical
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: tracy, Assigned: Charles Manske)

Tracking

({smoketest})

Trunk
mozilla0.8
All
Windows 98
smoketest
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

18 years ago
seen on commercial builds:

windows 2001-01-10-06-mtrunk
linux 2001-01-10-06-mtrunk

-open composer
-click in composer window
-click on image icon or use Insert | image
-click choose file to find an image to insert
-double click file name or  highlight and click open

expected results: file appears selected and ready to be inserted and okay button 
becomes activated

tested results:  the field for the selected image remains blank the okay button 
remains inactive
(Reporter)

Updated

18 years ago
Keywords: smoketest

Comment 1

18 years ago
sfraser made some changes in this area yesterday, cc'ing

Comment 2

18 years ago
Mine
Assignee: beppe → sfraser

Comment 3

18 years ago
reducing severity, i believe sfraser is on the case.
Severity: blocker → critical

Comment 4

18 years ago
This fails in smoketest but we won't hold tree closed since sfraser is on it. 
(Assignee)

Comment 5

18 years ago
Created attachment 22365 [details] [diff] [review]
Fixes bug.
(Assignee)

Comment 6

18 years ago
This is very easy to fix.
Kin and Simon: please review and I will check this in ASAP.
Assignee: sfraser → cmanske
Hardware: PC → All
Target Milestone: --- → mozilla0.8
(Assignee)

Comment 7

18 years ago
Note: The second 'hunk' in the diff is for a different bug. I won't check that
change in.
Status: NEW → ASSIGNED
(Assignee)

Comment 8

18 years ago
Created attachment 22372 [details] [diff] [review]
The simplest fix -- we don't need to get nsIFile object at all!
(Assignee)

Comment 9

18 years ago
Curious note: The fp.file.URL returns a conversion from local file sytax to URL
that has this syntax for a typical Windows path (e.g., C:\dir\filename.html):
file://C|/dir/filename.html
We could also use the nsILocalFile member variable "fileURL" in nsIFile to get
the spec:  fp.fileURL.spec
but this returns a different syntax:
file:///C:/dir/filename.html
Both seem to work as a URL in image dialog, but isn't the latter syntax
incorrect?
(Assignee)

Comment 10

18 years ago
Oops, in above comment, I meant to say:
fp.fileURL is the nsIFileURL variable, and fp.file is the nsILocalFile variable.
Should we stop using nsIFileURL?

Comment 11

18 years ago
Charley, your changes look ok to me, but shouldn't we also remove the useless fs
references?

Patch below.

Comment 12

18 years ago
Doh, Charley beat me to it ... though I also removed the GetScriptFileSpec()
function since no one will be using it. I'm not even sure why it was there to
begin with.

Comment 13

18 years ago
Created attachment 22376 [details] [diff] [review]
Diff that also removes GetScriptFileSpec().
(Assignee)

Comment 14

18 years ago
I've been informed that the new implementation for nsIFile::GetURL() isn't
checked in yet for all platforms (see bug 56354), so we should use
return fp.fileURL.spec;
instead of
return fp.file.URL;
for this bug.
After 56354 is checked in, both will return the identical URI string.
Is there a new patch to review?  Or should I mentally put a .spec after the
return fp.file.URL ?

/be
(Assignee)

Comment 16

18 years ago
Created attachment 22398 [details] [diff] [review]
Final version of fix using "fp.fileURL.spec"

Comment 17

18 years ago
sr=kin@netscape.com

Charley, I think we should also remove the GetScriptFileSpec() function (in
EdDialogCommon.js) since it is no longer referenced.
(Assignee)

Comment 18

18 years ago
Created attachment 22465 [details] [diff] [review]
Yet another "final" version of diff.
(Assignee)

Comment 19

18 years ago
checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 20

18 years ago
verified in 1/16 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.