odoo icon indicating copy to clipboard operation
odoo copied to clipboard

[FIX] web_editor, website, web_tour: not show link tools when link contains a media

Open bso-odoo opened this issue 3 years ago • 7 comments

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

bso-odoo avatar Feb 16 '22 15:02 bso-odoo

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.

bso-odoo avatar Feb 24 '22 09:02 bso-odoo

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.

bso-odoo avatar Feb 24 '22 12:02 bso-odoo

@qsm-odoo Changes done, can be reviewed.

bso-odoo avatar Mar 24 '22 07:03 bso-odoo

@qsm-odoo Rebased after BS5 and re-tested on runbot.

bso-odoo avatar Jul 29 '22 07:07 bso-odoo

@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.

bso-odoo avatar Aug 11 '22 13:08 bso-odoo

@qsm-odoo Changes done & re-tested on runbot.

bso-odoo avatar Aug 12 '22 09:08 bso-odoo

@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)

qsm-odoo avatar Aug 17 '22 14:08 qsm-odoo

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.

bso-odoo avatar Aug 18 '22 15:08 bso-odoo

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)

bso-odoo avatar Aug 19 '22 14:08 bso-odoo

@qsm-odoo Re-rebased and re-tested on runbot.

bso-odoo avatar Aug 22 '22 11:08 bso-odoo

Merge method set to rebase and fast-forward.

robodoo avatar Aug 24 '22 11:08 robodoo

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

rdeodoo avatar Aug 29 '22 15:08 rdeodoo

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.

bso-odoo avatar Aug 30 '22 07:08 bso-odoo