Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#23 closed Bug (fixed)

Show the content of anchors

Reported by: Alfonso Owned by: Alfonso
Priority: Normal Milestone: FCKeditor 2.4
Component: General Version:
Keywords: Cc:

Description

I plan to incorporate the patch as described here (upgrading everything to latest version, and reviewing that everything is fine) https://sourceforge.net/tracker/index.php?func=detail&aid=1496115&group_id=75348&atid=543655

this fixes several bugs and feature requests about this problem. First I'll do the changes locally and then upload a patch here so it can be reviewed before it's applied to SVN.

Change History (13)

comment:1 Changed 13 years ago by Frederico Caldeira Knabben

Great! It is a nice feature.

I was wondering if you instead would like to open a SVN branch for this development. In this way we can investigate how to work with branches, and we don't need to be worried about messing things, or creating patches to see if it works.

I'll be updating the SVN page with more info about how we could proceed with branches. Any idea is welcome and appreciated.

comment:2 Changed 13 years ago by Alfonso

Status: newassigned

Ok, I've applied the changes and I think that they have ended in a new branch. I've added also a missing part in the original code, when a new anchor is created preserve the selected content.

I've seen only two problems: in the toolbar the link and unlink buttons are active now when selecting an anchor (I don't think that it's very important but it's a bug from my point of view), and Opera doesn't seems capable of selecting the empty anchors so they can't be modified. I could make the code act like IE in that case and create a fake image.

comment:3 in reply to:  2 Changed 13 years ago by Frederico Caldeira Knabben

I've switched to the branch to take a look at it and it is really cool. I've loved the effect and it seams to work pretty well. Congratulations Alfonso.

I've seen only two problems: in the toolbar the link and unlink buttons are active now when selecting an anchor (I don't think that it's very important but it's a bug from my point of view)

Yes, it seams to be a relevant bug. I believe the link button should be disabled. We could also think about leaving the unlink active, so one can remove the anchor with one click. In both cases new functions must be introduced to check the state (today I think we are using getCommandState).

and Opera doesn't seems capable of selecting the empty anchors so they can't be modified. I could make the code act like IE in that case and create a fake image.

The fake image would be ok (it works well with Opera). I think you could also apply the same style to the fake image, to make it look like and anchor with contents. It looks pretty cool.

comment:4 Changed 13 years ago by Alfonso

I've added the patches for this issues.

I think that now the behavior is just transparent to the user, he won't notice any regression and now it's possible to work with anchors that have content.

In the fcklink.js I've added some code so that the fake class in IE isn't displayed in the dialog, I think that we could add the code to the table dialog so the class is editable in a text field instead of the dead code that it has now.

comment:5 Changed 13 years ago by Frederico Caldeira Knabben

It looks great Alfonso. I've just committed a small change in the styles, so now the empty anchors have the same behavior (look and positioning) as non empty ones in IE and Opera.

Please check it to see if you have any objection regarding it.

comment:6 Changed 13 years ago by Frederico Caldeira Knabben

I'm thinking about the "Create/Edit Link" button...

While for us, technicians, anchor and links share the same structure (the <a> tag), I believe end users see it as two different entities, used to achieve two different tasks. This is why, you have a button to create anchors (which is a marker in the document, similar to MS Word bookmarks), and another one to create links (which is text/image that points to somewhere else).

This is the power of FCKeditor. People doesn't have to understand the technical thing behind the scene.

As the Link dialog is made for "links", not <a> tags, leaving the link button active for anchors may be confusing. This is also the reason why we don't have "Link Properties" in the context menu for anchors.

Two side effect of the current behavior can be checked now. Just create an empty anchor and select it. Click the link button and hit "Ok". An error message will show up (when "editing" things, the ok button must just leave things how they were before). Then, set the link URL. Now you have an empty link. Go to the Preview and you will have no ways to click that link.

comment:7 Changed 13 years ago by Alfonso

The change to lower the image for empty anchor is perfect, I didn't want to mess with it as I've had too many headaches with vertical-align.

I understand that for non-technical people the anchors and the links are different things, that's why I didn't want to leave active the unlink command in anchors, and also the reason why I have left enabled both the link and anchor context menu if a link has a name attribute (like it does happen if you copy content from Word like footnotes.

But I don't know if disabling the link command is the right thing to do. Look for example at the image command, it's enabled on the fake images so if a user presses it it gets the chance to change some image that he won't understand.

Meanwhile I've added some other changes: in the anchor dialog if the user edits an anchor and leaves the name field blank then I remove the anchor (taking care about the fact that it can also be a link or it can have some content), and a feature request that when I saw I though that it was "useless", but as I was changing this code I found that it's easy so I've done it because it can avoid problems for the users.

Now I remember another thing: validate the characters used in the anchor name, I'll try to find out a regexp that can say if a name is valid or not.

comment:8 Changed 13 years ago by Frederico Caldeira Knabben

Milestone: FCKeditor 2.4

Ok... this feature is approaching its perfection. Let's leave the link button as you have implemented. We can always change it in the future based on users' feedback.

After the name validation, I think you can merge it to the trunk.

comment:9 Changed 13 years ago by Alfonso

I've added just this regexp:

	sNewName = sNewName.replace( /[^\w-_\.:]/g, '_' ) ;

That's according to the spec the valid set of characters, it should also start only with a letter, but that doesn't seems to bring any trouble.

I've made the change silent, so the user isn't confronted with useless warnings (like "you can't have spaces" or worse "the valid set of characters is only \w-_\.: "

If this is OK, how should I act now?

I mean: I'm still learning about branches so I don't know if I should try to merge in the branch with the current trunk and then if everything is fine go to the trunk or do it the other way and merge in the trunk the changes of the branch?

It seems to me that for this branch it's just easier to merge directly into the trunk, but for a complex branch the proper way should be to merge into the branch the changes of the trunk (every now and then instead of letting it diverge too much)

comment:10 Changed 13 years ago by Frederico Caldeira Knabben

Ok Alfonso... the name check is perfect.

I've updated the SVN page with more info regarding the merging. You can proceed with it.

I would prefer to make the "complex branch" approach the standard way of doing it. I mean, merging the trunk in the branch, and after that, the branch to the trunk. The only important thing is to manage well the correct revisions to do those tasks.

I think that we just need to make some practicing and then we'll be used to it.

Thanks again.

comment:11 Changed 13 years ago by Alfonso

Resolution: fixed
Status: assignedclosed

I've done the merge in the branch and then merged the changes to the trunk. This is done, I'll delete the branch once we are sure that there aren't any other problems.

comment:12 Changed 13 years ago by Frederico Caldeira Knabben

It seams the merge has been completed perfectly. Nice job Alfonso!

comment:13 Changed 13 years ago by Frederico Caldeira Knabben

This feature has been committed to the public SVN.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy