Closed Bug 1357365 Opened 7 years ago Closed 3 years ago

Cannot exit links in contenteditable elements, i.e. no way to place caret cursor outside `<a href>` element when there is no editable/navigable content before or after <a href>

Categories

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

Product:
Core
Component:
DOM: Editor
Version:
52 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
firefox86 --- fixed

People

(Reporter: j.swiderski, Assigned: masayuki)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [h2review-noted][Thunderbird painpoint], [wptsync upstream])

Attachments

(10 files)

test_link.html
331 bytes, text/html
Details
Bug 1357365 - part 0-1: Make `test_typing_at_edge_of_anchor.html` test both on `contenteditable` and `designMode` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1357365 - part 0-2: Add WPT for testing whether typing text is inserted into or outside of a link in various cases r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1357365 - part 1: Rename `HTMLEditor::OnMouseDown()`, `HTMLEditor::OnMouseUp()` and `HTMLEditor::OnMouseMove()` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1357365 - part 2: Make `TypeInState::OnSelectionChange()` stop inserting new content into the link if clicked outside it r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1357365 - part 3: Make `TypeInState::OnSelectionChange()` stop keeping link style if caret is positioned at edge of a link element and not moved in the link r=m_kato!,edgar!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1357365 - part 4: Make `TypeInState` stop keeping link style when selection change command is performed, but it didn't change selection r=m_kato!,edgar!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1357365 - part 5: Make `TypeInState::OnSelectionChange` stop keeping link style for new inserting content when selection is changed by JS r=m_kato!,edgar!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1357365 - part 6: Make `TypeInState::OnSelectionChange()` stop keeping link style when selection is not collapsed and the range is not entirely in a link r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1357365 - part 7: Make `TypeInState::OnSelectionChange` stop keeping link style for new inserting content after content around a link edge is deleted r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Attached file test_link.html — 
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170323105023

Steps to reproduce:

1. Use below code on your HTML page and open it in a browser:
<div style="margin:20px; border:1px solid black; width:500px;" contenteditable="true">
<p><a href="http://test">http://test</a></p>
</div>
2. Click right behind letter "t" and once cursor is blinking, press Space, Right Arrow or Enter or do all of that and then start typing.


Actual results:

Newly typed text is a link and it seems there is no way of getting out of it. Even if I press enter, new link is created and text is still linked.


Expected results:

Newly typed text should be a plain text.
URL: data:text/html,<div style="margin:20p...
Status: UNCONFIRMED → NEW
status-firefox55: --- → affected
Component: Untriaged → Editor
Ever confirmed: true
Product: Firefox → Core
Blocks: editor-blink-compat
Priority: -- → P3
Chrome seems to always position the cursor outside the link when you put it at the border of the link, but inside when it comes to bold (I didn't test other markup).  But if you have other text on the line, like abc<b>def</b>ghi, it will position the cursor outside the <b> if you place it before, and inside for after, like

  abc|<b>def</b>ghi
  abc<b>def</b>|ghi

I vaguely remember testing this a long time ago and finding this was how Word behaved, but I could be wrong.  It seems reasonable to match Word's behavior here.
After landing bug 1422234, pressing Enter at end of link stops creating another <a> element in new paragraph. However, we still don't have a way to put caret outside the <a> element.

After landing bug 1496320, if caret is moved from outside of <a href>, typing new character at edge of the element is treated as typing outside it. I guess that the remaining issue is, the case when there is no content before or after <a href> in the block or line.

status-firefox55: affected → ---
Depends on: 1496320, 1422234
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [h2review-noted]

I wonder this could be related to bug 1667129. Could we create an empty frame on a line edge so that reaching the line end could:

  1. Put the caret in a non-anchor plain text frame
  2. Make sure the caret is always in visual edges of lines, even in mixed bidi context

I'm sharing my random thought before actual investigation, though.

Emilio, does comment #5 make sense to you? It could also remove a workaround in nsCaret.

Flags: needinfo?(emilio)

So when you have an element in <div><a>bcd</a></div> you don't have any text frame, but I think moving past the d should set the caret association mode to CARET_ASSOCIATE_AFTER... I'm not sure you really need more DOM for that.

I'm not sure what you mean with (2) though, can you elaborate?

Flags: needinfo?(emilio)

(any text frame outside of the link I mean)

Well, I don't know selection handling with pointing device events though, cannot put selection after the <a> element and in the <div>? I.e., div - 1 in this case. If it's reasonable, editor could handle TypeInState specifically.

I'm not sure what you mean with (2) though, can you elaborate?

https://bug1667129.bmoattachments.org/attachment.cgi?id=9177660

If you select the first line from right to left by mouse, it's hard to fully select the RTL word because the logical end of the line is visually at the middle of the line. (Dragging to outside of the window should fully select the line but it does not.) I was wondering if adding an empty LTR frame at the end of the line could help here.

See Also: → 1273655
Whiteboard: [h2review-noted] → [h2review-noted][Thunderbird painpoint]

Well, today, I have much time for this. I'll post a patch.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Summary: Can't exit links in contenteditable elements → Can't exit links in contenteditable elements when you click outside `<a href>` element

Perhaps, the reasonable solution is, we should take almost same behavior. However, for better UX and keeping traditional better behavior,

  1. Basically, we should unlink when selection is changed and it's not in an <a href> element.
  2. But don't unlink when selection is collapsed to start or end of an <a href> with clicking first or last content.
  3. And also don't unlink when caret is moved to start or end of an <a href> with a key press and the previous caret position is in the link

So, I think that we should unlink if selection change occurred by JS for making exactly same behavior as the other browsers from the point of view of Selection API.

(Midair-collision with comment 21, will look at that in my next comment)

Hi Masayuki, thank you very much for picking this up, that'll be a nice Christmas present!!! As the duplicates testify, this has been an everlasting painpoint especially for Thunderbird.

Let's try to elaborate a bit what we really want here.
Please let me know what you think.
Sorry if I sound clumsy.

I think the remaining problem isn't exactly about clicking outside <a href>, that actually works pretty well when there's any editable content after the link: If you click on the left leg of the W, you can type regular text, but if you click on the right rounding of the D, you'll continue the link. Same for cursoring, coming from link will extend the link, coming from text will extend text:
<a href="...">linktextEND</a>Whatever

So per your own comment 3, the real problem here (as seen in attachment 8859119 [details]) occurs only when there's no editable (cursor-navigable) content before or after the link, regardless if you mouse-click or navigate with navigation keys: That is when there's no way to place the input cursor before or after the link, as indicated below:

Desired input positions for adding unlinked text outside the link:
<p>|<a href="http://test">http://test</a>|</p>

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold, working slower) from comment #3)

After landing bug 1496320, if caret is moved from outside of <a href>, typing new character at edge of the element is treated as typing outside it. I guess that the remaining issue is the case when there is no content before or after <a href> in the block or line.

So I think we want to fix two problems here:

  1. Mouse click before and after link: Add a condition that if there's no editable/cursor-navigable content before/after <a href>, clicking before or after the link(text) (or even on the very edges of linktext) must place the input cursor outside the link (for entering plain vanilla text).
  2. Cursor navigation: We must provide a way to reach the cursor position outside the link when there's no other editable/cursor-navigable content around it.

For cursor navigation, here's my proposal:

  • Let's remain consistent with the behaviour recently implemented in bug 1496320: Navigating from inside the link exactly to its edge (i.e. from linktext character pos 1 to pos 0, or link.length-1 to link.length) extends the link.
  • However, when the input cursor is already on the edge of the link text (pos 0, or link.length), and user deliberately presses any navigation keys again trying to move "more" outwards, that's when we should place the input cursor outside the link for entering plain text. Iow, while the visible cursor won't change its position, the internal editing position changes on the edges if you continue navigating (note that selections with Ctrl or Shift should not have that behaviour, but always immediately include the link edges).

Keyboard navigation from inside the link:
<p><a href="http://test">http://tes|t</a></p>

Keyboard navigation from inside (e.g. Arrow-right, or END), now on edge (extend link style):
<p><a href="http://test">http://test|</a></p>

Keyboard navigation from edge (e.g. Arrow-right or END again or held down), now outside link (plain text):
<p><a href="http://test">http://test</a>|</p>

I think we once had this behaviour even for ongoing text, but it was removed because a cursor which doesn't move after pressing a navigation key is irritating. However, this case is different as it is an edge case, and no further cursor movement is expected as we've already reached the edge. However, still doing the "internal" cursor movement from inside to outside the anchor tag will be tremendously helpful, and I can't think of any other keyboard way consistent with the current behaviour to solve this.

Flags: needinfo?(masayuki)
Summary: Can't exit links in contenteditable elements when you click outside `<a href>` element → Cannot exit links in contenteditable elements, i.e. no way to place caret cursor outside `<a href>` element when there is no editable/navigable content before or after <a href>

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold, working slower) from comment #21)

Perhaps, the reasonable solution is, we should take almost same behavior. However, for better UX and keeping traditional better behavior,

  1. Basically, we should unlink when selection is changed and it's not in an <a href> element.

I don't understand - can you elaborate? Is this mouse or keyboard? Or programmatically?
Is this new or existing behaviour?

  1. But don't unlink when selection is collapsed to start or end of an <a href> with clicking first or last content.

Ok, let's take this example:
<p><a href="...">STARTlinktextEND</a></p>

Do you mean this?

  • don't unlink when clicking on outer edges of S or D (existing behaviour)
  • (New:) Unlink when clicking "outside" S or D (is that even possible? What if there's no editable whitespace to click on, as on the left side of testcase attachment 8859119 [details]?)
  1. And also don't unlink when caret is moved to start or end of an <a href> with a key press and the previous caret position is in the link

Yes. That's existing behaviour. The interesting part is just after that:

  1. Unlink when navigation key is pressed with current internal cursor position still "inside" the link, but already on the edge.

So, I think that we should unlink if selection change occurred by JS for making exactly same behavior as the other browsers from the point of view of Selection API.

What's the behaviour in other browsers?

Btw, my older version of Word (2003) has a very simple, non-sticky behaviour:

Cursor at the edge of linktext will always create plain text (unlinked). That feels much better because there's no tricky sticky link stuff going on. So it becomes a bit harder when you want to change linktext on the edges, but you can always double-click to explicitly edit link text in a dialog, or to navigate one character to the inside of linktext, add what you want, and delete the outmost linktext character.

(In reply to Thomas D. (:thomas8) - [PTO 18 Dec - 03 Jan] from comment #22)

(Midair-collision with comment 21, will look at that in my next comment)
I think the remaining problem isn't exactly about clicking outside <a href>,

Exactly.

that actually works pretty well when there's any editable content after the link: If you click on the left leg of the W, you can type regular text, but if you click on the right rounding of the D, you'll continue the link. Same for cursoring, coming from link will extend the link, coming from text will extend text:
<a href="...">linktextEND</a>Whatever

Yes, I agree with that it's not an expected behavior for any users. So, only when clicking on the right half of the "D", editor should unlink temporarily for coming input.

So per your own comment 3, the real problem here (as seen in attachment 8859119 [details]) occurs only when there's no editable (cursor-navigable) content before or after the link, regardless if you mouse-click or navigate with navigation keys: That is when there's no way to place the input cursor before or after the link, as indicated below:

Desired input positions for adding unlinked text outside the link:
<p>|<a href="http://test">http://test</a>|</p>

Well, but it's hard to change caret position. So, I just change editor's temporary style (called "type-in-state" by its original author) in the mouse cases. For key navigation, I need another approach.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold, working slower) from comment #3)
For cursor navigation, here's my proposal:

  • Let's remain consistent with the behaviour recently implemented in bug 1496320: Navigating from inside the link exactly to its edge (i.e. from linktext character pos 1 to pos 0, or link.length-1 to link.length) extends the link.

Yeah, I think that it's reasonable, but I think that we should consider it with the command instead because it's hard to count the code points of first or last grapheme cluster. So, perhaps, editor should keep current behavior only when character-move command is ordered by a key press (typically, left/right arrow key without any modifiers).

  • However, when the input cursor is already on the edge of the link text (pos 0, or link.length), and user deliberately presses any navigation keys again trying to move "more" outwards, that's when we should place the input cursor outside the link for entering plain text. Iow, while the visible cursor won't change its position, the internal editing position changes on the edges if you continue navigating (note that selections with Ctrl or Shift should not have that behaviour, but always immediately include the link edges).

I don't think that editor should change "type-in-state" without any visual feedback. But anyway, this is not possible change with simple code under current design.

Flags: needinfo?(masayuki)

Currently, the test checks the result only in contenteditable, but I'd be
better to do same tests in designMode editor too.

Additionally, for detecting regressions of the following patches, it should
check the result of 2nd typing too because inserting text into end of a link
element may cause unlink it.

This patch adds WPTs whose expected result is based on Chrome's behavior.

Depends on D100997

They were designed for object resizer and grabber to move absolutely positioned
element. Therefore, they should have better name to explain what they do.
Then, we can create event listener methods for generic cases.

Depends on D100998

When mouse button is clicked outside a link element but caret is positioned
start or end of the link, our traditional behavior keeps inserting new content
into the link. But this is different from the other browsers, and it does
not make sense to treat such selection change is intended to keep typing in
the link element.

Therefore, this patch makes TypeInState::OnSelectionChange() handle
selection change reason is mousedown and mouseup cases. However,
it cannot know whether the event was fired in the parent link element or
not. Therefore, this patch makes HTMLEditorEventListener notifies
TypeInState of mouse events via HTMLEditor.

Depends on D100999

Although different from the other browsers' behavior, our traditional behavior
might be better than them because the behavior on the other browser does not
allow users to insert new content at start nor end of a link.

However, we can relax more about keeping traditional behavior for web-compat.
Perhaps, only when caret is moved from the other side of first or last character
in the link and moves caret to the edge of the link with arrow key, we should
allow users to modify the link text.

Otherwise, e.g., Home and End key press should make it stop keeping the
link style. This helps advanced users who are familiar with keyboard navigatin
in editor.

Note that this patch also changes the condition which checks
aReason & nsISelectionListener::KEYPRESS_REASON to check also
nsISelectionListener::COLLAPSETO_START_REASON and
nsISelectionListener::COLLAPSETO_END_REASON because all of them are
set by nsFrameSelection::MoveCaret exclusively.
https://searchfox.org/mozilla-central/rev/a0ccd492719b1ad2106f6456549be62a76f45acb/layout/generic/nsFrameSelection.cpp#738,741,745
Therefore, they should be treated as same as a key press.

Note that they are also set by Selection::CollapseToStart and
Selection::CollapseToEnd too. But a following patch will add a new
reason to notify selection listeners of caused by JS. So, the problem
will be fixed by the following patch.

Depends on D101001

When caret is not moved but selection change command is fired, it means that
the caret is already set to start or end of editing host. In this case, we
should stop keeping link style for new inserting content because otherwise,
user cannot exit from the first or last link with arrow keys.

Depends on D101002

This is the most important situation. If selection is collapsed to a edge of a
link, the other browsers does not insert new content into the link. So, from
the point of view of web developers, this cause should work exactly same as
the other browsers.

Note that the new failures in editing/run/inserttext.html are passed only
on Gecko. So, the test should be updated after fixing this bug.

Depends on D101003

When selection is not collapsed and is across link element boundary, we can
assume that user does not try to modify the link text because the other
browsers behave so. Therefore, we should take the same behavior in this case.

Note that all the new failures of editing/run/inserttext.html are passed
only on Gecko. So, it should be updated in another bug.

Depends on D101004

This is same behavior as the other browsers. When selection is collapsed to
a edge of a link after deletion, we should stop applying link style to new
inserted content.

Depends on D101005

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d81ab08e02b6
part 0-1: Make `test_typing_at_edge_of_anchor.html` test both on `contenteditable` and `designMode` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/32f63c6fa98e
part 0-2: Add WPT for testing whether typing text is inserted into or outside of a link in various cases r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ef37ec496436
part 1: Rename `HTMLEditor::OnMouseDown()`, `HTMLEditor::OnMouseUp()` and `HTMLEditor::OnMouseMove()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/8728fb04a7ef
part 2: Make `TypeInState::OnSelectionChange()` stop inserting new content into the link if clicked outside it r=m_kato
https://hg.mozilla.org/integration/autoland/rev/89469f0a9145
part 3: Make `TypeInState::OnSelectionChange()` stop keeping link style if caret is positioned at edge of a link element and not moved in the link r=m_kato,edgar
https://hg.mozilla.org/integration/autoland/rev/642a9425089b
part 4: Make `TypeInState` stop keeping link style when selection change command is performed, but it didn't change selection r=m_kato,edgar
https://hg.mozilla.org/integration/autoland/rev/065517dd03b6
part 5: Make `TypeInState::OnSelectionChange` stop keeping link style for new inserting content when selection is changed by JS r=m_kato,edgar
https://hg.mozilla.org/integration/autoland/rev/49ab7c774b1e
part 6: Make `TypeInState::OnSelectionChange()` stop keeping link style when selection is not collapsed and the range is not entirely in a link  r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c5fb8455ae1a
part 7: Make `TypeInState::OnSelectionChange` stop keeping link style for new inserting content after content around a link edge is deleted r=m_kato
Whiteboard: [h2review-noted][Thunderbird painpoint] → [h2review-noted][Thunderbird painpoint], [wptsync upstream error]
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27170 for changes under testing/web-platform/tests
Whiteboard: [h2review-noted][Thunderbird painpoint], [wptsync upstream error] → [h2review-noted][Thunderbird painpoint], [wptsync upstream]
Upstream PR merged by moz-wptsync-bot

Awesome, thank you so much Masayuki for tackling this!!! :-))
I'll be looking forward to trying this out in action...

Regressions: 1697249
See Also: → 1674359
Duplicate of this bug: 75035
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: