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

#7542: Widget type around UI should not be selectable by triple-click. #7599

Merged
merged 5 commits into from
Jul 10, 2020

Conversation

niegowski
Copy link
Contributor

@niegowski niegowski commented Jul 9, 2020

Suggested merge commit message (convention)

Fix (widget): Triple-click inside image caption should not crash editor in Firefox. Closes #7542.

Fix (widget): Triple-click on link inside image caption should not crash editor in Safari. Closes #6021.


Additional information

@niegowski niegowski requested a review from oleq July 9, 2020 17:04
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

Although this "solves" the issue, this is still a hack and the original issue (whatever it is) remains unsolved and could backfire in the future.

  • For one thing, the engine should never allow the selection to cross the boundary of the figcaption (nested editable) in the first place. And the type around UI being outside the figcaption in DOM should never have an impact on how the figcaption works.
  • Another thing is that the patch breaks the triple click functionality in FF (the third click selects the widget instead of the entire content of the nested editable).

I think we must dig deeper and figure out what happens on the native DOM level first to address the issue properly on the engine/widget level. This patch can only go as a last resort solution if we run out of ideas.

@niegowski
Copy link
Contributor Author

DOM Selection Ranges after triple-click:

View Ranges converted by DomConverter:

Model Ranges converted by Mapper that are triggering model-selection-range-intersects exception:

@niegowski
Copy link
Contributor Author

Also I noticed that <figcaption> has display: table-caption; style, but it's replaced with display: block; if image was resized. This problem doesn't occur if <figcaption> has display: block; style.

…support and properly handle click on AttributeElement.
@niegowski niegowski requested a review from oleq July 10, 2020 11:04
const mapper = editor.editing.mapper;
const modelElement = mapper.toModelElement( element );
const viewElement = element.is( 'attributeElement' ) ?
element.findAncestor( element => !element.is( 'attributeElement' ) ) : element;
Copy link
Member

Choose a reason for hiding this comment

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

What if there's a widget > nested editable > block-quote > link structure? Feels like viewElement will be the blockquote and the entire nested editable will not be selected. I think isInsideNestedEditable() could become getClosestNestedEditable() and you could re-use its output here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in that situation entire content of that content editable would be selected instead of single paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that table cells are also nested editables and triple-click action is expected to select current block only, not entire content editable.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, my bad.

Copy link
Member

Choose a reason for hiding this comment

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

Can we have another test then that checks only the closest non-attribute is selected and not the entire nested editable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@niegowski niegowski requested a review from oleq July 10, 2020 15:08
@oleq oleq merged commit ef4b1f9 into master Jul 10, 2020
@oleq oleq deleted the i/7542 branch July 10, 2020 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants