Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

t/951: DomConverter should actively prevent unwanted scrolling on focus #952

Merged
merged 3 commits into from
May 12, 2017

Conversation

oleq
Copy link
Member

@oleq oleq commented May 11, 2017

Suggested merge commit message (convention)

Fix: DomConverter should actively prevent unwanted scrolling on focus. Closes ckeditor/ckeditor5#4068. Closes ckeditor/ckeditor5#3903.


Additional information

Requires ckeditor/ckeditor5-utils#156.

@oleq oleq requested a review from Reinmar May 11, 2017 15:23
@Reinmar
Copy link
Member

Reinmar commented May 11, 2017

A more complete solution would be to look for all elements up in the tree (starting from the editable) which have scrollTop or something indicating that they are scrollable. But I guess that fixing the editable and viewport is OK for now.

@Reinmar
Copy link
Member

Reinmar commented May 11, 2017

R- for a different reason, though. AFAIU this issue concerns Blink and Webkit. So:

  • the issue and solution (env check) should not be about Webkit only,
  • perhaps we could target the solution to all browsers in order to avoid code branching?

I'm not 100% sure that the latter is a good idea, but it's something worth checking. The Webkit thing should be resolved though.

@oskarwrobel
Copy link
Contributor

@oleq
Copy link
Member Author

oleq commented May 12, 2017

@Reinmar

AFAIU this issue concerns Blink and Webkit

the issue and solution (env check) should not be about Webkit only,

I'm a little bit confused what you meant by this.

@Reinmar
Copy link
Member

Reinmar commented May 12, 2017

That this is not only about Webkit. This is about Webkit and Blink. Right? So why is there if ( webkit ) in the code?

@oleq
Copy link
Member Author

oleq commented May 12, 2017

Ah... so you're referring to ckeditor/ckeditor5-utils#156 (comment), right?

@Reinmar
Copy link
Member

Reinmar commented May 12, 2017

Yep. But it's not only about the code but also communication (issue, PR title, changelog, etc.).

@oleq oleq changed the title t/951: DomConverter should actively prevent window scroll on focus in WebKit t/951: DomConverter should actively prevent unwanted scrolling on focus May 12, 2017
@oleq
Copy link
Member Author

oleq commented May 12, 2017

I made the fix generic for all web browsers in 7431a14. Checked Firefox and Edge, and nothing looks wrong so I guess we may go with it this way.

@Reinmar
Copy link
Member

Reinmar commented May 12, 2017

It seems that it also fixed https://github.com/ckeditor/ckeditor5-engine/issues/706! :)

@Reinmar
Copy link
Member

Reinmar commented May 12, 2017

Would be green light but I miss a manual test. It's something that you need to test manually. OTOH... it's also an issue which we see quite well when using the editor. So, ok – I'm merging :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants