90229 - Don't disable "OK" button in Image dialog when image doesn't have proper extension.

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Charles Manske]

User testing has discovered that users can be confused by our disabling of 
OK button without explanation if file extension is not ".gif", ".jpg", ".jpeg",
or ".png". This is especially important when user has saved an image to a file
but did not supply an extension.
We may want to leverage the fact that in the image dialog, we always try to
load the user-entered URL, so we do know if the image loads correctly.
If it does not, them maybe we can put up a warning message that maybe this is
not a valid image instead of our current button disabling strategy.

Comment 1

[Kathleen :Brade]

Do we want to enable it when there is no extension?  What if user has file with 
.txt extension or .bmp (which we don't support right now)?  We have lots of 
little things to test here...  :-/

Comment 2

[Kathleen :Brade]

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

Comment 3

[Charles Manske]

I'm not sure what is worse, not letting them select a file that is a valid image,
but maybe has a bad filename, or letting them use a non-image filename. For 
latter, the worst that happens is they get a broken image in the page.
We should definitely not consider the image invalid if we know that it loaded 
correctly in the preview window, even if the extension is bad.

Comment 4

[Kathleen :Brade]

To be clear, there is nothing wrong with naming my image file:  myFamilyPicture
The problem isn't really at the user side (although we may want to think it is).

If we really wanted to help users avoid bad urls, we'd provide an image link
checker or a button in this dialog to help them confirm they have the correct
url entered (but that'd be another bug...).
My current leaning on this bug is to remove the check for extensions.
Sujay and/or Shrir--if we do this, you'll need to verify that we don't mishandle
various filetypes (*.txt, *.doc, *.bmp, *.html, etc.)

Comment 5

[jean-noel]

As I said in a comment in bug 90572 , it is mainly a problem when accessing say
a servlet where there is no extension. I like the idea of the button to check if
the image is a valid image. Or there can be a check on the extension that would
display something like "valid image" next to the "check button".

Comment 6

[Charles Manske]

We don't need a "check valid" button, we automatically try to load any image
referenced in the Image Dialog. If the preview window contains a thumbnail of
the image, then it was loaded correctly and thus is valid.
Besides simply showing a preview image, this is to do the following:
If you had an image that was "broken" (e.g., image wasn't in correct 
location when <img> tag made), but you now fixed it, or you edited the image
in an image editor, then launching the image dialog for that image will force 
reloading it (and we  ignore the image cache.)

Comment 7

[jean-noel]

This seems a good solution to me. Just to have an idea, when do you think it can
be fix ? (I don't want to push things, just to know). Could you tell me when it
gets in the nightly builds ?
Thanks

Comment 8

[Kathleen :Brade]

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

Comment 9

[Syd Logan]

moving out to 0.9.6

Comment 10

[Syd Logan]

spam composer change

Comment 11

[Charles Manske]

load balancing

Comment 12

[Charles Manske]

Attachment: Don't fail IsValidImage() unless filename is empty

The change to EdImageProps.js is needed because now that we accept anything for

an image URL, we don't want to try to load the preview image for every
keystroke,
so do that only after a 0.8sec idle delay when user types. (ChangeImageSrc() is
the  oninput event handler for the image SRC textbox.)

Comment 13

[Charles Manske]

This is the simplest fix for the bug. We could rip out all calls to 
"IsValidImage()", but we might come up another strategy in the future.
I decided not to rely on the preview image loading to "validate" since that
wouldn't work when image is not available like when user is offline.

Comment 14

[Charles Manske]

Attachment: The "aggessive" fix: Completely remove "IsValidImage()" and all code that used it.

Comment 15

[Kathleen :Brade]

Are you introducing any JS warnings?  In the 2nd patch, it doesn't look like
canEnableOk is declared (for example).

Sujay--when testing this fix, be sure to verify the image map button(s) and the
advanced edit button (in addition to the OK button).

Comment 16

[Charles Manske]

Comment on attachment 59419 [details] [diff] [review]
The "aggessive" fix: Completely remove "IsValidImage()" and all code that used it.

r=syd

Comment 17

[Kathleen :Brade]

I thought there was going to be a new patch to address the warning issue I
mentioned above... I don't understand where doOverallEnabling() initializes
canEnableOk.  

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 59419 [details] [diff] [review]
The "aggessive" fix: Completely remove "IsValidImage()" and all code that used it.

removing has-review flag since reviewers still have unanswered questions

Comment 19

[Charles Manske]

Attachment: Updated patch

Sorry! Forgot to attach updated fix.

Comment 20

[Kathleen :Brade]

r=brade if you fix up the spacing issues I mentioned on irc and remove the "if"
we discussed on irc

Comment 21

[Charles Manske]

Attachment: Updated patch with brade's suggestions

Comment 22

[Kathleen :Brade]

Comment on attachment 59735 [details] [diff] [review]
Updated patch with brade's suggestions

r=brade

Comment 23

[Joe Hewitt (gone)]

Comment on attachment 59735 [details] [diff] [review]
Updated patch with brade's suggestions

sr=hewitt

Comment 24

[Charles Manske]

checked in

Comment 25

[michael wendell]

Verified on build 2001113003.