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

Fix bottom margin jump on focus #15795

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brandonkelly
Copy link

@brandonkelly brandonkelly commented Feb 4, 2024

Suggested merge commit message (convention)

Fix: Bottom margin jumps on focus. Closes #15859


Additional information

This fixes a bug where the bottom padding of the editor will jump on focus, because the .ck-fake-selection-container element is added, which is fixed-positioned, but drops the margin-bottom styling from whatever was the :last-child.

It uses the :has selector, which is widely available now.

Related issues:

@Witoso
Copy link
Member

Witoso commented Feb 5, 2024

@brandonkelly could you provide some visual (video would be great) of the before and after, so that we are 100% on the same page. cc @oleq @pszczesniak

@brandonkelly
Copy link
Author

Sure!

Here’s a CKEditor field that ends with a <figure>, which normally has a bottom margin of .9em, except if it’s the :last-child where it will be set to var(--ck-spacing-large).

CleanShot.2024-02-05.at.05.00.02-trimmed.mp4

It’s subtle, but if you look closely you’ll see the editor shrinks by a few pixels when focused due to the difference between those margins.

With my PR, it no longer matters what elements’ normal margins are. They will get the same :last-child bottom margin, whether they are actually the last child, or if they are directly followed by the .ck-fake-selection-container element which is the actual last child.

CleanShot.2024-02-05.at.05.10.03.mp4

@Witoso
Copy link
Member

Witoso commented Feb 6, 2024

@vokiel could you verify CLA?

@vokiel
Copy link
Contributor

vokiel commented Feb 6, 2024

Confirmed. Proper status set.

@oleq
Copy link
Member

oleq commented Feb 14, 2024

The PR looks good to me (both the functionality and the code). Can we run it by the QA team @Witoso just to stay on the safe side?

@Witoso
Copy link
Member

Witoso commented Feb 15, 2024

Yep, we can do it.

@oleq
Copy link
Member

oleq commented Feb 15, 2024

@ckeditor/qa-team Can you take a look at this PR, please?

@dufipl
Copy link
Contributor

dufipl commented Feb 20, 2024

Sure, we will take a look at this 👍

@charlttsie
Copy link
Contributor

charlttsie commented Feb 27, 2024

Bottom margin jumps with certain styles applied

Steps

  1. Open all-features.html
  2. Set editor data to <figure class="image"><img src="sample.jpg" alt="bar"><figcaption>Caption</figcaption></figure><h2 class="document-title">test</h2>
  3. Shift the focus between the image and the heading

Result

1.mp4

Alternative scenario

  1. Open all-features.html
  2. Set editor data to <figure class="image"><img src="sample.jpg" alt="bar"><figcaption>Caption</figcaption></figure><p class="info-box">test</p>
  3. Shift the focus between the image and the paragraph

Result

2.mp4

Additional info

It's a regression - doesn't reproduce on master

@charlttsie
Copy link
Contributor

We've tested the PR with @marcelmroz and @kubaklatt and have found one regression that it introduces, which occurs when certain styles are used and is reported above. Besides that, the fix looks good and resolves the initial issue 👍

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.

Fix bottom margin jump on focus
6 participants