97455 - Attributes other than "href" are ignored when creating a link around existing selection

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[TucsonTester2]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.3+) Gecko/20010821
Netscape6/6.1b1
BuildID:    20010821

While I was adding javascript to my page so that it would show an alert on mouse
over it did not work the first time.  Instead the javascript was added the
second time I tried.  I found out the problem is if the object your assigning a
javascript event to (image, text, etc.) then it needs to have a link on it
before you can add the javascript.  You can not enter the javascript in the
advanced edit after putting a  link into the url box in the link properties window.

Reproducible: Always
Steps to Reproduce:
1.Open Composer
2.Type in Hello
3.Highlight the text and click on the link button in the toolbar
4.Use # for the link location
5.Click on advanced edit and then choose the javascript events tab
6.Choose onmouseover in the attributes drop down menu and put alert('Hello') in
the value box
7. Click ok and click ok again
8. Click on the HTML source tab

Actual Results:  From the HTML source view you can see the only thing added was
the link.  If you go into the advanced edit again and enter in the javascript
the event will finally appear.

Expected Results:  I would expect that I would not have to enter the link and
click ok and then highlight the link again and then go to the advanced edit to
enter the javascript.  I should be able to go into the link properties the one
time and be able to enter all of my information and not have it disappear.

Comment 1

[Kathleen :Brade]

we should fix this soon; not specific to JS handlers (adding an id also fails)
-->cmanske

Comment 2

[Charles Manske]

I can't reproduce this in recent builds. Please test again.

Comment 3

[Syd Logan]

spam composer change

Comment 4

[TucsonTester2]

I'm still seeing this bug.  You must make sure you click on advanced edit after
putting in the # for the link.  If you click ok after adding the # and then go
back to link properties and the advanced edit it won't work.  This bug has been
reproduced on the 20010910 build on Windows 9x pc's and on a Mac.

Comment 5

[Charles Manske]

The code for setting attributes via advanced edit dialog is the same for all 
dialogs: Image, H.Line, Table, etc. Do you see the problem for any of those 
elements? Is it only specific attributes or JS events that aren't retained? 
Latter definitely shouldn't happen.
Note that I'm using WinNT, but I can't see how platform should affect this.

Comment 6

[TucsonTester2]

Actually none of the advanced settings are kept at all.  Nothing from the HTML 
Attributes, Inline styles, or the javascript events properties are retained  
once values are addedd during link creation.

Comment 7

[Charles Manske]

Still can't reproduce this.
Do you see the same problem in any other dialog (e.g., Image, HLink, Table, etc.)

Comment 8

[michael wendell]

I am able to reproduce this on build 2001091703 both on Win2k and Mac OS 9.1. 
This problem does not appear to happen with Image, table, HLine, or Anchor.  One
thing to note, is that you have to click on Advanced Edit when initially setting
up the link, otherwise, it works fine. 

Comment 9

[Charles Manske]

I still can't reproduce this!

Comment 10

[Kathleen :Brade]

I see this following the steps provided initially.
Make sure that the link isn't created before you go into the advanced edit 
dialog.  All you need to add in the advanced edit dialog is id="foo"

Do we have similar problems with other elements (images, anchors)?  I think we 
fixed a similar bug with table insertion already.

Comment 11

[TucsonTester2]

This can happen when setting any kind of attribute.  If you use any tab in 
the advanced editor to add attributes they won't be applied on creation.

I tried this with anchors, images, and tables and they all work as they 
should.  The H.Line just gets put in without giving a prompt to set it up so 
there is not a problem there.  

Also, if you make a link from nothing (i.e. not highlighting anything first) , 
then it will work fine.  If you click the link button and it asks for you to type in 
the "Link Text"  then to type in the "Link Location" the advanced edit 
settings will be applied.

Comment 12

[Charles Manske]

Ok, I finally get it (sorry!), but it's a complete mystery why it's not working!

Comment 13

[Charles Manske]

Ok, I'm not going crazy. All attributes except HREF are ignored by
nsHTMLEditor::InsertLinkAroundSelection()!
This comment is in that method:
        //TODO: Enumerate through other properties of the anchor tag
        // and set those as well. 
        // Optimization: Modify SetTextProperty to set all attributes at once?

Comment 14

[Charles Manske]

Attachment: Fix: Set all attributes on the anchor element

Comment 15

[Charles Manske]

I thought I could optimize by finding the new anchor node created and use
nsEditor::CloneAttributes, but if the selection spans across different elements,
SetInlineProperty() will create > 1 anchor element! So it's better to just let
SetInlineProperty() find existing anchors after the first attribute is set.

Comment 16

[Kathleen :Brade]

Comment on attachment 51123 [details] [diff] [review]
Fix: Set all attributes on the anchor element

* the nsAutoString declarations inside the loop should be moved out (name, value)
* fix this line to have a space after "if":  if(attrNode)
* you left in this comment:
// Optimization: Modify SetTextProperty to set all attributes at once?
but it doesn't make sense to me since I don't see "SetTextProperty" anywhere

Unfortunately this fix is going to massively collide with the work I did 
yesterday on a different bug in this same method.  I'll have to back out what I 
did and wait for your fix and then start my stuff over.  :-/
Who would have thought?! (given our very different bugs)

Comment 17

[Charles Manske]

Attachment: updated patch to address reviewer's comments

Comment 18

[Charles Manske]

Updating summary

Comment 19

[Kathleen :Brade]

given all of the possible early return (error) situations in SetInlineProperty, 
shouldn't we check a return value for that call?

Comment 20

[Charles Manske]

Attachment: Updated patch: Check return value from SetInlineProperty()

Comment 21

[Kathleen :Brade]

Comment on attachment 51520 [details] [diff] [review]
Updated patch: Check return value from SetInlineProperty()

r=brade

Comment 22

[Charles Manske]

Taking this off the "fixed" radar. SetInlineProperty is buggy and we need to 
address those issues first.

Comment 23

[kinmoz]

I think attrMap->Item() addrefs the node it returns so there might be a leak here.

+          nsIDOMNode* attrNode;
+          res = attrMap->Item(i, &attrNode);

Comment 24

[Charles Manske]

Attachment: Updated patch.

Comment 25

[kinmoz]

Comment on attachment 54504 [details] [diff] [review]
Updated patch.

sr=[e-mail]

Are the bugs with SetInlinePropery that you mentioned above, still a problem?

Comment 26

[Joe Francis]

Comment on attachment 54504 [details] [diff] [review]
Updated patch.

r=jfrancis

Comment 27

[Charles Manske]

checked in

Comment 28

[michael wendell]

Verified.