[FIX] web_editor: tooltip with wrong information shows up #162621
Conversation
There was a problem hiding this comment.
Well, now it won't be wrongly shown indeed, since it won't be show ever since it's fully removed.
I don't see any explanation whatsoever in the commit message..
- Why is this happening?
- Since when is it happening?
- How was it behaving before the bug?
- What about previous version?
Checking 2 minutes on runbot in each versions, you can have a glimpse of information:
- In Odoo 15, that tooltip was correctly showing
- In Odoo 16 and later, no title was shown anymore, since a
pointer-event: nonerule was introduced (apparently, from quickly looking) - In master with owl conversion done here it went from JS to XML
- It's also that commit which changed the wording from
No location to drop intoThis block cannot be dropped anywhere on this page.
So it's maybe fine to just remove it, since it is not working since v16, but it should really come with some investigation and explanation.
And after investigating, one could actually wonder if it would not be better to replace the pointer-event none by something else to keep the title (but still prevent drag & drop, and maybe change the cursor to disabled visually). Without the title, it's IMO a bit confusing, you can't do it (drag & drop) but you don't know why.
I'll leave the rest up to you / RD India to check what you judge best, but please always come with some investigation and some reason / historical explanation of why is it done.
|
@rdeodoo Hello, thanks for the review. Well, the the root cause of the bug is that there is no such condition for displaying tooltip only when the block is disabled. As result tooltip is shown everywhere. When I checked the prior versions from 16.0, there was no such tooltip overthere. But even when I tried to show the tooltip using condition it was not working because of |
Due to the recent commit I mentioned, as I explained.
Exactly what I had to investigate and what I explained indeed
In all cases, full removal or making it work as before, always provide an explanation of what's going on in master and what happened in previous version (basically the full investigation I quickly made above). Also, the spec is IMO wrong, I don't know who wrote the get fully rid of it, but it's IMO worst than fixing it. The tooltip really provided added value (when it worked) as I explained. A disabled snippet with a ❗ symbol does not mean much, while a title is really adding value as it's very clearly explaining what's going on. Could you:
|
adsa-odoo
force-pushed
the
master-tooltip-lying-adsa
branch
5 times, most recently
from
b2c4c1b
to
eb45707
Compare
|
@rdeodoo Hello, I came up with a solution to show the tooltip. Here what we are doing is we are adding tooltip to the child element of the block and setting |
There was a problem hiding this comment.
Nice 👍
A step to reproduce would be really great:
- Login as admin and edit demo user, only give him the restricted editor right, remove everything else (alternatively switch the user from internal to public user to remove all rights then back from public to internal user, this field is only in debug mode).
- Now login as demo user
- Go to a product page
- Enter edit mode
- All snippets are disabled, because you don't have access rights to product
Something like that.
We should definitely consider backporting it after this one is merged (here in master it's also fixing 91293fe)
| t-att-class="{ 'o_we_already_dragging': snippet.renaming }" | ||
| t-att-data-snippet="snippet.baseBody.dataset.snippet" | ||
| t-att-style="snippet.disabled ? 'pointer-events: auto; cursor: not-allowed;' : ''" | ||
| t-att-data-tooltip="snippet.disabled ? 'This block cannot be dropped anywhere on this page.' : ''"> |
There was a problem hiding this comment.
This need to fallback on false, not empty string, so qweb doesn't set the attribute.
Otherwise, it will just look like this when it's not disabled:
| <div class="oe_snippet_thumbnail" | ||
| t-att-class="{ 'o_we_already_dragging': snippet.renaming }" | ||
| t-att-data-snippet="snippet.baseBody.dataset.snippet" | ||
| t-att-style="snippet.disabled ? 'pointer-events: auto; cursor: not-allowed;' : ''" |
There was a problem hiding this comment.
While applying the below remark about data-tooltip, can you quickly triple check if there is no classes related those 2 style properties? If it's the case, it could be moved in the t-att-class
(Don't create it if not exist, keep it like this)
Current behaviour before commit: -Hovering on any block of snippet options pannel shows up a tooltip with text 'This block cannot be dropped anywhere on this page' which is not true. The block can be dropped anywhere. This happens because of this commit [1]. However, the popover should be shown only when the snippet cannot be dropped anywhere. This situation occurs when a user has no editing rights. -Right now, when we try to show tooltip on this particular condition it doesn't work because the block element contains 'o_disabled' class which applies a CSS rule 'pointer-events: none' that prevents to show tooltip. Desired behaviour after commit: The tooltip should be shown only when block is disabled. [1]: 91293fe task-3875067
adsa-odoo
force-pushed
the
master-tooltip-lying-adsa
branch
from
eb45707
to
fe073d0
Compare
|
@adsa-odoo Don't forget to ping on the PR once you have made the changes. |
Current behaviour before commit:
Hovering on any block of snippet options pannel shows up a tooltip with text 'This block cannot be dropped anywhere
on this page' which is not true. The block can be dropped anywhere. This happens because of this commit 1.
However, the popover should be shown only when the snippet cannot be dropped anywhere. This situation occurs when a
user has no editing rights. This can be reproduced like this:
'o_disabled'class which applies a CSS rule'pointer-events: none'that prevents to show tooltip.Desired behaviour after commit:
The tooltip should be shown only when block is disabled.
task-3875067
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr