96477 - Image properties window needs an option to remove the link border

Status: VERIFIED FIXED

(SeaMonkey :: Composer, enhancement)

Description

[TucsonTester2]

From Bugzilla Helper:
User-Agent: Mozilla/4.77 [en] (Win98; U)
BuildID:    20010816

I added a link to my image and there was no option to easily click an option 
like "do not display link border around image" which would look better if my 
image was a button for example.  I'm just bringing this up because there are 
people out there that don't know much html and may not know that they can just 
make the border value 0 to make it not display.

Reproducible: Always
Steps to Reproduce:
1.insert an image
2.highlight the image and click the link button in the toolbar
3.insert a link and click ok

Actual Results:  A link border will be displayed around the image.  This can be 
a not desirable option, especially for images that are buttons.  So, in order to 
change this there should be an option in the Image Properties that allows users 
to check or uncheck an option like "Do not display link border", instead of 
having to set the border value to 0.  This is something that would be better for 
novice users.

Expected Results:  The option to turn the link border on or off would be a nice 
addition.

Comment 1

[Kathleen :Brade]

-->cmanske

Comment 2

[Charles Manske]

I'm not sure if a separate control is necessary to do what is already there:
set the border. I can see adding some text under the "Border" input field that
says:
"Use 0 to turn off
borders in links"
How does that sound?

Comment 3

[Kathleen :Brade]

*if* we choose to solve this problem with extra text (in an already crowded 
dialog, I'm not convinced it's a good idea), then we should only show that text 
if the image is a link.  Perhaps the control belongs in the link dialog instead 
(checkbox for Don't show link border on this image)?

Comment 4

[TucsonTester2]

That could work too, I believe the easiest for a user would be an option box in
either the Image or the link properties menu.  I like the image properties
window the most because the border="0" is an image property, and the image
properties menu comes up when double clicking the linked image.  The link
properties is a good solution too, because it can be set up at link creation.  

Another solution, if possible, would be to default linked or unlinked image
borders to 0.  It's not often that a person uses a link border around an image. 

But if there is any way of putting one of the options it would be very appreciated.

Comment 5

[Syd Logan]

spam composer change

Comment 6

[[e-mail]]

Attachment: Add Link tab to image properties

Now you can link an image when inserting it, which might remind you to turn the
border off...

Comment 7

[Charles Manske]

Attachment: Updated patch

Good work! I really like integrating the Named Anchors and headings as a popup
menulist. I improved what you did by doing the same for the main Link
Properties
dialog and moving common code into EdDialogOverlay.xul and EdDialogCommon.js.
I also added a checkbox in the "Link" panel:
[ ] Show border around linked image
that simply manipulates the "border" attribute in the "Appearance" tab.
After all, that was the original purpose of this bug!

Comment 8

[Charles Manske]

Adding Neil to CC list.
Note that this greatly simplifies the Link Properties dialog, completely removing
the need for a "More / Fewer" section.

Comment 9

[[e-mail]]

Attachment: A few extra fixes

Removed flex from the menulist, becuase it's in a vbox
Moved chooseLinkFile into EdDialogCommon.js
Merged 2nd checkbox and button into cmanske's new vbox
Put the remove link message back but fixed it not for new links.

Comment 10

[Charles Manske]

Attachment: Updated patch

Incorprated Neil's suggestions from patch 67261 as well as some more of my
own, except for the "Clear text to remove existing link from page" message
code.
Upon further reflection and discussion, we decided that just adds clutter to
the
dialog and isn't really necessary.

Comment 11

[[e-mail]]

Attachment: More issues

Removed some old code from an old patch rather than commenting it out.
Removed some old elements from EdDialogOverlay.xul, again from an old patch.
Commented out the remove link message in case it gets reinstated.
Called ChangeLinkLocation() in EdImageProps.js instead of duplicating code.
Moved link border checkbox code in case of advanced editing.
Fixed EdLinkProps.js to relativize the right checkbox.
Removed chooseLinkFile() and GetExistingHeadingIndex() from EdLinkProps.js
Removed unnecessary flex.

Comment 12

[Charles Manske]

Comment on attachment 67898 [details] [diff] [review]
More issues

r=cmanske
Used all of Neil's latests suggestions except keeping the "(Clear text to
remove existing link from page.)" text

Comment 13

[kinmoz]

Comment on attachment 67898 [details] [diff] [review]
More issues

sr=[e-mail]

Just remove the commented code that was added.

Also, I know you just moved onAccept() between files, I was just curious why
the code wraps only some editorShell calls with try/catch and not all the
others?

Comment 14

[Charles Manske]

checked in

Comment 15

[[e-mail]]

> Also, I know you just moved onAccept() between files, I was just curious why
> the code wraps only some editorShell calls with try/catch and not all the
> others?

Do you want to file a separate bug for that or should I just attach a fix here?

Comment 16

[[e-mail]]

Attachment: diff -uw so you can see what's going on

Comment 17

[[e-mail]]

Attachment: Move more of onAccept into try block

Comment 18

[Kathleen :Brade]

reopen bug

Comment 19

[Kathleen :Brade]

the above patches need work; the endBatchChanges call in the "finally" block
should probably have its own try/catch or something along those lines

Comment 20

[[e-mail]]

Attachment: Alternative using catch instead of finally

This version uses try/catch instead of try/finally so I have to dump any
exception to the console because the JS console won't see it.

brade: Begin/EndBatchChanges are always outside try blocks.

Comment 21

[Kathleen :Brade]

I think editorshell.BeginBatchChanges and editorshell.EndBatchChanges should
both be in a try/catch block since either of them could throw an error. 
However, you *do* need to ensure that they are matched up.

Comment 22

[Charles Manske]

We use the BeginBatch / EndBatch calls in many places and since they do need to
match, I don't see how we can put them in try/catch. Chances of failure are 
*extremely* low.

Comment 23

[Charles Manske]

Comment on attachment 68879 [details] [diff] [review]
Alternative using catch instead of finally

r=cmanske

Comment 24

[[e-mail]]

If BeginBatchChanges() fails, you could in theory try to do the changes anyway
but I don't see the point. If EndBatchChanges() fails then you are stuffed anyway.

Comment 25

[Charles Manske]

Comment on attachment 68879 [details] [diff] [review]
Alternative using catch instead of finally

r=kin

Comment 26

[Charles Manske]

checked in

Comment 27

[michael wendell]

Verified on Win 2k using trunk build 02-14.