24694 - Can't get out of link insertion

Status: VERIFIED FIXED

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

Description

[Akkana Peck]

Start composer.  Type a few chars, then press the link button (selection collapsed).
Type something for the link text, and something for the link reference, and hit ok.
The link goes in, but the selection is inside the link.  Type a few more chars,
observe that you're still inside the link.  Hit return a few times, and a few
more chars; you're still inside the link.  Hit the HR button: the link is split,
so any text you type below the hr is still inside the link.

I would suggest:
1. Link insertion should leave the selection outside the link when done.
2. Inserting a break when in a link should end the link, not split it.
3. Inserting a non-text node when in a link should end the link, not split it.
Points 2 and 3 are definitely arguable.

I can't test what happens in 4.x because the link dialog crashes 4.61/linux
every time.

Comment 1

[Joe Francis]

in 4.5 we have a "remove all styles" toolbar thingy.  You can click on that and 
further typing will not be in the link.  Are we going to have anything like this 
on our 5.0 ui?

Comment 2

[Charles Manske]

Getting out of a link is much easier than in 4.x, but not if link is the last
think inserted in the page. Thanks for the bug, as it reminds me that we did
plan to insert the link and a non-link space after it, so this behavior wouldn't
happen. We broke the old "Remove all styles" into 2 parts in 5.0: Remove text
styles and Remove Link(s). It seems the latter should behave as "Terminate/Stop
link" when the selection is a caret. 
As to your suggestions 2. and 3., I guess it might not be a bad idea to end the
link, but you're not suggesting removing the link associated with the right
side after spliting a link with a break or non-text insert, are you?
This sounds like a good newsgroup topic!

Comment 3

[Akkana Peck]

In fact, that was what I was suggesting, and I know it's arguable; if everyone
thinks it would be dreadfully unintuitive I'll be happy to withdraw either or
both the second and third suggestions.  I was just wondering how often people
actually want items like a br or an hr inside a link.

Comment 4

[Charles Manske]

After serious consideration, making sure a link is padded with spaces should 
be done in a more central location in the rules code, since the same checking
must be done when pasting HTML. Sorry to give Joe another bug, but he's the 
expert at this sort of thing. There isn't time to do this work for M14, so
setting to M15.
To ease this problem, I'll try to figure out what's wrong with RemoveLink
method so user can use that to terminate a link (bug 26994)

Comment 5

[Akkana Peck]

Taking this bug since Joe is overloaded with PDT+ stuff.  I have a fix that
ought to fix it (changing CreateBogusNodeIfNeeded to insert a bogus node at the
end of the document if we just inserted a non-text node and there's nothing
editable after it), but it turns out that it triggers the "strange position for
XIC" assertion (which is also a PDT+ bug, and which is also triggered when you
delete everything in the document) and although the caret appears to be in the
right place after the insert link, if you type, the characters end up in the
wrong place.

Comment 6

[Akkana Peck]

Accepting.  I'll also attach my current patch, in case Joe has time to look at
it.  Since it changes the rules code handling of the bogus node a bit, I'd like
to get Joe's blessing that it's okay to have a bogus node in a non-empty
document.

Comment 7

[Akkana Peck]

Attachment: the patch so far ...

Comment 8

[Akkana Peck]

That patch was broken.  Here's a better one, which inserts the bogus node in the
right place and appears to put the caret in the right place as well (before the
newly created bogus node).  Unfortunately, typing doesn't work after this, and
I'm not sure why.

Comment 9

[Akkana Peck]

Attachment: Better patch

Comment 10

[Akkana Peck]

Typing doesn't work afterward because of 27742 -- there's something weird about
the InsertLink dialog that confuses the event state manager and leads to a
crash.  This happens even without my patch.

Comment 11

[Akkana Peck]

Joe has a fix that fixes the caret-warping problem.  When my fix is combined
with Joe's fix, I can now type stuff after the link, and it goes into a new text
node and not into the link.  Also, I can click after the link and then type, and
what I type is not part of the link.

Comment 12

[Akkana Peck]

I've checked in the nsTextEditRules.cpp part.  Joe, this needs your
pointer-warping code in nsHTMLEditRules in order to make this work.  Reassigning
to Joe so he can check in his part.

Comment 13

[Joe Francis]

pointer warping code?  I'm confused. Do you just mean the nsHTMLEditRule.cpp 
changes that I gave you?  Or is something more needed?   Accepting bug.

Comment 14

[Joe Francis]

ok, I need to rework this a bit.  The first fix casued bug 27959.

Comment 15

[bobj]

A comment was made last week:
   --- Additional Comments From [e-mail] 2000-02-09 12:03 ---
   After serious consideration, making sure a link is padded with spaces
   should be done in a more central location in the rules code, since
   the same checking must be done when pasting HTML.

Padding spaces around links may be OK for English, but not for other
languages such as Japanese or Chinese which do not normally separate
words with spaces.

Comment 16

[Joe Francis]

I have a fix in hand now.  It doesn't use any added spaces.  awaiting approval

Comment 17

[Joe Francis]

fixed

Comment 18

[sujay]

this doesn't look fixed..also using remove all styles keeps it in link mode
and makes the text link italic!

Akkana, does this look fixed for you?

Comment 19

[sujay]

Akkana and I both tested this again and its still not fixed in today's
2/23 build.

Comment 20

[Joe Francis]

REmove All Styles is broken.  If you have a collapsed selection in regular text, 
Remove All Styles makes further typing be bold + italic + underline!  You or I or 
someone should file a seperate bug on this.

I'm surprised that inserting a link leaves you inside the link.  It doesn't for 
me.  When I type after inserting a link, I get non-link style.

Comment 21

[sujay]

I did file a bug for the remove style issue this afternoon...

Comment 22

[leger]

Please give us ETA for fix in the Status Whiteboard.

Comment 23

[Joe Francis]

I know the jist of whats going wrong here.  Sometimes there is no break node 
after the link, in which case selection has no choice but to be in the link 
(assuming the link is at the end of the document, or even the end of a block).  
This shouldn't take more than a couple of hours to fix.  I should be able to get 
to it tomorrow (Thursday).

Comment 24

[mjudge]

i cant reproduce this joe test on your system

Comment 25

[Joe Francis]

I'd like to remove this from PDT+ consideration, for the following reasons:

First, the bug now only shows up in some circumstances, rather than all the time 
(note that when Mike looked into this he couldn't reproduce it).
Second, since we have "Remove Link" menu item working, the user has a workaround.

I could fix this, and probably pretty quickly, but I'd have to make changes that 
are high risk.  I would rather do this carefully during M15.

Comment 26

[leger]

Putting on PDT- radar for beta1.

Comment 27

[Kathleen :Brade]

remove beta1 and PDT-; move to M15

Comment 28

[Joe Francis]

fixed

Comment 29

[sujay]

verified in 3/30 build.