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

Writing Flow: Stretch "Click Redirect" to occupy full height of content #18732

Merged
merged 3 commits into from
Dec 2, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Nov 25, 2019

This pull request seeks to resolve an issue where the editor canvas is not stretched enough on taller viewports.

Before After
Before After

The issue manifests itself in two ways:

  • Editor styles do not occupy the full height (see TwentyTwenty background color cuts off about 75% down the page)
  • The "Click Redirect" behavior (clicking below the editor to focus or create a last default block) does not work beyond the cut-off

This has caused some trouble for us in the past, typically around the nesting of elements within the EditPostVisualEditor component (most recently with typewriter revisions in #16460). I'm open to options for better ways to "stretch" descendents of this component, but as long as there is good test coverage, I'm not overly concerned with the nested selectors for height: 100% styling.

There existed tests previously which were meant to protect against regressions here ("clicking below the default block appender"). However, with the intrinsic height of the typewriter click appender, this test was not failing as it should have been. The tests have been updated here to (a) assure there is enough viewport height to allow for this gap to exist and (b) click at the bottom of the editor content area to confirm the expected behavior.

Testing Instructions:

Verify that, with a tall viewport, both editor styles and click redirect stretch to the bottom of the content area. You can use Chrome Responsive Viewport Mode if you do not have a tall enough monitor.

Ensure end-to-end tests pass:

npm run test-e2e packages/e2e-tests/specs/editor/various/adding-blocks.test.js

@aduth aduth added [Type] Bug An existing feature does not function as intended [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Nov 25, 2019
@aduth aduth requested a review from ellatrix November 25, 2019 20:00
.edit-post-visual-editor > .block-editor__typewriter,
.edit-post-visual-editor > .block-editor__typewriter > .block-editor-writing-flow,
.edit-post-visual-editor > .block-editor__typewriter > .block-editor-writing-flow > .block-editor-writing-flow__click-redirect {
height: 100%;
Copy link
Member

Choose a reason for hiding this comment

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

I see that .block-editor-writing-flow__click-redirect is smaller than .block-editor-writing-flow, taking into account the rest of the content, even though it's set to 100%. How does this work?

Copy link
Member Author

Choose a reason for hiding this comment

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

I see that .block-editor-writing-flow__click-redirect is smaller than .block-editor-writing-flow, taking into account the rest of the content, even though it's set to 100%. How does this work?

If I understand correctly:

.block-editor-writing-flow is a flex container. I recall that in the past, it was very specifically implemented that the click redirect have a base height to occupy the full space, shrinking if necessary. I think those specific shrink styles might have since been removed, but apparently it still works? I might want to do a round of cross-browser testing, and perhaps add back that attribute just to be explicit.

@jasmussen
Copy link
Contributor

I noticed what I think is a regression with the snackbar position. It's now in the top left corner:

Screenshot 2019-11-26 at 12 50 06

I think this PR might fix that. Can you test updating a published post, and see whether it does?

@aduth
Copy link
Member Author

aduth commented Nov 26, 2019

@jasmussen It was a separate regression, fixed in #18683.

@aduth
Copy link
Member Author

aduth commented Nov 26, 2019

Hmm, this doesn't work as expected when the content scrolls, presumably because the containers are set to a fixed height of 100% (so they only cover the initial 100% height of the scrollable region).

image

I would have thought simply changing this to min-height would be enough, but it seems to break the typewriter behavior.

I'll take a closer look.

@jasmussen
Copy link
Contributor

Do any of the changes in #18687 fix issues you encountered here?

@aduth
Copy link
Member Author

aduth commented Nov 26, 2019

It does seem to help when using min-height on .edit-post-visual-editor like you proposed in #18687, but then the 100% height elements within do not stretch as expected. Apparently this is a known browser bug. Still working through possible workarounds...

@aduth
Copy link
Member Author

aduth commented Nov 26, 2019

There are a few options I think we could consider:

  • From the referenced StackOverflow comment, a styling of display: table; height: 100%; appears to mimic the behavior of min-height: 100%, while not being prone to the same issues with children not being able to use height: 100%.
  • Change .editor-styles-wrapper to be the scrollable container. In some ways, this makes sense, since the theme background is applied to this element. However, I think there's a higher risk for unintended changes with this, notably with notices (Make notices push down content #13614). As far as the layout refactoring in Refactor the layout component to separate the UI from the content.  #18044, I think it makes sense to treat the "content region" as the scrollable area, regardless that we happen to put a block editor in that region.
  • Others that I haven't explored too deeply or that don't feel very viable:
    • display: flex; flex-grow: 1; on the container and each of its children?
    • position: absolute; to stretch to the container extents?

Personally, while it feels kinda hacky to use display: table; for this, I can't think of any other downside. I'd want to run this through a variety of browsers under some different conditions (notices, meta boxes, long/short content).

Unrelated: In testing this, I noticed as well there's a small gap between notices and the content area:

image

It seems to be caused by this margin:

.components-notice {
box-sizing: border-box;
margin: 0 0 5px;

I expect it might make sense to change that selector to .components-notice:not(:last-child) ?

@aduth
Copy link
Member Author

aduth commented Nov 26, 2019

Looking more closely at the changes in #18044, I think the issue might be simpler, in that previously the VisualEditor expanded to try to occupy as much space as possible using flex-basis: 100%.

That still exists in the VisualEditor styling:

But in #18044, the styling to treat the container as a flex container was removed:

https://github.com/WordPress/gutenberg/pull/18044/files#diff-ec5f5c91d0b3295f448977820605e4e4L41

Restoring the flex container styling seems to be sufficient to fix all the issues (ae77d49).

cc @youknowriad Since I understand the intention of #18044 to make these regions more unopinionated about the contents they contain, I'm curious if you see the changes of ae77d49 being problematic, or if you can think of another convention where these opinionated styles can exist (e.g. styling .edit-post-layout__content from the .visual-editor stylesheet).

I do notice one remaining issue which affects very-narrow, very-tall viewports (screenshot). However, I think this is unrelated to the changes here, since the issue stems from the fact that the entire page contents (#wpcontent) don't occupy the full height of the viewport.

@youknowriad
Copy link
Contributor

I'm curious if you see the changes of ae77d49 being problematic, or if you can think of another convention where these opinionated styles can exist (e.g. styling .edit-post-layout__content from the .visual-editor stylesheet).

Yes, my initial reaction is, that maybe these are specific to the post editor which means, we could define these styles in edit-post-layout__content instead. That said, I'm not strongly against this addition. I hope that we can move the current edit-widgets screen to use this new EditorRegions component (still some work to extract it properly), this will help us make better decisions like that.

@aduth
Copy link
Member Author

aduth commented Nov 26, 2019

I tried to describe the styles in a way which could be generalized for other use-cases. I think this behavior of "stretch to full height of the content region, shrink if necessary" could be common for other usage as well.

@aduth
Copy link
Member Author

aduth commented Nov 26, 2019

Unrelated: In testing this, I noticed as well there's a small gap between notices and the content area:

I created an issue for this at #18762.

@aduth aduth force-pushed the fix/writing-flow-click-redirect branch from ae77d49 to 3f6b393 Compare November 29, 2019 16:38
@aduth
Copy link
Member Author

aduth commented Dec 2, 2019

I'd like to allow this to have some time to sit in master to shake out potential issues before Monday's 7.1.0-RC release, so I'm going to merge using the prior approval, despite there being a couple more-recent revisions.

For additional testing, I used this branch to author a few blog posts last week. I also ran it through all supported browsers, with and without meta boxes and notices, and with short and long content.

Aside: Have meta boxes always been pushed down below content, vs. fixed at the bottom of the screen? I notice the behavior here is identical to when the plugin is disabled (i.e. WordPress 5.3), but I thought I recall it appearing "fixed" at some point in the past.

@aduth aduth merged commit e00fa01 into master Dec 2, 2019
@aduth aduth deleted the fix/writing-flow-click-redirect branch December 2, 2019 15:23
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants