fixes: remove unfinished shapes, remove selectable and make shapes remain inside canvas

[krmanik]

IO fixes

#2588
#2770
#2774
#2800
#2808

[krmanik]

Click through transparent area have following warning.

Canvas2D: Multiple readback operations using getImageData are faster with the willReadFrequently attribute set to true. See: https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-will-read-frequently

I have tried this for both upper and lower canvas, but it does not solve the warning so maybe the issues is in fabric.js.

canvas.getContext("2d", {willReadFrequently: true});

[dae]

Thanks a lot for looking into these Mani! I'll give them a proper review in the next few days after 23.10.1 is out the door.

[BEST8OY]

Not sure if I'm allowed to ask for this here, sorry in advance if I'm not.
A shortcut for "Toggle Masks" in reviewer would be handy.
Something like "M"

[krmanik]

Not sure if I'm allowed to ask for this here, sorry in advance if I'm not. A shortcut for "Toggle Masks" in reviewer would be handy. Something like "M"

Added key H for toggle mask because, M is already assigned to menu.

[dae]

Sorry for the delay here Mani - I've just gotten around to testing this. There seems to have been a regression in the positioning of images in the editor. I open the editor, select an image, and the image is offset to the right, with the right side cut off:

If I use the zoom tool, I can move the image so it appears correctly, but when I resize the window it moves back to the right.

It appears to have regressed in d44de20.

Other notes:

• Zooming seems to work well in the editor, but I would say we haven't solved Image occlusion doesn't scale when page zoomed in #2588 until the review screen supports zooming too. Minor: when making the window bigger, the mask borders get blurry.
• The shortcuts are a nice addition, but currently they conflict with the text tool - for example, try typing a 'p' into a text area
• Very minor: rectangles and ellipses are moved inside bounds after you let go of the mouse, whereas polygons and groups are never allowed to leave the bounds. If it's very easy to make the former behave like the latter, maybe it would be worth doing that to be consistent?
• Other fixes look good!

[krmanik]

Fixing these issues added more issues, I will push the changes considering above suggested review in next commit latest by weekend. :-)

Todo - add shortcuts info to tooltip

[dae]

No problem Mani, please let me know when you're ready for another review.

[krmanik]

I have solved some of minor IO issues and it should be merged first then I will check other issues in next PR.

[dae]

Hi Mani,

There's one issue that I think we should probably solve before merging this in, as it will break the text tool for people using git otherwise. I see you've pushed c689444, which has presumably fixed editing, but I hadn't actually noticed that issue. What I meant in my earlier comment is that when the text tool is used (below polygon), it's not possible to type text into it because the shortcuts are still active.

Re 5a27427, we don't include the shortcuts inside translation strings - instead they are stored separately, so we can modify them depending on the platform, and change them without breaking translations. For example, the bold button:

    const keyCombination = "Control+B";
...
    <IconButton
        tooltip="{tr.editingBoldText()} ({getPlatformString(keyCombination)})"

[krmanik]

I have updated the shortcut implementation as suggested.

The Ctrl+R is conflicting with RemoveFormatButton, what could be solution if we want to use Ctrl+R for rectangle tool?

[dae]

Thanks Mani, I'll merge this in now and we can make any further tweaks in follow-up PRs.

One possible way to work around the RemoveFormatButton issue would be to only enable that shortcut when the IO mask editor is not visible. That said, I think users coming from the existing add-on may miss the existing shortcuts, that don't require the control key. Maybe an alternative solution would be to use the text:editing:entered and text:editing:exited events to set a editingTextElement flag, and only respond to those shortcut keys when the flag is not set?

[krmanik]

I will match the keyboard shortcut and use the text event in next PR.