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

Cutting the widget scrolls editor to the end (with t/468) #1170

Merged
merged 5 commits into from
Nov 20, 2017
Merged

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented Nov 15, 2017

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

What changes did you make?

Closes #1160.

Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

Issue is still reproducible inside editor inside h1 tag after adding some more content (2-3 times more than now).

@@ -0,0 +1,80 @@
<head>
Copy link
Member

Choose a reason for hiding this comment

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

Can't these tests be moved to plugins/widget? They're widget specific, so that place seems more obvious to me. We got even plugins/widget/integration directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Moved to widget directory.

@f1ames
Copy link
Contributor Author

f1ames commented Nov 17, 2017

The only way I was able to workaround this issue is to use the div element for blockless editor. Haven't found any negative side effects so it seems to be working fine.

The only problem is that for this 100ms (when pastebin container is present) we have invalid html structure (div inside h1).

I think the only reliable, not hacky solution would be get rid of pastebin as described in #1169.

Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

Hmm, I have some doubts…

The biggest one is if we really want to merge this hack? Maybe we could (should?) just fix it in #1169?

The second issue is about produced markup. One of our defined editable elements is p. In terms of HTML putting div into p is not only disallowed, but even impossible as browsers parse <p><div></div></p> as <p></p><div></div>. However it seems that DOM doesn't have any issues with it 🤔

My proposal is to ditch this PR and work on #1169. WDYT @f1ames?

@mlewand
Copy link
Contributor

mlewand commented Nov 20, 2017

Just to chime in, there's no chance in the world we'll be able to deliver #1169 in a reasonable time. If this PR does the job, let's just use it as a workaround, and then revisit this in future releases.

Just one more question: is this div in a p thing introduced by this very PR, or was it already in the editor since beginning?

@Comandeer
Copy link
Member

Just one more question: is this div in a p thing introduced by this very PR, or was it already in the editor since beginning?

It seems to be there from beginning – that's how our fake selection works: putting hidden div at the end of the editable. It shouldn't work, but works flawlessly so far.

@mlewand
Copy link
Contributor

mlewand commented Nov 20, 2017

So this is nothing critical that we need to fix ASAP in this release. Instead we'll deal with it in the upcoming.

Meanwhile to avoid UX regression for Edge users, let's use this PR as a temporary workaround. Feel free to add a reference in code comments to a #1169 saying that xyz could be safely removed when #1169 is fixed.

BTW:

It shouldn't work, but works flawlessly so far.

Love that part 😁

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.

3 participants