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

Blocks: Un-collapse Classic block margins #8308

Closed
wants to merge 2 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 30, 2018

Fixes #7918
Related (regression of): #7365

This pull request seeks to resolve an issue where the Classic block TinyMCE panel is not flush with its block container. It does so by reverting the margin collapsing for the Classic block only, similar to as proposed for the Columns block in #8283.

Before After
Before After

Testing instructions:

Repeat steps to reproduce from #7918, verifying that there is no excess gap between the Classic block panel and its block container.

@aduth aduth added the [Feature] Blocks Overall functionality of blocks label Jul 30, 2018
@aduth aduth added this to the 3.4 milestone Jul 30, 2018
@aduth aduth requested a review from jasmussen July 30, 2018 18:52
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

This works for me. Sorry about the issues — I knew the margin collapsing was a big refactor which is why it was underway for more than a month. I hope this is the last of the issues it caused, and I still deeply believe the refactor is worth it.

Noting, as also mentioned in Slack, https://wordpress.slack.com/archives/C02QB2JS7/p1532978287000072, that .1px margin collapsings can cause visual issues. Notably on retina screens. You might see a flickering as you scroll, because the rounding math doesn't always work the way we want it to. The alternative would be to use 1px and then compensate for this 1px with a negative 1px elsewhere.

@jasmussen
Copy link
Contributor

Not sure how kosher this is, but if you change the .1px padding to 1px, then you can compensate for it like this:

.freeform-toolbar {
position: relative;
top: -1px;
}

@aduth
Copy link
Member Author

aduth commented Jul 30, 2018

More generally, we should figure out what it is about the collapsed margins that is incompatible with various blocks, rather than addressing them in ad hoc fashions like I'm doing here.

@aduth
Copy link
Member Author

aduth commented Jul 30, 2018

Not sure how kosher this is, but if you change the .1px padding to 1px, then you can compensate for it like this:

This seems to have undesirable consequences, offsetting the activated block's toolbar. Maybe something with the toolbar's existing sticky position styling.

@jasmussen
Copy link
Contributor

jasmussen commented Jul 30, 2018

The reason why we need collapsing margins in the first place is that the most basic blocks, paragraph, list, image and so on, should ideally behave the same as they do on the frontend so the editor style doesn't have to account for an arbitrary margin we apply to the editor. It's one small step on the wysiwyg path.

I agree it would be nice if things felt more resilient. Since two things in a row broke because of this effect, columns and classic block, it can feel brittle. But perhaps this is a side effect of having to refactor to support collapsing margins midstream.

@aduth
Copy link
Member Author

aduth commented Jul 30, 2018

Digging into it further, it seems that conflicts can occur when elements within a block try to offset themselves by negative margins, as we were doing in the freeform block with the toolbar and the TinyMCE element. I've removed these and all appears to work well; the main uncertainty I have now is that I'd needed to remove position: sticky to achieve this.

@jasmussen
Copy link
Contributor

Hmm, we need the toolbars to be sticky though, classic and modern alike. If that's broken now perhaps the padding fix was better

jasmussen added a commit that referenced this pull request Jul 31, 2018
This is an alternative to #8308, which doesn't rely on uncollapsing margins.

What it does is position the classic toolbar using translate, which is the same way every other toolbar is positioned. This fixes the issue and cleans up the CSS.
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Although I approved this as a hotfix, I think #8314 might be a better fix. Can you take a look?

jasmussen added a commit that referenced this pull request Jul 31, 2018
This is an alternative to #8308, which doesn't rely on uncollapsing margins.

What it does is position the classic toolbar using translate, which is the same way every other toolbar is positioned. This fixes the issue and cleans up the CSS.
@jasmussen
Copy link
Contributor

I merged in a better fix, inspired by this branch. Thanks for fixing my regression. I think this PR can be closed.

@pento pento removed this from the 3.4 milestone Jul 31, 2018
@aduth
Copy link
Member Author

aduth commented Jul 31, 2018

Thanks @jasmussen ! Will close this one.

@aduth aduth closed this Jul 31, 2018
@aduth aduth deleted the fix/7918-uncollapsed-freeform-margins branch July 31, 2018 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Classic Block toolbar doesn't sit on the edge of frame
3 participants