fix: comment position in RTL (#7934)

* fix: how comments are laid out in RTL

* fix: comment positioning with JSON

* fix: comment positioning with XML

core/comments/comment_view.ts

@@ -19,8 +19,11 @@ export class CommentView implements IRenderedElement {
   /** The root group element of the comment view. */
   private svgRoot: SVGGElement;
 
+  /** The group containing all of the top bar elements. */
+  private topBarGroup: SVGGElement;
+
   /** The rect background for the top bar. */
-  private topBar: SVGRectElement;
+  private topBarBackground: SVGRectElement;
 
   /** The delete icon that goes in the top bar. */
   private deleteIcon: SVGImageElement;
@@ -97,7 +100,8 @@ export class CommentView implements IRenderedElement {
     });
 
     ({
-      topBar: this.topBar,
+      topBarGroup: this.topBarGroup,
+      topBarBackground: this.topBarBackground,
       deleteIcon: this.deleteIcon,
       foldoutIcon: this.foldoutIcon,
       textPreview: this.textPreview,
@@ -115,6 +119,9 @@ export class CommentView implements IRenderedElement {
 
     // Set size to the default size.
     this.setSize(this.size);
+
+    // Set default transform (including inverted scale for RTL).
+    this.moveTo(new Coordinate(0, 0));
   }
 
   /**
@@ -125,21 +132,27 @@ export class CommentView implements IRenderedElement {
     svgRoot: SVGGElement,
     workspace: WorkspaceSvg,
   ): {
-    topBar: SVGRectElement;
+    topBarGroup: SVGGElement;
+    topBarBackground: SVGRectElement;
     deleteIcon: SVGImageElement;
     foldoutIcon: SVGImageElement;
     textPreview: SVGTextElement;
     textPreviewNode: Text;
   } {
-    const topBar = dom.createSvgElement(
-      Svg.RECT,
+    const topBarGroup = dom.createSvgElement(
+      Svg.G,
       {
         'class': 'blocklyCommentTopbar',
-        'x': 0,
-        'y': 0,
       },
       svgRoot,
     );
+    const topBarBackground = dom.createSvgElement(
+      Svg.RECT,
+      {
+        'class': 'blocklyCommentTopbarBackground',
+      },
+      topBarGroup,
+    );
     // TODO: Before merging, does this mean to override an individual image,
     // folks need to replace the whole media folder?
     const deleteIcon = dom.createSvgElement(
@@ -148,22 +161,22 @@ export class CommentView implements IRenderedElement {
         'class': 'blocklyDeleteIcon',
         'href': `${workspace.options.pathToMedia}delete-icon.svg`,
       },
-      svgRoot,
+      topBarGroup,
     );
     const foldoutIcon = dom.createSvgElement(
       Svg.IMAGE,
       {
         'class': 'blocklyFoldoutIcon',
         'href': `${workspace.options.pathToMedia}arrow-dropdown.svg`,
       },
-      svgRoot,
+      topBarGroup,
     );
     const textPreview = dom.createSvgElement(
       Svg.TEXT,
       {
         'class': 'blocklyCommentPreview blocklyCommentText blocklyText',
       },
-      svgRoot,
+      topBarGroup,
     );
     const textPreviewNode = document.createTextNode('');
     textPreview.appendChild(textPreviewNode);
@@ -184,7 +197,14 @@ export class CommentView implements IRenderedElement {
       this.onDeleteDown,
     );
 
-    return {topBar, deleteIcon, foldoutIcon, textPreview, textPreviewNode};
+    return {
+      topBarGroup,
+      topBarBackground,
+      deleteIcon,
+      foldoutIcon,
+      textPreview,
+      textPreviewNode,
+    };
   }
 
   /**
@@ -258,7 +278,7 @@ export class CommentView implements IRenderedElement {
    * elements to reflect the new size.
    */
   setSize(size: Size) {
-    const topBarSize = this.topBar.getBBox();
+    const topBarSize = this.topBarBackground.getBBox();
     const deleteSize = this.deleteIcon.getBBox();
     const foldoutSize = this.foldoutIcon.getBBox();
     const textPreviewSize = this.textPreview.getBBox();
@@ -274,8 +294,7 @@ export class CommentView implements IRenderedElement {
     this.svgRoot.setAttribute('height', `${size.height}`);
     this.svgRoot.setAttribute('width', `${size.width}`);
 
-    this.topBar.setAttribute('width', `${size.width}`);
-
+    this.updateTopBarSize(size);
     this.updateTextAreaSize(size, topBarSize);
     this.updateDeleteIconPosition(size, topBarSize, deleteSize);
     this.updateFoldoutIconPosition(topBarSize, foldoutSize);
@@ -286,9 +305,7 @@ export class CommentView implements IRenderedElement {
       deleteSize,
       resizeSize,
     );
-
-    this.resizeHandle.setAttribute('x', `${size.width - resizeSize.width}`);
-    this.resizeHandle.setAttribute('y', `${size.height - resizeSize.height}`);
+    this.updateResizeHandlePosition(size, resizeSize);
 
     this.onSizeChange(oldSize, this.size);
   }
@@ -340,6 +357,11 @@ export class CommentView implements IRenderedElement {
     return (topBarSize.height - foldoutSize.height) / 2;
   }
 
+  /** Updates the size of the top bar to reflect the new size. */
+  private updateTopBarSize(size: Size) {
+    this.topBarBackground.setAttribute('width', `${size.width}`);
+  }
+
   /** Updates the size of the text area elements to reflect the new size. */
   private updateTextAreaSize(size: Size, topBarSize: Size) {
     this.foreignObject.setAttribute(
@@ -411,6 +433,12 @@ export class CommentView implements IRenderedElement {
     this.textPreview.setAttribute('width', `${textPreviewWidth}`);
   }
 
+  /** Updates the position of the resize handle to reflect the new size. */
+  private updateResizeHandlePosition(size: Size, resizeSize: Size) {
+    this.resizeHandle.setAttribute('y', `${size.height - resizeSize.height}`);
+    this.resizeHandle.setAttribute('x', `${size.width - resizeSize.width}`);
+  }
+
   /**
    * Triggers listeners when the size of the comment changes, either
    * progrmatically or manually by the user.
@@ -750,7 +778,7 @@ css.register(`
   cursor: se-resize;
 }
 
-.blocklyCommentTopbar {
+.blocklyCommentTopbarBackground {
   fill: var(--commentBorderColour);
   height: 24px;
 }
@@ -774,13 +802,11 @@ css.register(`
   transform: rotate(-90deg);
 }
 
-.blocklyRTL .blocklyComment {
+.blocklyRTL .blocklyCommentTopbar {
   transform: scale(-1, 1);
 }
 
 .blocklyRTL .blocklyCommentForeignObject {
-  /* Revert the scale and control RTL using direction instead. */
-  transform: scale(-1, 1);
   direction: rtl;
 }
 
@@ -791,6 +817,7 @@ css.register(`
 }
 
 .blocklyRTL .blocklyResizeHandle {
+  transform: scale(-1, 1);
   cursor: sw-resize;
 }
 `);

core/comments/workspace_comment.ts

@@ -48,7 +48,7 @@ export class WorkspaceComment {
    *     be generated.
    */
   constructor(
-    protected readonly workspace: Workspace,
+    public readonly workspace: Workspace,
     id?: string,
   ) {
     this.id = id && !workspace.getCommentById(id) ? id : idGenerator.genUid();

core/serialization/workspace_comments.ts

@@ -39,15 +39,17 @@ export function save(
     saveIds?: boolean;
   } = {},
 ): State {
+  const workspace = comment.workspace;
   const state: State = Object.create(null);
 
   state.height = comment.getSize().height;
   state.width = comment.getSize().width;
 
   if (saveIds) state.id = comment.id;
   if (addCoordinates) {
-    state.x = comment.getRelativeToSurfaceXY().x;
-    state.y = comment.getRelativeToSurfaceXY().y;
+    const loc = comment.getRelativeToSurfaceXY();
+    state.x = workspace.RTL ? workspace.getWidth() - loc.x : loc.x;
+    state.y = loc.y;
   }
 
   if (comment.getText()) state.text = comment.getText();
@@ -76,9 +78,10 @@ export function append(
   if (state.text !== undefined) comment.setText(state.text);
   if (state.x !== undefined || state.y !== undefined) {
     const defaultLoc = comment.getRelativeToSurfaceXY();
-    comment.moveTo(
-      new Coordinate(state.x ?? defaultLoc.x, state.y ?? defaultLoc.y),
-    );
+    let x = state.x ?? defaultLoc.x;
+    x = workspace.RTL ? workspace.getWidth() - x : x;
+    const y = state.y ?? defaultLoc.y;
+    comment.moveTo(new Coordinate(x, y));
   }
   if (state.width !== undefined || state.height) {
     const defaultSize = comment.getSize();

core/xml.ts

@@ -62,8 +62,11 @@ function saveWorkspaceComment(
   const elem = utilsXml.createElement('comment');
   if (!skipId) elem.setAttribute('id', comment.id);
 
-  elem.setAttribute('x', `${comment.getRelativeToSurfaceXY().x}`);
-  elem.setAttribute('y', `${comment.getRelativeToSurfaceXY().y}`);
+  const workspace = comment.workspace;
+  const loc = comment.getRelativeToSurfaceXY();
+  loc.x = workspace.RTL ? workspace.getWidth() - loc.x : loc.x;
+  elem.setAttribute('x', `${loc.x}`);
+  elem.setAttribute('y', `${loc.y}`);
   elem.setAttribute('w', `${comment.getSize().width}`);
   elem.setAttribute('h', `${comment.getSize().height}`);
 
@@ -503,9 +506,12 @@ function loadWorkspaceComment(
 
   comment.setText(elem.textContent ?? '');
 
-  const x = parseInt(elem.getAttribute('x') ?? '', 10);
+  let x = parseInt(elem.getAttribute('x') ?? '', 10);
   const y = parseInt(elem.getAttribute('y') ?? '', 10);
-  if (!isNaN(x) && !isNaN(y)) comment.moveTo(new Coordinate(x, y));
+  if (!isNaN(x) && !isNaN(y)) {
+    x = workspace.RTL ? workspace.getWidth() - x : x;
+    comment.moveTo(new Coordinate(x, y));
+  }
 
   const w = parseInt(elem.getAttribute('w') ?? '', 10);
   const h = parseInt(elem.getAttribute('h') ?? '', 10);