99089 - Inserting a link should pad with a non-link space and move caret outside the link

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Charles Manske]

The difficult with terminating a link (e.g., bug 75035) would be helped by always
adding a non-link space when inserting a link. This is especially problematic
when link is inserted at the end of a document or as last content in a table cell.
We did this in 4.7, I think only when link is pasted, not when inserted via the
link button+dialog.
It is tricky since you should not add the space if there's already one after the
link, but instead only move the caret.If the latter request is too tricky, maybe
we can should add the space only if we are at end of cell or document?

Comment 1

[Joe Francis]

097

Comment 2

[Joe Francis]

Charlie, are you saying you only want this when pasting?  Or also when using
link tool?

Comment 3

[Charles Manske]

It would be best if this happened when any link is created, including pasting,
inserting a link via link dialog, and when a link is created around existing 
text.

Comment 4

[Charles Manske]

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

Comment 5

[Joe Francis]

pushing off 098 to 099

Comment 6

[Kathleen :Brade]

making EDITORBASE+ per meeting

Comment 7

[Joe Francis]

099 to 1.0; set pri to 1 for these pushed off EB+ bugs

Comment 8

[Joe Francis]

Attachment: patch to libeditor

handles three cases: inserting a link around existing selection; creating a new
link and link text at collapsed selection; pasting html that ends in a link.

Comment 9

[Joe Francis]

revised patch coming later this morning.  revision will also fix bug 112167:
copy/paste list item leaves selection in funky place

Comment 10

[Joe Francis]

Attachment: revised patch to editor

This patch improves paste behavior, and also fixes bug 112167

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 74759 [details] [diff] [review]
revised patch to editor

In nsHTMLEditor.h :

>-  NS_IMETHOD TypedText(const nsAReadableString& aString, PRInt32 aAction);
>+  nsresult TypedText(const nsAReadableString& aString, PRInt32 aAction);

This nsresult above makes build on Win2K fail:

 nsHTMLEditor.h(410) : error C2555: 'nsHTMLEditor::TypedText' : overriding
 virtual function differs from 'nsPlaintextEditor::TypedText' only by
 return type or calling  convention

Comment 12

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: image test case

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 74759 [details] [diff] [review]
revised patch to editor


This patch is problematic for web authors using images as source of links.
Example :
  1. create a new document in Composer
  2. insert *twice* the image attached above (attachment 74931 [details])
  3. select the first one and click on Link button ; insert link to #foo
  4. click at end of line and come back to beginning of line using left arrow

  you'll notice that the patch inserts a whitespace between the two images.
  When the node right after the link is an image, no whitespace should be added.

Comment 14

[Charles Manske]

Since I filed this bug, I did think it was a good idea, but I was thinking 
primarily about inserting text links. Daniel's comments about image links is 
very good; we do *not* want to insert extra spaces between images.
Would it be hard to do the extra check for an image as Daniel suggests?
I *do* like the fact that the paste-related portion fixes bug 112167. I think
that problem is more important than this one.
So I'm ambivalent. If there are more important bugs that Joe should be fixing,
I think we can postpone fixing this for 1.0

Comment 15

[Joe Francis]

I can fix 112167 withouth this patch.  I only lumped them together because the 
patches kinda overlap.  But they are independent.  In fact, I'm seperating out 
the 112167 patch to post it seperately and get it reviewed approved etc.

I would need a spec on what to do in various cases before I could proceed here.  
That's why I am seeking feedback.  If everyone can agree on what I should do I 
will try to do it.

If you want my opinion, I'm inclined to drop the whole idea of adding spaces.  I 
think instead that we should just make sure that when you paste a link, or create 
a new one at a collapsed selection (ie, you type the text in the dialog) that we 
make sure the resulting collapsed selection is outside the link.  That way 
further typing will not be a link.  But I am notorious for being out of sync with 
what our users want.

Comment 16

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

> If you want my opinion, I'm inclined to drop the whole idea of adding spaces.  I 
> think instead that we should just make sure that when you paste a link, or create 
> a new one at a collapsed selection (ie, you type the text in the dialog) that we 
> make sure the resulting collapsed selection is outside the link.

Ooooh, I find this solution elegant, simple, and clear.

Comment 17

[Syd Logan]

I would like to see link extension after a click to be the exception, not the
norm. If you click, the caret should be considered outside the link, and maybe a
space is added if we are at the end of a list, end of a table, end of a line. If
you click and hold the shift key (control key??) then the caret is placed inside
the link, and typing or pasting becomes a part of the link. 

My feeling is that most people clicking after a link want the text or content to
not be a part of the link. If they want to change the link, they are likely
selecting the link text, or editing it in the link properties dialog. Clicking
at the end is to me almost a guarantee that they are typing content not a part
of the link.

my 2 cents

Oh, and it would be cool if there was a link icon in the UI to show state before
you type. If it is bright or solid, typing happens in a link. If it is dimmed,
the caret is not in a link.

Comment 18

[Joe Francis]

The days of having a half dozen milestones out in front of us to divide bugs 
between seem to be gone, though I dont know why.  Lumping everything together as 
far out as I can.  I'll pull back things that I am working on as I go.

Comment 19

[Joe Francis]

migrating milestones now that there are more available.  i'll pull these back as
I work on them

Comment 20

[Kevin McCluskey (gone)]

nsbeta1-

Comment 21

[saari (gone)]

should be fixed, but we not going to add the non link space

Comment 22

[rubydoo123]

tested using trunk build 200304308 on win2K

scenarios tested:
1. set the caret at the end of a line, select link icon, enter related text and
location, select OK, then continue to type. The text following the link is
normal text not linked text.[[good]]

2. select a link from the browser, set caret at the end of the line, select
paste, beginning typing. The text after the paste was outside of the link. [[good]]

3. type a line of text, select the last word in the line, select the link icon
and enter a link location, select OK, begin typing. The text after the link
continues as an extension of the link. [[bad]]
Hitting enter, however, terminates the link. [[good]]

4. insert table, set caret in a cell, enter text, select text and select link
icon, enter location, select OK, continue to enter text. The text following the
link continues as an extension of the link. [[bad]]

5. in another cell, select link icon, enter related text and location, select
OK, then continue to type. The text following the link is normal text not linked
text.[[good]]

6. select a link that was defined above, set caret within a cell, select paste,
beginning typing. The text after the paste was outside of the link. [[good]]


The remaining issue is when existing text is selected to create the link both in
the document paragraph or in a table cell.

Comment 23

[rubydoo123]

just talked with Kin, and the remaining issue : not being able to select out of
the link if the link is inserted by selecting existing text, and creating link :
and not being able to set the caret outside the link if the link is the last
thing or only thing -- is really a selection bug and is already filed.

Comment 24

[rubydoo123]

verified