Page MenuHomePhabricator

Add a link in VE: Use a permanent context instead of VE's built-in context
Closed, ResolvedPublic

Description

Currently, the drawer at the bottom of the screen that displays details about the selected suggestion and buttons to interact with it is implemented as a ContextItem that is displayed by VE's context system when the cursor is inside a suggested link. This seemed like the easy way to do it because it's how other annotations are inspected and manipulated in VE, but it has some issues.

Context items are ephemeral, meaning they are rerendered from scratch every time a different annotation is selected or the annotation changes (because annotations aren't editable either, changing a suggestion from undecided to accepted requires blowing it away and reannotating the text with a slightly different suggestion annotation). This is wasteful, and would also make it quite difficult to implement things like transitions for navigating between suggestions.

The context can't be activated manually, it activates automatically based on whether the selection is inside a matching annotation. However, because link annotations are nailed, there are selection management issues that make it difficult for us to convince the context to open or stay open when we want it to (see T267705: Add a link in VE: issues with opening context when selection is at the edges of an annotation).

We should consider using our own context instead of VE's, and have that permanently live at the bottom of the screen. This could simplify a lot of these issues, and could also make it easier to implement the desired design on desktop (where the context defaults to appearing inline at the cursor, instead of at the bottom of the screen).

Related Objects

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
kostajh added subscribers: mewoph, mepps, Tgr, kostajh.

@mewoph @mepps @Tgr I'm inclined to mark this as a not-for-initial-release task, unless we find that there are UX issues with the mobile/desktop implementation that would be solved by switching to a permanent context. Thoughts?

kostajh moved this task from Post-release backlog to April 12 - April 16 on the Add-Link board.

@mewoph will have a look at this in the current week.

Use case for having a permanent context item from discussions in T269638

Thanks as always for the thorough review @Etonkovidova. This may be a case where for @mewoph and @kostajh to decide whether to open as new tasks for the following:

(1)

  • Tapping on links in the article should do nothing (except the link suggestions).

The links (not the suggested links) can be right-clicked and opened in another tab/window. It seems to me perfectly fine. I added to signal that there are, in fact, some ways that users are still able to interact with a content.
Also, tapping on an article's templates, images and on the footer will bring up disabled template editor, media settings and page options window repectively.

Media SettingsPage optionsTemplate editor
Screen Shot 2021-04-07 at 4.55.33 PM.png (691×840 px, 221 KB)
Screen Shot 2021-04-07 at 4.55.14 PM.png (656×1 px, 117 KB)
Screen Shot 2021-04-07 at 5.06.41 PM.png (668×820 px, 209 KB)

This should be fixed as part of initial release imo as this is an important main requirement to not enable opening other cards, especially on Desktop where the add links card is no fixed as an overlay on the entire page. Equally, it should *not* be possible for all add link inspector cards to be closed:

Desktop
image.png (1×1 px, 362 KB)
Mobile
image.png (1×848 px, 388 KB)
unexpected ability to close all link suggestion cards
kostajh triaged this task as Medium priority.Apr 14 2021, 12:53 PM

Initial idea: set up a permanent context item module that stores a reference to the transient context item so that we don't have to re-implement many of the logic that's already there (showing link text, image & article preview etc). This permanent context item will take over the rendering functionality from the transient one.

  • Create RecommendedLinkPermanentContextItem, maybe initialize this in one of the hooks in article target classes
  • RecommendedLinkPermanentContextItem sets up base context item templates (header w/progress indicator, empty body, nav & acceptance buttons) before the first recommendation is selected in AddLinkArticleTarget. This is simpler on mobile since the context item is fixed at the bottom of the page, on desktop, the context item moves as the annotation moves around (as the user scrolls).
  • Upon initialization, RecommendedLinkContextItem (VE's transient context) passes its this to RecommendedLinkPermanentContextItem
  • RecommendedLinkPermanentContextItem keeps track of RecommendedLinkContextItem's context for the currently opened link recommendation (this will be used to update the annotations and to navigate to other recommendations) and renders the content specific to the current recommendation.
  • Override (no-op) RecommendedLinkContextItem's setup method to not render anything
  • When a new recommendation is selected, the transient context item notifies the permanent one and passes its context. RecommendedLinkPermanentContextItem renders recommendation-specific content and updates button states in accordance with the current recommendation's states.
  • Since this context item stays open at all times, when the user focuses outside the annotation, it still needs to know what the annotation data model is.

Pros: Don't need to re-implement existing VE functionality; the permanent context item is only for rendering only

Cons: Heavily rely on the existing transient context item so there is some redundancy

You could also try adding-to and reusing ve.ui.ToolbarDialogs such as are used by find-and-replace. Currently they only support 'above' and 'side' positioning, but that could be extended. I used a toolbar dialog when we did some work on link suggestions back in 2015: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/257040/11/modules/ve-linktool/ve.ui.MWLinkSuggestionDialog.js

Change 682204 had a related patch set uploaded (by MewOphaswongse; author: MewOphaswongse):

[mediawiki/extensions/GrowthExperiments@master] [WIP] Add a link: use a permanent context for link inspector

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

I posted a WIP patch with RecommendedLinkToolbarDialog (extending ve.ui.ToolbarDialog) with the following existing functionalities:

  • accepting/rejecting recommendations
  • auto advancing upon acceptance/rejection
  • updating the UI based on current selection
  • showing link preview
  • navigating through recommendations

The remaining bulk of the work is to position the context item inline (desktop) and at the bottom of the page (mobile).

Side note: this implementation also happens to address T279493: Add a link: clicking on robot icon on the annotation doesn't bring up link inspector.

Prototype:

kostajh renamed this task from Add a link in VE: consider using a permanent context instead of VE's built-in context to Add a link in VE: Use a permanent context instead of VE's built-in context.Apr 26 2021, 7:36 AM
kostajh raised the priority of this task from Medium to High.Apr 26 2021, 9:39 AM
kostajh moved this task from April 19 - April 23 to April 26 - April 30 on the Add-Link board.

I posted a WIP patch with RecommendedLinkToolbarDialog (extending ve.ui.ToolbarDialog) with the following existing functionalities:

  • accepting/rejecting recommendations
  • auto advancing upon acceptance/rejection
  • updating the UI based on current selection
  • showing link preview
  • navigating through recommendations

The remaining bulk of the work is to position the context item inline (desktop) and at the bottom of the page (mobile).

Side note: this implementation also happens to address T279493: Add a link: clicking on robot icon on the annotation doesn't bring up link inspector.

Prototype:

Nice work @mewoph!

The positioning has been implemented in my latest patchset. For desktop, one difference from the original context item is that the link inspector is now aligned with the annotation rather than positioned based on the position of the cursor. I was unable to re-use existing positioning logic from ve.ui.DesktopContext and this was a simpler implementation. The positioning behavior is as follows:

The link inspector is aligned to the start of the annotation by default.

Screen Shot 2021-04-27 at 9.19.32 AM.png (554×856 px, 93 KB)

The link inspector is aligned to the end of the annotation if the default position would overflow past the article's edge.

Screen Shot 2021-04-27 at 9.19.26 AM.png (604×850 px, 98 KB)

@mewoph you mentioned a follow-up item about re-organizing the CSS, do you want to do that as part of this work or in a separate task?

Change 682204 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Add a link: use a permanent context for link inspector

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

@kostajh I'll do that as part of this work. Will also include the follow up changes from your comments

Change 683048 had a related patch set uploaded (by MewOphaswongse; author: MewOphaswongse):

[mediawiki/extensions/GrowthExperiments@master] Add a link: follow-up changes for permanent context item

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

@Etonkovidova @RHo this could be QA'ed now. Sorry for the extra work but in QA / Design Review you might want to look over the items from T269644: Add a link: link inspector to double-check that everything is as you've intended, as a bunch of code was shifted around for this change.

Change 683048 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Add a link: follow-up changes for permanent context item

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

A side effect of this is that the context dialog does not open anymore if one clicks on a suggested link. Closing the dialog is not easy but possible (e.g. on desktop, click outside the surface, then press Esc).

Another side effect is that there is no easy way to abandon editing - previously you could press Esc to close the context dialog and then exit VE, but now that doesn't work, I assume because the dialog needs to suppress it. One can always just navigate away, but that's maybe less intuitive and leaves VE a draft edit behind.

Not sure if it's related, but since this got merged I can't reproduce T267690: Add a link in VE: don't write to or read from restored edits anymore. (That task was closed a while ago but the bug definitely existed afterwards.)

A side effect of this is that the context dialog does not open anymore if one clicks on a suggested link. Closing the dialog is not easy but possible (e.g. on desktop, click outside the surface, then press Esc).

Another side effect is that there is no easy way to abandon editing - previously you could press Esc to close the context dialog and then exit VE, but now that doesn't work, I assume because the dialog needs to suppress it. One can always just navigate away, but that's maybe less intuitive and leaves VE a draft edit behind.

Not sure if it's related, but since this got merged I can't reproduce T267690: Add a link in VE: don't write to or read from restored edits anymore. (That task was closed a while ago but the bug definitely existed afterwards.)

Not being able to close/dismiss the context dialog does seem as a limitation to me. Ideally, there should be a logical way to switch to editing modes (VE or source editor).

I filed T282509: Add link: clicking Esc closes context dialog