Skip to content

Image insertion support for TipTap editor integration#23599

Open
f2cmb wants to merge 10 commits intoglpi-project:mainfrom
f2cmb:kb/editor/insertImage
Open

Image insertion support for TipTap editor integration#23599
f2cmb wants to merge 10 commits intoglpi-project:mainfrom
f2cmb:kb/editor/insertImage

Conversation

@f2cmb
Copy link
Copy Markdown
Contributor

@f2cmb f2cmb commented Mar 24, 2026

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

Screenshots (if appropriate):

@f2cmb f2cmb marked this pull request as ready for review March 25, 2026 14:00
@f2cmb f2cmb linked an issue Mar 25, 2026 that may be closed by this pull request
Copy link
Copy Markdown
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a few functional issues:

1) Bubble menu actions are mostly useless on image, maybe it should be disabled when selecting an image (or filtered to show only useful actions)?

Image

2) Can't paste image

I was able to paste a first image but now it fails with this error:

Image
[2026-03-30 09:38:14] glpi.CRITICAL:   *** Uncaught PHP Exception TypeError: "Safe\preg_replace_callback(): Argument #2 ($callback) must be of type callable, array given, called in ./src/UploadHandler.php on line 692" at pcre.php line 100
  Backtrace :
  ...hecodingmachine/safe/generated/8.1/pcre.php:100 
  ./src/UploadHandler.php:692                        Safe\preg_replace_callback()
  ./src/UploadHandler.php:733                        UploadHandler->upcount_name()
  ./src/UploadHandler.php:868                        UploadHandler->get_unique_filename()
  ./src/UploadHandler.php:1334                       UploadHandler->get_file_name()
  ./src/UploadHandler.php:1804                       UploadHandler->handle_file_upload()
  ./src/GLPIUploadHandler.php:63                     UploadHandler->post()
  ./ajax/fileupload.php:40                           GLPIUploadHandler::uploadFiles()
  ...Glpi/Controller/LegacyFileLoadController.php:64 require()
  ./vendor/symfony/http-kernel/HttpKernel.php:183    Glpi\Controller\LegacyFileLoadController->__invoke()
  ./vendor/symfony/http-kernel/HttpKernel.php:76     Symfony\Component\HttpKernel\HttpKernel->handleRaw()
  ./vendor/symfony/http-kernel/Kernel.php:193        Symfony\Component\HttpKernel\HttpKernel->handle()
  ./public/index.php:71                              Symfony\Component\HttpKernel\Kernel->handle()

3) Duplicated link for the first image I was able to paste:

Image

4) Sessions messages after reloading the page

Image

Either show them when saving or none at all, it feel weird to see them pop on the next page navigation.

@f2cmb f2cmb force-pushed the kb/editor/insertImage branch from c3391b3 to 0d60a81 Compare April 2, 2026 13:45
@f2cmb
Copy link
Copy Markdown
Contributor Author

f2cmb commented Apr 3, 2026

Asking new validation as all changes were adressed except createElement vs glpi_html_dialog : still investigating on my side on this point to see if it's better or no in the context of the feature

Edit : i think both <template> and glpi_html_dialog are not good in our context bcz of jQuery dependency, or regression risks. Keeping vanilla DOM feels like the right call here. Happy to revisit once glpi_html_dialog is rewritten in vanilla JS.

@cconard96
Copy link
Copy Markdown
Contributor

Edit : i think both <template> and glpi_html_dialog are not good in our context bcz of jQuery dependency, or regression risks. Keeping vanilla DOM feels like the right call here. Happy to revisit once glpi_html_dialog is rewritten in vanilla JS.

Using an existing helper function that uses jQuery is not the same as using it directly, and jQuery is not a requirement to use template literals.

const dialog = document.createElement('div');
dialog.innerHtml = `
    <div class="image-dialog">
        <div class="image-dialog-header">[...]</div>
        [...]
    </div>
`;

You already use them a little bit, but it is cleaner if you can refactor it to only use createElement for the container, and then the rest is a single template literal.

Anyways, this is exactly the kind of thing that makes more sense in Vue where there is a clean way to encapsulate JS with an HTML template and styles, and is why I was pushing for it for the new KB when work began on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Image upload in TipTap

3 participants