-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 box sizing for pseudo elements. #7545
Conversation
@jasmussen Thanks for looking into this: Is it possible to do .gutenberg {
box-sizing: border-box;
&::before,
&::after,
*,
::before,
::after {
box-sizing: inherit;
}
} For the rest of this comment, I've attempted to include the nesting but some items may have slipped through. The PR will need to take account of the following components/selectors reverting to the traditional box model to avoid introducing the same bug in reverse: .edit-post-meta-boxes-area {
* {}
}
.components-toolbar__control.components-button {
> svg {}
}
.components-tab-button {
&> span {} // Also, the & may be able to be removed here
}
.editor-post-permalink {} // uses the padding-box model
/* (code block 1) */ The following are also reverted to the traditional box model but are unaffected div.components-toolbar {
&.has-left-divider:before {}
}
/* (code block 2) */ Additionally, using the inherit pattern may allow you to remove the box model being set on: .core-blocks-spacer__resize-handler-top,
.core-blocks-spacer__resize-handler-bottom {}
.components-form-toggle {
.components-form-toggle__track {}
}
@mixin range-thumb() {}
.wp-block-image__resize-handler-top-right,
.wp-block-image__resize-handler-bottom-right,
.wp-block-image__resize-handler-top-left,
.wp-block-image__resize-handler-bottom-left { }
.wp-block-text-columns {
.wp-block-column { }
}
/* (code block 3) */ Setting the box model to .edit-post-meta-boxes-area {
textarea, input {}
#poststuff h3.hndle,
#poststuff .stuffbox > h3,
#poststuff h2.hndle { /* WordPress selectors yolo */ }
}
/* (code block 4) */ I'm on client work full time at the moment so haven't had the chance to test all my comments, thus weasle words like may and could instead of absolutes. Edit: Code block numbers added. |
Sure I can do that — but can you clarify a bit why the |
The source is from a CSS-Tricks article on box-sizing. Using ie In Gutenberg, this will become most apparent on the core
It's a bit of a mix of issues in master, issues that 7179576 introduce and issues that need to be protected against if you decide to go with the I've edited my original comment to include reference numbers in each of the code blocks. Block 1 These items all have the box model reverted back to The 7179576 approach will set their pseudo elements to use the The Block 2, Block 4 Unaffected by either solution, can be ignored. Block 3 On a really quick pre-coffee scan of the code base this-morning, it looks like inherit will allow you to remove setting the box model on these items. It's just about removing dead code. |
Okay this goes deep. I've tried to push a change in 941a2cb that also seems to have no adverse effects. Let me know if that's what you intended. I know that you wrote I'm a little fuzzy on the blocks and elements that need protecting from this — searching the codebase there area already a number of items that override back into |
I've pushed several changes after updating the branch against a04ae19 removes the wildcard selector from the legacy metabox display for returning the layout to the default box model. I've needed to add content-box to the d0b8c06 removes the only occurrence of 1c12a99 removes a bunch of redundant settings, either existing or because of the new code. Testing I visually tested the changes by comparing the new with the old using JS in the console. |
Very cool cleanup work. The latest rounds of pushes work fine for me. I'm about to head afk for some summer vacation, so I'm reassigning gutenberg-core for review so we can get this in. |
This PR tries to fix #6513. In my testing, this has no adverse side-effects, but like the original ticket reporter suggests, we need to verify that there are no visual regressions. It would be good to test: - metaboxes - plugins - all breakpoints - browsers - IE
The child elements now inherit the container’s box-sizing rules.
Value removed from spec and depricated in Firefox 57.
1c12a99
to
9ea510b
Compare
Overridden by what? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased to resolve conflicts. Changes look good 👍
Thanks Andrew. Can you merge when the checks pass? I'll be back in a little over a week 💕 |
I'm unclear of intent, so it may mean setting the box-sizing on |
This PR tries to fix #6513.
In my testing, this has no adverse side-effects, but like the original ticket reporter suggests, we need to verify that there are no visual regressions. It would be good to test:
The point of this is to ensure consistent behavior. If we're changing the box sizing behavior for basic elements, we should do it for pseudo elements also.