Page MenuHomePhabricator

[Regression wmf4] Link continuation is broken
Closed, ResolvedPublic40 Estimated Story Points

Description

Screenshot

Steps to reproduce:

1.Open a page with VE
2.Select a text and apply link on it
3.Now while link is selected, type something else to replace the link text.

Observed Result:
The previous link is getting applied only to the first letter of the newly added text

See the screenshot attached

Environment:Betalabs


Version: unspecified
Severity: major
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=72261

Attached:

Screen_Shot_2014-10-15_at_4.46.51_PM.png (81×231 px, 9 KB)

Details

Reference
bz72108

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:57 AM
bzimport set Reference to bz72108.

Yes, this is related to unicorning. There are several problems.

Link continuation is now no longer done through pawning, because pawning is evil and kills IMEs. However, now when you type at the end of a link, DM and CE get out of sync: DM knows to continue the link until the first word break, but CE will either continue indefinitely or not at all depending on the browser (in Chrome, it doesn't continue at all). This leads to interesting surprises: you type at the end of a link, the text you type doesn't appear linked, but as soon as you press backspace it does, but only the text up to the first space.

David and I had this idea of doing link continuation with unicorns. We could unicorn whenever the cursor is at the edge of a forceContinuation annotation. That would allow IMEs to type text there, and the text you type would either appear like it was all part of the link or like none of it was part of the link (looks like currently it's none of it) until such time as we can safely de-unicorn (generally when you move the cursor somewhere else).

This logic doesn't seem to be working very well. I can see it work in the standalone demo, but only when I move the cursor using the arrow keys (not when using the mouse) and only when moving from the right to the left (not when moving left to right). I think this might be due to some of Ed's selection changes causing the render lock to be engaged at the time when we would like to start unicorning. It doesn't unicorn at all in VE-MW, for reasons I don't understand.

Even if we do get this to work as originally designed again, the user experience isn't great. The typed text would appear as either linked or not linked, even past a word break, until the selection is moved. It would be nice if we could do one of the following:

  • De-unicorn as soon as a wordbreak is inserted. I think it's likely that this isn't feasible because there are IMEs that can insert wordbreaks. David?
  • De-unicorn aggressively, either at the first character or at the first wordbreak, when we have not seen any compositionstart/end or IME-related events. David, I know that we can't reliably detect when an IME has closed, but maybe we can detect that no IME interaction whatsoever is happening? Or are there IMEs that fly under the radar so much that we can't detect that they're being used?
  • Throw the towel in the right and change our link continuation logic to either 'always' or 'never'. I don't really like solving a problem by redefining it to be correct behavior, and I also don't think it would be a good user experience, but IIRc this was David's first suggestion when confronted with this general problem.

Oh also: the bug as filed (select link, start typing, replacement text is linked in CE but not in DM) also appears to be caused by the unicorn change, presumably because such replacements are now considered "simple" rather than "complex" so as to not get in the way of IMEs that replace text?

But I'm widening the scope of this bug to "Link continuation is broken".

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

(In reply to Roan Kattouw from comment #3)

Oh also: the bug as filed (select link, start typing, replacement text is
linked in CE but not in DM) also appears to be caused by the unicorn change,
presumably because such replacements are now considered "simple" rather than
"complex" so as to not get in the way of IMEs that replace text?

Actually, I think it's just due to the basic issue with link continuation when typing. The change of the linked word (<a href="x">before</a>) to the first character typed (<a href="x">a</a>) works correctly. But then the continuation does not occur, so we end up with just the first character linked (<a href="x">a</a>fter).

I tell a lie, in Chromium the behaviour is as Roan describes for the first letter, i.e. linked in the DOM but not in DM (but then subsequent letters are not linked the DOM either). In Firefox, there is no link in either the DOM or in DM.

https://gerrit.wikimedia.org/r/168327/ is a POC fix to the problem of right-cursoring to a continuation position, and it also illustrates the general issues in the other cases:

  1. Unicorning is triggered by DM selection changes only if ve.dm.Surface 'insertionAnnotationsChange' is fired: that's how ve.ce.Surface renderSelectedContentBranchNode gets called. But moving to a continuation position doesn't necessarily mean the insertion annotations will have changed.
  1. DOM-originated selection changes are applied to the DM with a render lock in place. This has to be so, because there are more DOM cursor positions than DM ones.
  1. Therefore, perhaps we should make continuation unicorning happen even if there is a render lock in place, probably with its own emit event.
  1. The DM needs to know for which annotations will browser continuation suffice. The merged code in ve.ce.ContentBranchNode getRenderedContents special-cases links only, whereas the patchset above adds a property to the annotation classes.

Continuation unicorns can give us short-term consistency, but not the desired "word-break" behaviour. If we want to do go with the short-term fix, we should define exactly what behaviour to implement. In the longer term, a UI change might provide a better experience -- e.g. show the user more explicitly that they're inside link continuation and let them cursor/click out of it.

(In reply to D Chan from comment #7)

  1. Unicorning is triggered by DM selection changes only if ve.dm.Surface

'insertionAnnotationsChange' is fired: that's how ve.ce.Surface
renderSelectedContentBranchNode gets called. But moving to a continuation
position doesn't necessarily mean the insertion annotations will have
changed.

  1. DOM-originated selection changes are applied to the DM with a render lock

in place. This has to be so, because there are more DOM cursor positions
than DM ones.

  1. Therefore, perhaps we should make continuation unicorning happen even if

there is a render lock in place, probably with its own emit event.

Both #1 and #2 are good reasons for #3. Maybe this could be done in setSelection()?

  1. The DM needs to know for which annotations will browser continuation

suffice. The merged code in ve.ce.ContentBranchNode getRenderedContents
special-cases links only, whereas the patchset above adds a property to the
annotation classes.

Yeah there is a CE property for this (forceContinuation), but I guess my proposed solution to #3 may require the DM know. I would still prefer, though, to find a way to keep this knowledge in CE, possibly by having DM emitting too many events that are then filtered by CE.

Continuation unicorns can give us short-term consistency, but not the
desired "word-break" behaviour. If we want to do go with the short-term fix,
we should define exactly what behaviour to implement. In the longer term, a
UI change might provide a better experience -- e.g. show the user more
explicitly that they're inside link continuation and let them cursor/click
out of it.

Yeah we have talked about this in the past. I will try to find this and show it to you, it's scarily analogous to unicorns (before unicorns were invented).

gerritadmin wrote:

Change 168327 had a related patch set uploaded by Catrope:
Make setSelection emit activeAnnotationChange if continuation unicorns may be needed

https://gerrit.wikimedia.org/r/168327

  • Bug 72638 has been marked as a duplicate of this bug. ***
Jdforrester-WMF renamed this task from VisualEditor: [Regression wmf4] Link continuation is broken to [Regression wmf4] Link continuation is broken.Nov 24 2014, 4:48 AM
Jdforrester-WMF set Security to None.
Jdforrester-WMF lowered the priority of this task from High to Medium.Jan 15 2015, 12:15 AM

@dchan/@Jdforrester-WMF, is the issue described in T102919 related to recent work in link continuation area?

nshahquinn-wmf raised the priority of this task from Medium to High.Jun 30 2015, 10:51 PM

Change 217257 had a related patch set uploaded (by Divec):
Explicitly enter and exit link annotations

https://gerrit.wikimedia.org/r/217257

Commit a32524705038bfc832bcd0e04e2e4e6322b080f5 (heed cursor position relative to annotations) has bearing on this, in that if we ripped out the special casing for links, the native behaviour of Firefox would just work. However, Chromium aggressively discontinues links, so getting reasonable and consistent behaviour still needs T91285 .