Fix DrawingArea sending messages with an empty id #14009

celestial-starfruit · 2026-01-08T11:16:19Z

Resolves: #
Target version: main

Summary

TODO

...

Checklist

I have run make prettier-write and formatted the code.
All commits have Change-Id
I have run tests with make check
I have issued make run and manually verified that everything looks okay
Documentation (manuals or wiki) has been updated or is not required

eszkadev · 2026-01-09T09:00:56Z

Possibly it something which fixes similar problem to: #12484
but this one looks better

celestial-starfruit · 2026-01-12T11:23:07Z

This reminded me to update the code to include all references to callback, I believe my current implementation ends up creating a chain of elements to find the most recent which is a bit messy, we could directly find the most recent by searching for the id?

celestial-starfruit · 2026-01-12T11:47:04Z

I feel like finding the element by the id is a simpler solution, also I think that [ #12484 ] solves the same issue. Replacing the id of the deleted element should also work; we set the id to "" so that the new replacement element can take the old id, but it is then removed from the DOM, so setting the id back to the old id after removing is fine. Basically I think either would work.

eszkadev · 2026-01-14T07:24:50Z

I feel like finding the element by the id is a simpler solution, also I think that [ #12484 ] solves the same issue. Replacing the id of the deleted element should also work; we set the id to "" so that the new replacement element can take the old id, but it is then removed from the DOM, so setting the id back to the old id after removing is fine. Basically I think either would work.

be careful with id, we have MakeIdUnique function which appends some random numbers, we shouldn't have multiple nodes with the same id in the DOM for a11y compatibility. let me check the patch

eszkadev · 2026-01-14T07:25:41Z

browser/src/control/jsdialog/Widget.DrawingArea.js


+	container.getCurrent = function () {
+		if (container.id == "") {
+			return document.getElementById(data.id);


best to not query whole document, but the minimum required -> for example parentContainer

There isn't a clean way to do this; the element has been removed from the DOM (which is the entire problem), so no longer has a parent in the case that the id == "", and when the containing function is ran is the child of a document fragment, so the parent is also useless. It is possible f=by storing the parent after the element is removed:
https://github.com/celestial-starfruit/online/tree/private/maya/push-wwxovxkzppnp

isn't the object under ref: parentContainer still valid? is that detached too?
I guess it migth be scroll window in the .ui in core which is replaced.
In that case - yes we can query the DOM to be safe.

eszkadev · 2026-01-14T07:34:26Z

@celestial-starfruit code looks ok, but possibly the problem here is with update approach.
IIRC we don't have action to update DrawingArea widget, instead we will use update what basically replaces old node with new node.
What we could do is: implement action to change content in-place. it requires both core and online change.
That code would find canvas by id and then instead of removing it -> would replace content with new image.
Could you try? (can be separate PR to have this one as a fallback plan :) )

Where we execute action in online:

online/browser/src/control/Control.JSDialogBuilder.js

Line 2268 in 12f4743

executeActionImpl: function(container, data) {

How to send action in core:

example in: void JSComboBox::render_entry(int pos, int dpix, int dpiy) inside jsdialogbuilder.cxx
you will need to adjust void JSDrawingArea::queue_draw() and other methods if they produce update events

celestial-starfruit · 2026-01-14T15:17:48Z

That would avoid a lot of annoying work arounds. Even with these commits, there is flickering on (which I only got on firefox) (fixed with celestial-starfruit@45691d4) due to the new dataurl not being parsed yet but the old element being removed (also changing the dialog size cause the unloaded image has size 0), the workaround isn't great so I'll look into only updating the image url!

celestial-starfruit · 2026-01-15T13:49:32Z

Flickering is fixed by https://github.com/celestial-starfruit/online/tree/private/maya/push-wnytxnqpxpkz, but kinda nasty code.

firefox_flicker.mp4

chrome_no_flicker.mp4

eszkadev · 2026-01-15T13:51:55Z

browser/src/control/jsdialog/Widget.DrawingArea.js

-			ret[1] = e.changedTouches[e.changedTouches.length-1].pageY - $(imageElement).offset().top;
-		}
+		var boundingBox = imageElement.getBoundingClientRect();
+		var ret = [e.x - boundingBox.left, e.y - boundingBox.top];


did we check with touch device (real not simulated) too?

Nope, really good point! Not sure how to test that at the moment

@Minion3665 could you help?

eszkadev

code looks good in my opinion - improves a bit the state. If during testing will be ok,
we can merge and do the even better approach as a followup as it requires more time

When the drawing area is used, it is updated to use a new photo from the server; this replaces the old element, setting the ID to the empty string in the process. The next time it tries to send a message to the server, the message has an empty id, triggering an assert on the browser and crashing the server. Signed-off-by: Maya Stephens <[email protected]> Change-Id: Ia32fbadc730c83f2a89da38287a2c4406a6a6964

The previous method of calculating drawing area position had issues if the mouse went above a different element. Signed-off-by: Maya Stephens <[email protected]> Change-Id: Ia32fbadc730c83f2a89da38287a2c4406a6a6964

github-project-automation bot added this to Collabora Online Jan 8, 2026

github-project-automation bot moved this to To Review in Collabora Online Jan 8, 2026

celestial-starfruit force-pushed the private/maya/push-pwxkopmnswzn branch from b3af965 to 71a5e47 Compare January 8, 2026 16:08

eszkadev mentioned this pull request Jan 9, 2026

iconview-fix: Wrap builder callbacks to prevent empty widget IDs #12484

Open

5 tasks

celestial-starfruit force-pushed the private/maya/push-pwxkopmnswzn branch from 71a5e47 to 540b6e3 Compare January 12, 2026 11:20

celestial-starfruit force-pushed the private/maya/push-pwxkopmnswzn branch from 540b6e3 to f19f478 Compare January 12, 2026 11:39

celestial-starfruit marked this pull request as ready for review January 12, 2026 11:40

celestial-starfruit changed the title ~~Private/maya/push pwxkopmnswzn~~ Fix DrawingArea sending messages with an empty id Jan 12, 2026

celestial-starfruit force-pushed the private/maya/push-pwxkopmnswzn branch 2 times, most recently from 5f25e13 to 68bfe0c Compare January 12, 2026 14:20

mistmist requested a review from eszkadev January 13, 2026 19:36

eszkadev reviewed Jan 14, 2026

View reviewed changes

eszkadev reviewed Jan 15, 2026

View reviewed changes

eszkadev approved these changes Jan 15, 2026

View reviewed changes

github-project-automation bot moved this from To Review to To Test in Collabora Online Jan 15, 2026

vmiklos force-pushed the private/maya/push-pwxkopmnswzn branch from 68bfe0c to c93ca8d Compare January 16, 2026 13:02

vmiklos mentioned this pull request Jan 16, 2026

g: add a 'rebase-pr' sub-command #14108

Merged

celestial-starfruit added 2 commits January 16, 2026 16:51

browser: correctly calculate drawing area positon

c13990c

The previous method of calculating drawing area position had issues if the mouse went above a different element. Signed-off-by: Maya Stephens <[email protected]> Change-Id: Ia32fbadc730c83f2a89da38287a2c4406a6a6964

caolanm force-pushed the private/maya/push-pwxkopmnswzn branch from c93ca8d to c13990c Compare January 16, 2026 16:51

Fix DrawingArea sending messages with an empty id #14009

Are you sure you want to change the base?

Fix DrawingArea sending messages with an empty id #14009

Conversation

celestial-starfruit commented Jan 8, 2026

Summary

TODO

Checklist

Uh oh!

eszkadev commented Jan 9, 2026

Uh oh!

celestial-starfruit commented Jan 12, 2026

Uh oh!

celestial-starfruit commented Jan 12, 2026

Uh oh!

eszkadev commented Jan 14, 2026

Uh oh!

eszkadev Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

celestial-starfruit Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

eszkadev Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

eszkadev commented Jan 14, 2026

Uh oh!

celestial-starfruit commented Jan 14, 2026

Uh oh!

celestial-starfruit commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eszkadev Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

celestial-starfruit Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

eszkadev Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

eszkadev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

celestial-starfruit commented Jan 15, 2026 •

edited

Loading