Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Calculate the offsetHeight for the toolbar only when the table is used #1543

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

orsolyaDekany
Copy link
Contributor

@orsolyaDekany orsolyaDekany commented Nov 8, 2022

Hi Team,

Issue: #1542
LPS: https://issues.liferay.com/browse/LPS-166604

The issue is not reproduceable before this commit.

Fix: the offsetHeight is calculated only when the toolbar is positioned for the table
With the fix, the toolbar is positioned again on top of each image and also on top of every cell/row in a table that is currently selected.

Could you please review it?

cc. @markocikos

@markocikos
Copy link
Member

@orsolyaDekany Is the bug reproducible in other places where we use alloy editor, or is it specific only to web content on 7.3? I couldn't reproduce it on master Blogs page.

Copy link
Member

@markocikos markocikos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orsolyaDekany see comment inline, and also #1543 (comment)

src/selections/selection-position.js Outdated Show resolved Hide resolved
@orsolyaDekany
Copy link
Contributor Author

Hi @markocikos ,
The fix is specific to 7.3.x and the issue is reproducible in Blogs as well. The issue is not reproducible using Alloyeditor in Blogs on Master, because of another root cause that was a unique fix to 7.3.x, caused by this commit based on my testing. If that change is reverted, the issue is not reproducible in Blogs anymore, however it is still in Web Content. The different behavior might be because of the different editor configurations between the modules, I am not sure.

In the meantime, I also added "startContainer.$.nodeName === "TH", because I realized that header can be set for the columns and rows after creating a table.

Copy link
Member

@markocikos markocikos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orsolyaDekany almost good to go! Please see comment inline.

@@ -56,7 +56,7 @@ const centerToolbar = function(toolbar, rect) {
if (startContainer.$.nodeType !== Node.ELEMENT_NODE) {
startContainer = startContainer.getParent();
}
if (startContainer) {
if (startContainer.$.nodeName === "TD" || startContainer.$.nodeName === "TH") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this actually fixes a small bug in master, where a text line (paragraph) followed by an image causes wrong positioning. Awesome! I'll trust you that this also fixes the 7.3 bug from the ticket.

Before merging, we should just tidy this a bit. Please move node name to a variable, use single quotes, break line if needed. If you followed setup, you can use yarn format (and yarn test for doublecheck).

Copy link
Member

@markocikos markocikos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants