Page MenuHomePhabricator

CX2: Rich text editing in dialogs is not possible since cards are shown in a deactivated tools column
Closed, ResolvedPublic

Description

Can't add a link in a basic reference dialog, because the inspector opens on the right side of the page which is deactivated when the primary dialog is open

Steps to replicate:

  1. Open the Basic reference dialog
  2. Click on the link inspector icon in the toolbar

Observe that, the link inspector is opening in the background on the right side of the page which is deactivated/not accessible when the primary dialog is open therefore making it unusable.

Screen Shot 2018-05-01 at 3.51.35 PM.png (583×1 px, 159 KB)

Proposed solution

We may want to keep the tools column both (a) usable to access relevant cards for the content in the dialog and (b) focused on the relevant tools (e.g., not showing two identical editing toolbars in different places). In order to achieve this, the following is proposed:

  • Do not disable the tools column. Restrict the dialog overlay to only cover the document, not the whole screen (alternatively, moving the overlay an disable the document similarly to what is done when publishing).
  • Remove all cards except for the one being edited, and show such card as disabled (disabled button and 75% opacity).
  • Show new cards as needed in the tools column as the user interacts with the dialog.

An example below illustrates the idea:

CX-refs.png (720×1 px, 242 KB)
CX-dialog-rich-edit.png (720×1 px, 174 KB)
CX-dialog-rich-edit-card.png (720×1 px, 179 KB)

In the example, the user selects a reference and edits it by using the "edit" action in the card. As a result, a dialog appears, and only the reference card remains (as disabled) on the tools column. When the user clicks on the link from the rich text field of the dialog, a link card appears in the tools column that can be operated in the usual way.

Event Timeline

Pginer-WMF renamed this task from Can't add a link in a basic reference dialog, because the nested dialog opens on the left side of the page which is deactivated when the primary dialog is open to CX2: Rich text editing in dialogs is not possible since cards are shown in a deactivated tools column.May 3 2018, 1:58 PM
Pginer-WMF updated the task description. (Show Details)
Pginer-WMF moved this task from Needs Triage to CX2 on the ContentTranslation board.

If we constrain the overlay to cover the source and translation columns only, we can get the tools column interactable. Following patch can add a custom css class to the overlay, allowing to set the required dimensions. It will require adjustment as window is resized.

diff --git a/modules/ve-cx/ui/ve.ui.CXSurface.js b/modules/ve-cx/ui/ve.ui.CXSurface.js
index d7a34543..10c3bb8c 100644
--- a/modules/ve-cx/ui/ve.ui.CXSurface.js
+++ b/modules/ve-cx/ui/ve.ui.CXSurface.js
@@ -15,6 +15,9 @@ ve.ui.CXSurface = function VeUiCXSurface( dataOrDoc, toolsColumn, config ) {
        this.toolsContainer = toolsColumn.toolContainer;
        // Move context to toolsContainer
        this.toolsContainer.$element.append( this.context.$element );
+
+       this.globalOverlay.$element.addClass( 've-cx-ui-overlay-global' );
+
        this.getView().connect( this, {
                focus: 'onFocus',
                blur: 'onBlur'

If we constrain the overlay to cover the source and translation columns only, we can get the tools column interactable.

Just one consideration: although in many cases the user may have scrolled the document already, it is possible for this to happen with the viewport at the very top. In such case, we may want to adjust the overlay top to cover only the document (and not the grey header where "Wikipedia Translations" can be read).

Change 437445 had a related patch set uploaded (by Petar.petkovic; owner: Petar.petkovic):
[mediawiki/extensions/ContentTranslation@master] Enable complex editing for reference dialog

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

Here is the state with the change (patchset 5), where nesting of complex dialogs is shown. Image caption has a reference which has a template:

cx2-complex-dialog-437445.gif (932×1 px, 3 MB)

There is a small scrolling happening when user opens link card, which is unrelated to this patch.

Here is the state with the change (patchset 5), where nesting of complex dialogs is shown. Image caption has a reference which has a template

Great progress @Petar.petkovic!
Some comments about a couple details:

  1. The card for the element that is being edited is currently shown as enabled (with the "edit" button active). The idea was to show those as disabled. This was captured in the ticket description, so maybe you already considered this for an upcoming iteration.
  2. Regarding recursive cases, I'm more inclined to keep visible only the relevant cards for the current dialog, rather than keep accumulating them as shown in the gif. That is, in your example the "image" card will hide after clicking the "edit" button of the "reference" card. We can discuss if there is a significant additional technical complexity to support this since the current behaviour is not bad, and I don't expect this recursion to go too many levels deep in practice.

Some comments about a couple details:

  1. The card for the element that is being edited is currently shown as enabled (with the "edit" button active). The idea was to show those as disabled. This was captured in the ticket description, so maybe you already considered this for an upcoming iteration.
  2. Regarding recursive cases, I'm more inclined to keep visible only the relevant cards for the current dialog, rather than keep accumulating them as shown in the gif. That is, in your example the "image" card will hide after clicking the "edit" button of the "reference" card. We can discuss if there is a significant additional technical complexity to support this since the current behaviour is not bad, and I don't expect this recursion to go too many levels deep in practice.

The reason I didn't disable the card for element that is being edited is that clicking again on "Edit" does nothing. It can be done in next iteration.

Looking at F19012641, I feel like having all action items on the side bar is strange. I would have expected to have the same behavior as on VE when I edit a regular page.

Looking at F19012641, I feel like having all action items on the side bar is strange. I would have expected to have the same behavior as on VE when I edit a regular page.

That's the first option we explored to keep the information inside the dialog. However, technically it was problematic to provide the information in two different ways depending on the context. So for now, the main goal is to make the cards reachable on an active sidebar and clear the irrelevant cards to enable the access and minimize the confusion.

Got it. I think the best is to make some tests with users, like you mentioned it in yesterday's meeting.

I really don't like the proposed solution here. It breaks the modality of dialogs, and will lead to more problems down the line, e.g. narrower screens (mobile/tablet), wider dialogs (image dialog) or sidebar buttons being clickable when they should be obscured by the modal (causing unexpected action sequences).

I doubt this will be the last issue this causes.

The most sensible option would be to move the regular editing inspectors back inline (in practice this will usually just be link/cite/images). It should still be possible to keep the sidebar for translation-specific contexts that display more regularly than the classic editing inspectors, e.g. the plain-text link suggestion context, and the paragraph-level translation source menu. Adding an extra window manager, and registering CX-specific windows to appear in it will be much less problematic.

Let me comment on the specific issue of modality. There are definitely parts of the VE code that assume dialogs operating on a portion of the document must be modal. One example is that there's a transaction undo stack that mirrors the dialog opening stack. Another is that there are places where stored offsets could be invalidated if the main document remains editable while the dialog is visible.

So whatever UI solution you choose, I would caution you to preserve the effective modality of the dialog (including immutability of the rest of the document by toolbar actions), else you're liable to experience some subtle and complex data corruption bugs.

As a very active user of VE, I'm still super confused by the location of the dialogs. :/

Thanks for the input. This represents an initial step where we try to accomplish the main goals of (a) allowing people to access the tools they need, and (b) prevent from accessing other tools that would modify the underlying document (related to David's comment).

There is still a disconnect between the dialogs acting as a container and the tools provided next to them. We'll capture ways to improve on that front as follow-up tickets.

Change 437445 merged by jenkins-bot:
[mediawiki/extensions/ContentTranslation@master] Prevent complex editing dialogs from covering editing tools

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

There are also serious accessibility issues with breaking the modality of dialog. OOUI adds aria-hidden attributes to elements not in the dialog, and we go to great lengths to prevent the user from accessing elements beneath the dialog using the keyboard (something they would actually want to do now). @Volker_E and @matmarex may have some thoughts on this.

Having read the code in a bit more detail, I would also like to highlight the volume and complexity of the code required to support this hack, it is likely going to cause lots of maintenance problems when upstream changes are made to VE contexts, or OOUI dialogs.

Again, I strongly recommend not merging this patch and reconsidering your approach.

Vvjjkkii renamed this task from CX2: Rich text editing in dialogs is not possible since cards are shown in a deactivated tools column to wtdaaaaaaa.Jul 1 2018, 1:12 AM
Vvjjkkii removed Petar.petkovic as the assignee of this task.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.

There are also serious accessibility issues with breaking the modality of dialog. OOUI adds aria-hidden attributes to elements not in the dialog, and we go to great lengths to prevent the user from accessing elements beneath the dialog using the keyboard (something they would actually want to do now). @Volker_E and @matmarex may have some thoughts on this.

Accessibility is an important aspect for us. Do you have an example of such problems, with steps to reproduce, so that we could take a closer look? I have been doing some tests but I only found a potential issue with dialogs (T198615), but that does not seem to be CX-specific.

Having read the code in a bit more detail, I would also like to highlight the volume and complexity of the code required to support this hack, it is likely going to cause lots of maintenance problems when upstream changes are made to VE contexts, or OOUI dialogs.
Again, I strongly recommend not merging this patch and reconsidering your approach.

Content Translation supports the translation process, which has both commonalities and particularities with respect to other kinds of contribution such as editing. One of the particularities of translation is the need for context. In order to provide a good translation users may need to check different pieces of information to decide how the original idea is better represented in the target language. For example, in Content Translation, you can select a word and get (a) the Wikipedia article for such word (to explore it and learn more about the topic and quickly add it as a link), (b) dictionary definitions for the word, and (c) alternative translations for the word to quickly replace it. This rich context appears on a separate tools sidebar to allow users to access it when it’s needed, but avoid getting in the way of editing the content when this context is not needed.

This is a departure from Visual Editor popups to avoid showing three of them hiding the content in the example above. In order to minimize the maintenance cost we tried to keep the interventions minimal. While the inspectors are repositioned to support the tools sidebar concept, the user interface that each inspector provides is aligned with Visual Editor, to minimize the impact of future changes.

For dialogs, we are trying to find a similar balance where dialogs can adapt to a different context as they do on mobile. The short term goal is to make it possible to support the scenario described in the ticket, but we are open to explore long-term solutions once the specific issues are identified.

CommunityTechBot renamed this task from wtdaaaaaaa to CX2: Rich text editing in dialogs is not possible since cards are shown in a deactivated tools column.Jul 2 2018, 1:45 PM
CommunityTechBot assigned this task to Petar.petkovic.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.

Accessibility is an important aspect for us. Do you have an example of such problems, with steps to reproduce, so that we could take a closer look? I have been doing some tests but I only found a potential issue with dialogs (T198615), but that does not seem to be CX-specific.

We are now setting aria-disabled on items which aren't disabled (someone more familiar with accessibility tools will know the details on how this affects users), and there are gaps in the modal overlay allowing the user to focus the document behind, causing the document to scroll, or the edit area to be interacted with. All of these are because the modal dialog code assumes complete modality, which has now been broken.

This is a departure from Visual Editor popups to avoid showing three of them hiding the content in the example above. In order to minimize the maintenance cost we tried to keep the interventions minimal. While the inspectors are repositioned to support the tools sidebar concept, the user interface that each inspector provides is aligned with Visual Editor, to minimize the impact of future changes.

I would not consider this intervention minimal, and am increasingly concerned about the maintenance cost (especially as there are still more issues to fix, and more code to write to support this). If the concern contexts being too largeit there is more information in the future (e.g. dictionary lookups) I'm sure there are other design approaches that can be considered that don't break so many upstream assumptions.

@Petar.petkovic - the major issue, per the title, 'Rich text editing in dialogs is not possible since cards are shown in a deactivated tools column" is fixed. However, when testing in cx2-testing, the dialog window takes the whole left column when open:
The image is clicked and the 'Edit' option becomes active:

Screen Shot 2018-07-03 at 3.24.31 PM.png (635×1 px, 240 KB)

Then, the "Edit" option is clicked and the 'Media settings' window takes the left column entirely:

Screen Shot 2018-07-03 at 3.24.45 PM.png (659×1 px, 68 KB)

I can file it as a separate issue, just let me know.

@Petar.petkovic - the major issue, per the title, 'Rich text editing in dialogs is not possible since cards are shown in a deactivated tools column" is fixed. However, when testing in cx2-testing, the dialog window takes the whole left column when open:
The image is clicked and the 'Edit' option becomes active:

Screen Shot 2018-07-03 at 3.24.31 PM.png (635×1 px, 240 KB)

Then, the "Edit" option is clicked and the 'Media settings' window takes the left column entirely:

Screen Shot 2018-07-03 at 3.24.45 PM.png (659×1 px, 68 KB)

I can file it as a separate issue, just let me know.

That is what T198390 actually requested :)

We can mark as resolved the current ticket (since now it is possible to edit links inside a reference, as @Etonkovidova confirmed). A separate ticket (T198390) is in charge of adjusting dialog positioning to conform with the document editing metaphor and reduce the dissonance between the information presented and the tool.
For future explorations on how to reduce the amount of customization code, and the potential issues that were mentioned in this ticket, I created T198799.

Thanks everyone involved for their input.