[FIX] web_editor: fix colorpicker position in iframe #160505
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Mtaylorr
force-pushed
the
saas-16.4-task-3614965-macr
branch
from
434dfd4
to
739b850
Compare
|
@fw-bot up to 17.0 |
|
Forward-porting to '17.0'. |
Issue: ====== Colorpicker doesn't appear in mass mailing Steps to reproduce the issue: ============================= - Create a new mass mailing with a some template other than plain text - Try to change the color of some text - The position of the colorpicker is wrong. Origin of the issue: ==================== When wysiwyg was converted to owl in [1], an effort was made to speed up the loading of the iframe in mass_mailing. One of the changes that were done in that regard was to remove assets from the iframe to make it load faster. This required to create the sidebar (SnippetsMenu) outside of the iframe since the iframe did not have the required files anymore, and insert it back in the iframe afterwards, since it was designed to work inside the iframe. This change actually had an impact on the positioning of the colorpicker, and basically anything that relied on popper.js for positioning, because since popper.js was outside of the iframe then the checks it did based on `instanceof HTMLElement` were returning false for every node inside the iframe. At the time of [1] this went unnoticed because the chatter was not yet in the side of the screen for mass_mailing, so the wrong positioning of the colorpicker was actually only slightly off the right position, thus being hard to catch while not specifically looking for that particular issue. As soon as the chatter was made to be on the side even in the case of mass_mailing, the wrong colorpicker position became visible but the issue went unnoticed at the time as well, probably because the two changes were completely unrelated. This went live in saas-16.4 and is the case in 17.0 as well. However, the issue does not exist anymore in saas-17.1 due to the refactor of mass_mailing to have the sidebar (SnippetsMenu) working from outside of the iframe instead of inside. Solution: ========= Fixing this issue properly would require huge changes to how the SnippetsMenu is constructed and would most likely require going back to the slow iframe with all the assets inside. That would not be a desirable outcome, especially in a stable version. With that in mind, and considering the issue doesn't exist in saas-17.1, we decided it was a prime example where a local change in the popper.js library was actually the best fix. The library is very unlikely to be updated in a stable version and the change won't reach saas-17.1. [1]: odoo@76d4f98 co-authored with dmo-odoo task-3614965
Mtaylorr
force-pushed
the
saas-16.4-task-3614965-macr
branch
from
739b850
to
2088237
Compare
C3POdoo
requested review from
a team,
aab-odoo and
Iucapad
and removed request for
a team
|
@robodoo r+ |
robodoo
pushed a commit
that referenced
this pull request
Apr 23, 2024
Issue: ====== Colorpicker doesn't appear in mass mailing Steps to reproduce the issue: ============================= - Create a new mass mailing with a some template other than plain text - Try to change the color of some text - The position of the colorpicker is wrong. Origin of the issue: ==================== When wysiwyg was converted to owl in [1], an effort was made to speed up the loading of the iframe in mass_mailing. One of the changes that were done in that regard was to remove assets from the iframe to make it load faster. This required to create the sidebar (SnippetsMenu) outside of the iframe since the iframe did not have the required files anymore, and insert it back in the iframe afterwards, since it was designed to work inside the iframe. This change actually had an impact on the positioning of the colorpicker, and basically anything that relied on popper.js for positioning, because since popper.js was outside of the iframe then the checks it did based on `instanceof HTMLElement` were returning false for every node inside the iframe. At the time of [1] this went unnoticed because the chatter was not yet in the side of the screen for mass_mailing, so the wrong positioning of the colorpicker was actually only slightly off the right position, thus being hard to catch while not specifically looking for that particular issue. As soon as the chatter was made to be on the side even in the case of mass_mailing, the wrong colorpicker position became visible but the issue went unnoticed at the time as well, probably because the two changes were completely unrelated. This went live in saas-16.4 and is the case in 17.0 as well. However, the issue does not exist anymore in saas-17.1 due to the refactor of mass_mailing to have the sidebar (SnippetsMenu) working from outside of the iframe instead of inside. Solution: ========= Fixing this issue properly would require huge changes to how the SnippetsMenu is constructed and would most likely require going back to the slow iframe with all the assets inside. That would not be a desirable outcome, especially in a stable version. With that in mind, and considering the issue doesn't exist in saas-17.1, we decided it was a prime example where a local change in the popper.js library was actually the best fix. The library is very unlikely to be updated in a stable version and the change won't reach saas-17.1. [1]: 76d4f98 co-authored with dmo-odoo task-3614965 closes #160505 Signed-off-by: David Monjoie (dmo) <dmo@odoo.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue:
Colorpicker doesn't appear in mass mailing
Steps to reproduce the issue:
Origin of the issue:
When wysiwyg was converted to owl in 1, an effort was made to speed up the loading of the iframe in mass_mailing. One of the changes that were done in that regard was to remove assets from the iframe to make it load faster. This required to create the sidebar (SnippetsMenu) outside of the iframe since the iframe did not have the required files anymore, and insert it back in the iframe afterwards, since it was designed to work inside the iframe.
This change actually had an impact on the positioning of the colorpicker, and basically anything that relied on popper.js for positioning, because since popper.js was outside of the iframe then the checks it did based on
instanceof HTMLElementwere returning false for every node inside the iframe. At the time of 1 this went unnoticed because the chatter was not yet in the side of the screen for mass_mailing, so the wrong positioning of the colorpicker was actually only slightly off the right position, thus being hard to catch while not specifically looking for that particular issue.As soon as the chatter was made to be on the side even in the case of mass_mailing, the wrong colorpicker position became visible but the issue went unnoticed at the time as well, probably because the two changes were completely unrelated. This went live in saas-16.4 and is the case in 17.0 as well. However, the issue does not exist anymore in saas-17.1 due to the refactor of mass_mailing to have the sidebar (SnippetsMenu) working from outside of the iframe instead of inside.
Solution:
Fixing this issue properly would require huge changes to how the SnippetsMenu is constructed and would most likely require going back to the slow iframe with all the assets inside. That would not be a desirable outcome, especially in a stable version. With that in mind, and considering the issue doesn't exist in saas-17.1, we decided it was a prime example where a local change in the popper.js library was actually the best fix. The library is very unlikely to be updated in a stable version and the change won't reach saas-17.1.
co-authored with dmo-odoo
task-3614965