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

[Safari] Triple-click in a nested editable selects the whole widget #1463

Closed
Reinmar opened this issue Jan 15, 2019 · 12 comments
Closed

[Safari] Triple-click in a nested editable selects the whole widget #1463

Reinmar opened this issue Jan 15, 2019 · 12 comments
Assignees
Labels
package:widget type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jan 15, 2019

Is this a bug report or feature request? (choose one)

🐞 Bug report

💻 Version of CKEditor

Master

📋 Steps to reproduce

  1. Open https://ckeditor.com/docs/ckeditor5/latest/examples/builds/classic-editor.html in Safari
  2. Scroll the content to the image.
  3. Place the caret in the caption.
  4. Triple click there.

✅ Expected result

image

❎ Actual result

image

📃 Other details that might be useful

Which is even/also very uncool – this leads to excessive scroll jump.

@Reinmar
Copy link
Member Author

Reinmar commented Jan 15, 2019

I'm adding it to the next iteration because this is very annoying. Actually, it was frustrating for me when I tried to edit the content.

@Reinmar Reinmar added this to the iteration 22 milestone Jan 15, 2019
@Reinmar Reinmar added type:bug This issue reports a buggy (incorrect) behavior. status:confirmed labels Jan 15, 2019
@Reinmar
Copy link
Member Author

Reinmar commented Jan 15, 2019

cc @jodator

@pomek
Copy link
Member

pomek commented Feb 4, 2019

The MouseEvent (for the click event) defines the detail property which describes how many clicks a user did.

https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-MouseEvent

We could observe the whole editable area and prevent the event if details value is higher than 2.

@pomek
Copy link
Member

pomek commented Feb 6, 2019

Unfortunately, preventing the event or/and stopping propagation didn't work. Seems like the selection is being modified before firing the click event.

@Reinmar
Copy link
Member Author

Reinmar commented Feb 11, 2019

Unfortunately, preventing the event or/and stopping propagation didn't work. Seems like the selection is being modified before firing the click event.

Can't we simply handle this event and set selection ourselves? We know the event target, right? So we know where the user was clicking so we can select the right content. Am I right?

@pomek
Copy link
Member

pomek commented Feb 12, 2019

Because I don't have an idea on how to select a line which was triple clicked by a user, I selected the whole figcaption element. Unfortunately, it blinks. Safari modifies selection earlier than me. It looks very weird. See the attached GIF.

Blink selection GIF

@Reinmar
Copy link
Member Author

Reinmar commented Feb 12, 2019

I selected the whole figcaption elemen

The selection should be put inside the <figcaption> element. In fact, inside the <caption> element because this should be done on the model.

@pomek
Copy link
Member

pomek commented Feb 12, 2019

I changed it but it still blinks. The worst thing is that is blinks randomly.

@Reinmar
Copy link
Member Author

Reinmar commented Feb 12, 2019

No code, no comments.

@pomek
Copy link
Member

pomek commented Feb 12, 2019

@oleq
Copy link
Member

oleq commented Feb 19, 2019

I did a quick research and this is what I found:

  • listening to click makes little sense as the selection is made upon mousedown
  • the issue also happens when domEvent.detail > 3

I created a solution draft which is as simple as

view.addObserver( MouseObserver );

view.document.on( 'mousedown', ( evt, data ) => {
	console.log( 'mousedown', data.domEvent.detail );

	if ( data.domEvent.detail >= 3 && data.target.is( 'figcaption' ) ) {
		data.domEvent.preventDefault();

		view.change( writer => {
			writer.setSelection( data.target, 'in' );
		} );
	}
} );

and it seems to be working. All we need now is:

  • a check that makes sure this covers affected browsers
  • more general element discovery, probably it concerns all nested editables, not just <figcaptions>,
  • tests

2019-02-19 11-37-43 2019-02-19 11_38_45

@oleq
Copy link
Member

oleq commented Mar 18, 2019

This issue must be fixed at the widget–level (not image–level) so I'm afraid ckeditor/ckeditor5-image#280 will not do.

oleq added a commit to ckeditor/ckeditor5-utils that referenced this issue Mar 28, 2019
Feature: Added `isSafari` property and `isSafari()` helper to the `env` module. See: ckeditor/ckeditor5#1463.
oleq added a commit to ckeditor/ckeditor5-widget that referenced this issue Mar 28, 2019
Fix: Triple clicking inside a nested editable should not select the entire widget in Safari. Closes ckeditor/ckeditor5#1463.
@mlewand mlewand changed the title [Safari] Tripple-click in a nested editable selects the whole widget [Safari] Triple-click in a nested editable selects the whole widget Mar 29, 2019
oleq added a commit to ckeditor/ckeditor5-widget that referenced this issue Apr 4, 2019
pomek added a commit to ckeditor/ckeditor5-widget that referenced this issue Apr 5, 2019
Internal: The triple-click fix for Safari introduced in ckeditor/ckeditor5#1463 should alter the model to work properly. Closes ckeditor/ckeditor5#1677.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:widget type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

3 participants