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

Table selection is active during image resize #6755

Closed
Mgsy opened this issue May 7, 2020 · 4 comments Β· Fixed by #7550
Closed

Table selection is active during image resize #6755

Mgsy opened this issue May 7, 2020 · 4 comments Β· Fixed by #7550
Assignees
Labels
package:image package:table package:widget type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Mgsy
Copy link
Member

Mgsy commented May 7, 2020

πŸ“ Provide detailed reproduction steps (if any)

  1. Insert a table with an image.
  2. Start the image resize and move the selection to the next cell.

βœ”οΈ Expected result

Cells are not selected.

❌ Actual result

Cells are selected, resizing fails.

πŸ“ƒ Other details

  • Browser: All browsers

GIF


If you'd like to see this fixed sooner, add a πŸ‘ reaction to this post.

@Mgsy Mgsy added the type:bug This issue reports a buggy (incorrect) behavior. label May 7, 2020
@mlewand mlewand added this to the unknown milestone May 11, 2020
@mlewand mlewand modified the milestones: unknown, backlog May 11, 2020
@niegowski
Copy link
Contributor

While checking this issue I noticed that table selection and widget resize dragging handling listens for mousedown on view document but this makes it impossible to stopPropagation for nested handlers. Image resize should stop the propagation of mousedown event so that the table handler will not start dragging. I think that we should listen to mousedown on specific objects (image, table) to use DOM event bubbling. mousemove and mouseup should still listen on the document.

@jodator jodator assigned niegowski and pomek and unassigned niegowski Jun 22, 2020
@jodator jodator modified the milestones: backlog, iteration 34 Jun 22, 2020
@pomek
Copy link
Member

pomek commented Jul 1, 2020

While checking this issue I noticed that table selection and widget resize dragging handling listens for mousedown on view document but this makes it impossible to stopPropagation for nested handlers.

Is not true at all. The resizing mechanism attaches its events to DOM document.

const domDocument = global.window.document;
this.editor.model.schema.setAttributeProperties( 'width', {
isFormatting: true
} );
this._observer = Object.create( DomEmitterMixin );
this._observer.listenTo( domDocument, 'mousedown', this._mouseDownListener.bind( this ) );
this._observer.listenTo( domDocument, 'mousemove', this._mouseMoveListener.bind( this ) );
this._observer.listenTo( domDocument, 'mouseup', this._mouseUpListener.bind( this ) );

That's why we can't use stopPropagation(). I tried to use different priorities for those events but with no luck.

I guess the fix must land to TableMouse plugin instead of WidgetResize. WDYT?

@jodator
Copy link
Contributor

jodator commented Jul 2, 2020

That's interesting. I wonder why there's Object.create() instead of direct implementation. Similar functionality in tables was created using custom MouseObserver class:

// Currently the MouseObserver only handles `mouseup` events.
// TODO move to the engine?
editor.editing.view.addObserver( MouseEventsObserver );

It might be worth checking if table & widget resize mouse handlers could not be merged together into existing MouseHandler.

But before that maybe @mlewand or @Reinmar have some insights why this observer is "detached" from editor view document.

@mlewand
Copy link
Contributor

mlewand commented Jul 2, 2020

Honestly, at this point I don't really remember why we went with Object.create() I recall there were some problems using observers, but unfortunately I don't remember what precisely has been an issue.

Joining observers, as suggested by @jodator, might be beneficial as both should work pretty much the same way. With this we should be able to to prioritize the image resize event listener over table's one.

That's fine for a workaround, but in the grand schema of things we should be able to solve it without relying on priority.

jodator added a commit that referenced this issue Jul 6, 2020
Fix (widget): The resizing mechanism will not trigger other `view.Document#mousedown` events. Thanks to that while resizing an image inside a cell, the mouse will not trigger the table's actions. Closes #6755.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image package:table package:widget type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants