Merge branch '338937-content-editor-warning' into 'master'

Show warning for markdown structure changes

See merge request gitlab-org/gitlab!71064

app/assets/javascripts/content_editor/components/content_editor.vue

@@ -3,7 +3,7 @@ import { GlLoadingIcon } from '@gitlab/ui';
 import { EditorContent as TiptapEditorContent } from '@tiptap/vue-2';
 import { LOADING_CONTENT_EVENT, LOADING_SUCCESS_EVENT, LOADING_ERROR_EVENT } from '../constants';
 import { createContentEditor } from '../services/create_content_editor';
-import ContentEditorError from './content_editor_error.vue';
+import ContentEditorAlert from './content_editor_alert.vue';
 import ContentEditorProvider from './content_editor_provider.vue';
 import EditorStateObserver from './editor_state_observer.vue';
 import FormattingBubbleMenu from './formatting_bubble_menu.vue';
@@ -12,7 +12,7 @@ import TopToolbar from './top_toolbar.vue';
 export default {
   components: {
     GlLoadingIcon,
-    ContentEditorError,
+    ContentEditorAlert,
     ContentEditorProvider,
     TiptapEditorContent,
     TopToolbar,
@@ -92,7 +92,7 @@ export default {
   <content-editor-provider :content-editor="contentEditor">
     <div>
       <editor-state-observer @docUpdate="notifyChange" @focus="focus" @blur="blur" />
-      <content-editor-error />
+      <content-editor-alert />
       <div
         data-testid="content-editor"
         data-qa-selector="content_editor_container"

app/assets/javascripts/content_editor/components/content_editor_error.vue → app/assets/javascripts/content_editor/components/content_editor_alert.vue

@@ -9,23 +9,25 @@ export default {
   },
   data() {
     return {
-      error: null,
+      message: null,
+      variant: 'danger',
     };
   },
   methods: {
-    displayError({ error }) {
-      this.error = error;
+    displayAlert({ message, variant }) {
+      this.message = message;
+      this.variant = variant;
     },
-    dismissError() {
-      this.error = null;
+    dismissAlert() {
+      this.message = null;
     },
   },
 };
 </script>
 <template>
-  <editor-state-observer @error="displayError">
-    <gl-alert v-if="error" class="gl-mb-6" variant="danger" @dismiss="dismissError">
-      {{ error }}
+  <editor-state-observer @alert="displayAlert">
+    <gl-alert v-if="message" class="gl-mb-6" :variant="variant" @dismiss="dismissAlert">
+      {{ message }}
     </gl-alert>
   </editor-state-observer>
 </template>

app/assets/javascripts/content_editor/components/editor_state_observer.vue

@@ -7,7 +7,7 @@ export const tiptapToComponentMap = {
   transaction: 'transaction',
   focus: 'focus',
   blur: 'blur',
-  error: 'error',
+  alert: 'alert',
 };
 
 const getComponentEventName = (tiptapEventName) => tiptapToComponentMap[tiptapEventName];

app/assets/javascripts/content_editor/components/wrappers/table_cell_base.vue

@@ -26,8 +26,8 @@ export default {
       type: Object,
       required: true,
     },
-    getPos: {
-      type: Function,
+    node: {
+      type: Object,
       required: true,
     },
   },
@@ -61,7 +61,17 @@ export default {
       const { state } = this.editor;
       const { $cursor } = state.selection;
 
-      this.displayActionsDropdown = $cursor?.pos - $cursor?.parentOffset - 1 === this.getPos();
+      if (!$cursor) return;
+
+      this.displayActionsDropdown = false;
+
+      for (let level = 0; level < $cursor.depth; level += 1) {
+        if ($cursor.node(level) === this.node) {
+          this.displayActionsDropdown = true;
+          break;
+        }
+      }
+
       if (this.displayActionsDropdown) {
         this.selectedRect = getSelectedRect(state);
       }
@@ -99,7 +109,11 @@ export default {
     :as="cellType"
     @click="hideDropdown"
   >
-    <span v-if="displayActionsDropdown" class="gl-absolute gl-right-0 gl-top-0">
+    <span
+      v-if="displayActionsDropdown"
+      contenteditable="false"
+      class="gl-absolute gl-right-0 gl-top-0"
+    >
       <gl-dropdown
         ref="dropdown"
         dropup

app/assets/javascripts/content_editor/components/wrappers/table_cell_body.vue

@@ -11,8 +11,8 @@ export default {
       type: Object,
       required: true,
     },
-    getPos: {
-      type: Function,
+    node: {
+      type: Object,
       required: true,
     },
   },

app/assets/javascripts/content_editor/components/wrappers/table_cell_header.vue

@@ -11,8 +11,8 @@ export default {
       type: Object,
       required: true,
     },
-    getPos: {
-      type: Function,
+    node: {
+      type: Object,
       required: true,
     },
   },

app/assets/javascripts/content_editor/extensions/table.js

-export { Table as default } from '@tiptap/extension-table';
+import { Table } from '@tiptap/extension-table';
+import { debounce } from 'lodash';
+import { __ } from '~/locale';
+import { getMarkdownSource } from '../services/markdown_sourcemap';
+import { shouldRenderHTMLTable } from '../services/serialization_helpers';
+
+let alertShown = false;
+const onUpdate = debounce((editor) => {
+  if (alertShown) return;
+
+  editor.state.doc.descendants((node) => {
+    if (node.type.name === 'table' && node.attrs.isMarkdown && shouldRenderHTMLTable(node)) {
+      editor.emit('alert', {
+        message: __(
+          'The content editor may change the markdown formatting style of the document, which may not match your original markdown style.',
+        ),
+        variant: 'warning',
+      });
+
+      alertShown = true;
+
+      return false;
+    }
+
+    return true;
+  });
+}, 1000);
+
+export default Table.extend({
+  addAttributes() {
+    return {
+      isMarkdown: {
+        default: null,
+        parseHTML: (element) => Boolean(getMarkdownSource(element)),
+      },
+    };
+  },
+
+  onUpdate({ editor }) {
+    onUpdate(editor);
+  },
+});

app/assets/javascripts/content_editor/extensions/table_cell.js

 import { TableCell } from '@tiptap/extension-table-cell';
 import { VueNodeViewRenderer } from '@tiptap/vue-2';
 import TableCellBodyWrapper from '../components/wrappers/table_cell_body.vue';
-import { isBlockTablesFeatureEnabled } from '../services/feature_flags';
 
 export default TableCell.extend({
-  content: isBlockTablesFeatureEnabled() ? 'block+' : 'inline*',
+  content: 'block+',
 
   addNodeView() {
     return VueNodeViewRenderer(TableCellBodyWrapper);

app/assets/javascripts/content_editor/extensions/table_header.js

 import { TableHeader } from '@tiptap/extension-table-header';
 import { VueNodeViewRenderer } from '@tiptap/vue-2';
 import TableCellHeaderWrapper from '../components/wrappers/table_cell_header.vue';
-import { isBlockTablesFeatureEnabled } from '../services/feature_flags';
 
 export default TableHeader.extend({
-  content: isBlockTablesFeatureEnabled() ? 'block+' : 'inline*',
+  content: 'block+',
   addNodeView() {
     return VueNodeViewRenderer(TableCellHeaderWrapper);
   },

app/assets/javascripts/content_editor/services/feature_flags.js

-export function isBlockTablesFeatureEnabled() {
-  return gon.features?.contentEditorBlockTables;
-}

app/assets/javascripts/content_editor/services/serialization_helpers.js

 import { uniq } from 'lodash';
-import { isBlockTablesFeatureEnabled } from './feature_flags';
 
 const defaultAttrs = {
   td: { colspan: 1, rowspan: 1, colwidth: null },
@@ -75,7 +74,7 @@ function getChildren(node) {
   return children;
 }
 
-function shouldRenderHTMLTable(table) {
+export function shouldRenderHTMLTable(table) {
   const { rows, cells } = getRowsAndCells(table);
 
   const cellChildCount = Math.max(...cells.map((cell) => cell.childCount));
@@ -282,11 +281,6 @@ export function renderOrderedList(state, node) {
 }
 
 export function renderTableCell(state, node) {
-  if (!isBlockTablesFeatureEnabled()) {
-    state.renderInline(node);
-    return;
-  }
-
   if (!isInBlockTable(node) || containsParagraphWithOnlyText(node)) {
     state.renderInline(node.child(0));
   } else {
@@ -303,9 +297,7 @@ export function renderTableRow(state, node) {
 }
 
 export function renderTable(state, node) {
-  if (isBlockTablesFeatureEnabled()) {
-    setIsInBlockTable(node, shouldRenderHTMLTable(node));
-  }
+  setIsInBlockTable(node, shouldRenderHTMLTable(node));
 
   if (isInBlockTable(node)) renderTagOpen(state, 'table');
 
@@ -317,9 +309,7 @@ export function renderTable(state, node) {
   state.closeBlock(node);
   state.flushClose();
 
-  if (isBlockTablesFeatureEnabled()) {
-    unsetIsInBlockTable(node);
-  }
+  unsetIsInBlockTable(node);
 }
 
 export function renderHardBreak(state, node, parent, index) {

app/assets/javascripts/content_editor/services/upload_helpers.js

@@ -72,8 +72,9 @@ const uploadImage = async ({ editor, file, uploadsPath, renderMarkdown }) => {
     );
   } catch (e) {
     editor.commands.deleteRange({ from: position, to: position + 1 });
-    editor.emit('error', {
-      error: __('An error occurred while uploading the image. Please try again.'),
+    editor.emit('alert', {
+      message: __('An error occurred while uploading the image. Please try again.'),
+      variant: 'danger',
     });
   }
 };
@@ -102,8 +103,9 @@ const uploadAttachment = async ({ editor, file, uploadsPath, renderMarkdown }) =
     );
   } catch (e) {
     editor.commands.deleteRange({ from, to: from + 1 });
-    editor.emit('error', {
-      error: __('An error occurred while uploading the file. Please try again.'),
+    editor.emit('alert', {
+      message: __('An error occurred while uploading the file. Please try again.'),
+      variant: 'danger',
     });
   }
 };

app/controllers/projects/wikis_controller.rb

@@ -7,9 +7,5 @@ class Projects::WikisController < Projects::ApplicationController
 
   alias_method :container, :project
 
-  before_action do
-    push_frontend_feature_flag(:content_editor_block_tables, @project, default_enabled: :yaml)
-  end
-
   feature_category :wiki
 end

config/feature_flags/development/content_editor_block_tables.yml

----
-name: content_editor_block_tables
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66187
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/338937
-milestone: '14.3'
-type: development
-group: group::editor
-default_enabled: false

locale/gitlab.pot

@@ -34366,6 +34366,9 @@ msgstr ""
 msgid "The contact does not belong to the same group as the issue"
 msgstr ""
 
+msgid "The content editor may change the markdown formatting style of the document, which may not match your original markdown style."
+msgstr ""
+
 msgid "The content of this page is not encoded in UTF-8. Edits can only be made via the Git repository."
 msgstr ""
 

spec/frontend/content_editor/components/content_editor_error_spec.js → spec/frontend/content_editor/components/content_editor_alert_spec.js

 import { GlAlert } from '@gitlab/ui';
 import { nextTick } from 'vue';
 import { shallowMountExtended } from 'helpers/vue_test_utils_helper';
-import ContentEditorError from '~/content_editor/components/content_editor_error.vue';
+import ContentEditorAlert from '~/content_editor/components/content_editor_alert.vue';
 import EditorStateObserver from '~/content_editor/components/editor_state_observer.vue';
 import { createTestEditor, emitEditorEvent } from '../test_utils';
 
-describe('content_editor/components/content_editor_error', () => {
+describe('content_editor/components/content_editor_alert', () => {
   let wrapper;
   let tiptapEditor;
 
@@ -14,7 +14,7 @@ describe('content_editor/components/content_editor_error', () => {
   const createWrapper = async () => {
     tiptapEditor = createTestEditor();
 
-    wrapper = shallowMountExtended(ContentEditorError, {
+    wrapper = shallowMountExtended(ContentEditorAlert, {
       provide: {
         tiptapEditor,
       },
@@ -28,22 +28,28 @@ describe('content_editor/components/content_editor_error', () => {
     wrapper.destroy();
   });
 
-  it('renders error when content editor emits an error event', async () => {
-    const error = 'error message';
+  it.each`
+    variant      | message
+    ${'danger'}  | ${'An error occurred'}
+    ${'warning'} | ${'A warning'}
+  `(
+    'renders error when content editor emits an error event for variant: $variant',
+    async ({ message, variant }) => {
+      createWrapper();
 
-    createWrapper();
-
-    await emitEditorEvent({ tiptapEditor, event: 'error', params: { error } });
+      await emitEditorEvent({ tiptapEditor, event: 'alert', params: { message, variant } });
 
-    expect(findErrorAlert().text()).toBe(error);
-  });
+      expect(findErrorAlert().text()).toBe(message);
+      expect(findErrorAlert().attributes().variant).toBe(variant);
+    },
+  );
 
   it('allows dismissing the error', async () => {
-    const error = 'error message';
+    const message = 'error message';
 
     createWrapper();
 
-    await emitEditorEvent({ tiptapEditor, event: 'error', params: { error } });
+    await emitEditorEvent({ tiptapEditor, event: 'alert', params: { message } });
 
     findErrorAlert().vm.$emit('dismiss');
 

spec/frontend/content_editor/components/content_editor_spec.js

@@ -3,7 +3,7 @@ import { EditorContent } from '@tiptap/vue-2';
 import { nextTick } from 'vue';
 import { shallowMountExtended } from 'helpers/vue_test_utils_helper';
 import ContentEditor from '~/content_editor/components/content_editor.vue';
-import ContentEditorError from '~/content_editor/components/content_editor_error.vue';
+import ContentEditorAlert from '~/content_editor/components/content_editor_alert.vue';
 import ContentEditorProvider from '~/content_editor/components/content_editor_provider.vue';
 import EditorStateObserver from '~/content_editor/components/editor_state_observer.vue';
 import FormattingBubbleMenu from '~/content_editor/components/formatting_bubble_menu.vue';
@@ -111,10 +111,10 @@ describe('ContentEditor', () => {
     ]);
   });
 
-  it('renders content_editor_error component', () => {
+  it('renders content_editor_alert component', () => {
     createWrapper();
 
-    expect(wrapper.findComponent(ContentEditorError).exists()).toBe(true);
+    expect(wrapper.findComponent(ContentEditorAlert).exists()).toBe(true);
   });
 
   describe('when loading content', () => {

spec/frontend/content_editor/components/wrappers/table_cell_base_spec.js

@@ -11,13 +11,13 @@ jest.mock('prosemirror-tables');
 describe('content/components/wrappers/table_cell_base', () => {
   let wrapper;
   let editor;
-  let getPos;
+  let node;
 
   const createWrapper = async (propsData = { cellType: 'td' }) => {
     wrapper = shallowMountExtended(TableCellBaseWrapper, {
       propsData: {
         editor,
-        getPos,
+        node,
         ...propsData,
       },
     });
@@ -36,7 +36,7 @@ describe('content/components/wrappers/table_cell_base', () => {
   const setCurrentPositionInCell = () => {
     const { $cursor } = editor.state.selection;
 
-    getPos.mockReturnValue($cursor.pos - $cursor.parentOffset - 1);
+    jest.spyOn($cursor, 'node').mockReturnValue(node);
   };
   const mockDropdownHide = () => {
     /*
@@ -48,7 +48,7 @@ describe('content/components/wrappers/table_cell_base', () => {
   };
 
   beforeEach(() => {
-    getPos = jest.fn();
+    node = {};
     editor = createTestEditor({});
   });
 

spec/frontend/content_editor/components/wrappers/table_cell_body_spec.js

@@ -6,19 +6,19 @@ import { createTestEditor } from '../../test_utils';
 describe('content/components/wrappers/table_cell_body', () => {
   let wrapper;
   let editor;
-  let getPos;
+  let node;
 
   const createWrapper = async () => {
     wrapper = shallowMount(TableCellBodyWrapper, {
       propsData: {
         editor,
-        getPos,
+        node,
       },
     });
   };
 
   beforeEach(() => {
-    getPos = jest.fn();
+    node = {};
     editor = createTestEditor({});
   });
 
@@ -30,7 +30,7 @@ describe('content/components/wrappers/table_cell_body', () => {
     createWrapper();
     expect(wrapper.findComponent(TableCellBaseWrapper).props()).toEqual({
       editor,
-      getPos,
+      node,
       cellType: 'td',
     });
   });

spec/frontend/content_editor/components/wrappers/table_cell_header_spec.js

@@ -6,19 +6,19 @@ import { createTestEditor } from '../../test_utils';
 describe('content/components/wrappers/table_cell_header', () => {
   let wrapper;
   let editor;
-  let getPos;
+  let node;
 
   const createWrapper = async () => {
     wrapper = shallowMount(TableCellHeaderWrapper, {
       propsData: {
         editor,
-        getPos,
+        node,
       },
     });
   };
 
   beforeEach(() => {
-    getPos = jest.fn();
+    node = {};
     editor = createTestEditor({});
   });
 
@@ -30,7 +30,7 @@ describe('content/components/wrappers/table_cell_header', () => {
     createWrapper();
     expect(wrapper.findComponent(TableCellBaseWrapper).props()).toEqual({
       editor,
-      getPos,
+      node,
       cellType: 'th',
     });
   });

spec/frontend/content_editor/extensions/attachment_spec.js

@@ -157,11 +157,11 @@ describe('content_editor/extensions/attachment', () => {
           });
         });
 
-        it('emits an error event that includes an error message', (done) => {
+        it('emits an alert event that includes an error message', (done) => {
           tiptapEditor.commands.uploadAttachment({ file: imageFile });
 
-          tiptapEditor.on('error', ({ error }) => {
-            expect(error).toBe('An error occurred while uploading the image. Please try again.');
+          tiptapEditor.on('alert', ({ message }) => {
+            expect(message).toBe('An error occurred while uploading the image. Please try again.');
             done();
           });
         });
@@ -233,11 +233,11 @@ describe('content_editor/extensions/attachment', () => {
           });
         });
 
-        it('emits an error event that includes an error message', (done) => {
+        it('emits an alert event that includes an error message', (done) => {
           tiptapEditor.commands.uploadAttachment({ file: attachmentFile });
 
-          tiptapEditor.on('error', ({ error }) => {
-            expect(error).toBe('An error occurred while uploading the file. Please try again.');
+          tiptapEditor.on('alert', ({ message }) => {
+            expect(message).toBe('An error occurred while uploading the file. Please try again.');
             done();
           });
         });

spec/frontend/content_editor/extensions/table_spec.js

+import Bold from '~/content_editor/extensions/bold';
+import BulletList from '~/content_editor/extensions/bullet_list';
+import ListItem from '~/content_editor/extensions/list_item';
+import Table from '~/content_editor/extensions/table';
+import TableCell from '~/content_editor/extensions/table_cell';
+import TableRow from '~/content_editor/extensions/table_row';
+import TableHeader from '~/content_editor/extensions/table_header';
+import { createTestEditor, createDocBuilder } from '../test_utils';
+
+describe('content_editor/extensions/table', () => {
+  let tiptapEditor;
+  let doc;
+  let p;
+  let table;
+  let tableHeader;
+  let tableCell;
+  let tableRow;
+  let initialDoc;
+  let mockAlert;
+
+  beforeEach(() => {
+    tiptapEditor = createTestEditor({
+      extensions: [Table, TableCell, TableRow, TableHeader, BulletList, Bold, ListItem],
+    });
+
+    ({
+      builders: { doc, p, table, tableCell, tableHeader, tableRow },
+    } = createDocBuilder({
+      tiptapEditor,
+      names: {
+        bold: { markType: Bold.name },
+        table: { nodeType: Table.name },
+        tableHeader: { nodeType: TableHeader.name },
+        tableCell: { nodeType: TableCell.name },
+        tableRow: { nodeType: TableRow.name },
+        bulletList: { nodeType: BulletList.name },
+        listItem: { nodeType: ListItem.name },
+      },
+    }));
+
+    initialDoc = doc(
+      table(
+        { isMarkdown: true },
+        tableRow(tableHeader(p('This is')), tableHeader(p('a table'))),
+        tableRow(tableCell(p('this is')), tableCell(p('the first row'))),
+      ),
+    );
+
+    mockAlert = jest.fn();
+  });
+
+  it('triggers a warning (just once) if the table is markdown, but the changes in the document will render an HTML table instead', () => {
+    tiptapEditor.commands.setContent(initialDoc.toJSON());
+
+    tiptapEditor.on('alert', mockAlert);
+
+    tiptapEditor.commands.setTextSelection({ from: 20, to: 22 });
+    tiptapEditor.commands.toggleBulletList();
+
+    jest.advanceTimersByTime(1001);
+    expect(mockAlert).toHaveBeenCalled();
+
+    mockAlert.mockReset();
+
+    tiptapEditor.commands.setTextSelection({ from: 4, to: 6 });
+    tiptapEditor.commands.toggleBulletList();
+
+    jest.advanceTimersByTime(1001);
+    expect(mockAlert).not.toHaveBeenCalled();
+  });
+
+  it('does not trigger a warning if the table is markdown, and the changes in the document can generate a markdown table', () => {
+    tiptapEditor.commands.setContent(initialDoc.toJSON());
+
+    tiptapEditor.on('alert', mockAlert);
+
+    tiptapEditor.commands.setTextSelection({ from: 20, to: 22 });
+    tiptapEditor.commands.toggleBold();
+
+    jest.advanceTimersByTime(1001);
+    expect(mockAlert).not.toHaveBeenCalled();
+  });
+
+  it('does not trigger any warnings if the table is not markdown', () => {
+    initialDoc = doc(
+      table(
+        tableRow(tableHeader(p('This is')), tableHeader(p('a table'))),
+        tableRow(tableCell(p('this is')), tableCell(p('the first row'))),
+      ),
+    );
+
+    tiptapEditor.commands.setContent(initialDoc.toJSON());
+
+    tiptapEditor.on('alert', mockAlert);
+
+    tiptapEditor.commands.setTextSelection({ from: 20, to: 22 });
+    tiptapEditor.commands.toggleBulletList();
+
+    jest.advanceTimersByTime(1001);
+    expect(mockAlert).not.toHaveBeenCalled();
+  });
+});

spec/frontend/content_editor/services/markdown_serializer_spec.js

@@ -34,10 +34,6 @@ import { createTestEditor, createDocBuilder } from '../test_utils';
 
 jest.mock('~/emoji');
 
-jest.mock('~/content_editor/services/feature_flags', () => ({
-  isBlockTablesFeatureEnabled: jest.fn().mockReturnValue(true),
-}));
-
 const tiptapEditor = createTestEditor({
   extensions: [
     Blockquote,