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

VERIFIED FIXED in mozilla0.9.6

Status

SeaMonkey
Composer
--
major
VERIFIED FIXED
17 years ago
13 years ago

People

(Reporter: TucsonTester2, Assigned: Charles Manske)

Tracking

Trunk
mozilla0.9.6

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: EDITORBASE,)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

17 years ago
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

17 years ago
we should fix this soon; not specific to JS handlers (adding an id also fails)
-->cmanske
Assignee: brade → cmanske
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Trouble entering javascript on first try, works the second time. → Adding any advanced edit settings on link creation fails
Target Milestone: --- → mozilla0.9.5
(Assignee)

Comment 2

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

Updated

16 years ago
Component: Editor: Core → Editor: Composer

Comment 3

16 years ago
spam composer change
(Reporter)

Comment 4

16 years ago
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.
(Assignee)

Comment 5

16 years ago
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.
(Reporter)

Comment 6

16 years ago
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.
(Assignee)

Comment 7

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

Comment 8

16 years ago
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. 
(Assignee)

Comment 9

16 years ago
I still can't reproduce this!
Whiteboard: EDITORBASE

Comment 10

16 years ago
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.
(Reporter)

Comment 11

16 years ago
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.
(Assignee)

Comment 12

16 years ago
Ok, I finally get it (sorry!), but it's a complete mystery why it's not working!
Status: NEW → ASSIGNED
(Assignee)

Comment 13

16 years ago
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?
(Assignee)

Comment 14

16 years ago
Created attachment 51123 [details] [diff] [review]
Fix: Set all attributes on the anchor element
(Assignee)

Comment 15

16 years ago
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.
Severity: normal → major
Keywords: patch, review
Whiteboard: EDITORBASE → EDITORBASE FIX IN HAND need r=, sr=

Comment 16

16 years ago
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)
Attachment #51123 - Flags: needs-work+
(Assignee)

Updated

16 years ago
Attachment #51123 - Attachment is obsolete: true
(Assignee)

Comment 17

16 years ago
Created attachment 51296 [details] [diff] [review]
updated patch to address reviewer's comments
(Assignee)

Comment 18

16 years ago
Updating summary
Summary: Adding any advanced edit settings on link creation fails → Attributes other than "href" are ignored when creating a link around existing selection

Comment 19

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

Updated

16 years ago
Attachment #51296 - Attachment is obsolete: true
(Assignee)

Comment 20

16 years ago
Created attachment 51520 [details] [diff] [review]
Updated patch: Check return value from SetInlineProperty()

Comment 21

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

r=brade
Attachment #51520 - Flags: review+
(Assignee)

Updated

16 years ago
Whiteboard: EDITORBASE FIX IN HAND need r=, sr= → EDITORBASE FIX IN HAND need sr=
(Assignee)

Comment 22

16 years ago
Taking this off the "fixed" radar. SetInlineProperty is buggy and we need to 
address those issues first.
Whiteboard: EDITORBASE FIX IN HAND need sr= → EDITORBASE
Target Milestone: mozilla0.9.5 → mozilla0.9.6

Comment 23

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

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

Updated

16 years ago
Attachment #51520 - Attachment is obsolete: true
Attachment #51520 - Flags: review+
(Assignee)

Comment 24

16 years ago
Created attachment 54504 [details] [diff] [review]
Updated patch.
(Assignee)

Updated

16 years ago
Whiteboard: EDITORBASE → EDITORBASE, FIX IN HAND, need r=,sr=

Comment 25

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

sr=kin@netscape.com

Are the bugs with SetInlinePropery that you mentioned above, still a problem?
Attachment #54504 - Flags: superreview+

Updated

16 years ago
Whiteboard: EDITORBASE, FIX IN HAND, need r=,sr= → EDITORBASE, FIX IN HAND, need r=

Comment 26

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

r=jfrancis
Attachment #54504 - Flags: review+

Updated

16 years ago
Whiteboard: EDITORBASE, FIX IN HAND, need r= → EDITORBASE, FIX IN HAND
(Assignee)

Comment 27

16 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Keywords: patch, review
Resolution: --- → FIXED
Whiteboard: EDITORBASE, FIX IN HAND → EDITORBASE,

Comment 28

16 years ago
Verified.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.