odoo
odoo copied to clipboard
[FIX] web_editor, website, web_tour: not show link tools when link contains a media
Since 1 the link tools got created when the destroyed sub-elements of the link were clicked on. This causes a problem when the element is a media because upon detroy the tools reset the link to an incorrect state.
After this commit the link tools is not touched anymore if the element that was clicked on is a media. Also, it reflects the changes done in the popup into the media link options, and the ones done in the option into the popup.
Steps to reproduce:
- Add an image-text snippet in the page
- Click on the image
- Set up a link
- DO NOT click on the image again
- Click on the left of the image (to click on the snippet itself)
- Click on the image again => The image was removed instead and a text link remained
This PR also fixes the fact that when emptying the URL the previously shown URL remained displayed in the popover.
task-2765857
Description of the issue/feature this PR addresses:
Current behavior before PR:
Desired behavior after PR is merged:
-- I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr
So activate_snippet does not work?
It correctly updates the displayed options, but it does not correctly select the image: if you press CTRL+K it messes up the DOM by creating another link.
Found another problem related to the tools unlink operation, but created task-2778912 for it because it does not seem related to the media link edition.
@qsm-odoo Changes done, can be reviewed.
@qsm-odoo Rebased after BS5 and re-tested on runbot.
@bso-odoo Sorry for coming back to this this late. I tried reproducing the orignal issue in a 15.0 and I cannot do it anymore grimacing Would another fix indirectly fixed that meanwhile? grimacing
@qsm-odoo It seems that exact scenario does not happen anymore - but a slightly different one still does. I updated the scenario of the first commit (& PR description). The scenario of the second commit still fails as it did before.
@qsm-odoo Changes done & re-tested on runbot.
@bso-odoo
In fact, I don't have the tooltip anymore after your commit when the link is on an image grimacing
Indeed, cannot reproduce anymore, probably had some local diff, my bad :shrug: Although I still have bugs after not searching for very long... maybe it's worth a pair-reviewing/testing tomorrow :+1:
- Add snippet image - text
- Click on the image
- Click on the link addition option
- Type "/test" in the URL input, do not focusout
- Click on the image again => Crash
Uncaught Javascript Error > this._requestUserValueWidgets(...)[0] is undefined
(tested on Firefox)
Uncaught Javascript Error > this._requestUserValueWidgets(...)[0] is undefined
I removed the core.bus.trigger('activate_image_link_tool'); in toggleLinkTools: the image link tool is already started at that point.
EDIT: Rollback - reverted this because the URL field did not get focused anymore. Resumed investigating root cause. Added a simple if around the focus call in the meantime.
Made sure the (de)activate events are only triggered once the snippet menu is ready.
Maybe that whole part of toggleLinkTools should wait for snippet menu too.
I could not reproduce the runbot error (that now disappeared) locally yet. It appeared this morning after a rebase, and disappeared after another rebase this afternoon. (non-conflicting)
@qsm-odoo Re-rebased and re-tested on runbot.
Merge method set to rebase and fast-forward.
Is there any reason for the test to be inside the test_website module and not the website one?
Unless I missed something, I don't see any reason that would make it go into test_website.
As per the manifest, those reasons should be:
- Doing some module operation like update/uninstall etc -> For perf reason we don't want to reload the whole website module and module depending of it
- Using some specific controllers for tests -> We don't want to introduce fake controller in website for only test purpose
- Using some specific records and/or views for tests -> Same rationale, we don't want to introduce fake data inside a real module for only test purpose
Sorry if I read this PR too quickly and it indeed match one of those reason :p
Unless I missed something, I don't see any reason that would make it go into
test_website.
@rdeodoo Indeed, I made that mistake, it should have been in website.