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

Try using left borders for hover + selection states #14145

Merged
merged 38 commits into from
Mar 15, 2019
Merged

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Feb 27, 2019

This PR is a possible way to address two problems:

  1. Our current blue hover state is too prominent. (Consider a subtler hover state for blocks #14095)
  2. Our current block selection state is too low contrast. (Make the selected block outline darker  #12254)

For visual reference, here's how our hover and selection states currently work:

hover-focus-progression

This PR removes the blue outline from our hover state, and replaces it with a blue left border. It adjusts the location of the block breadcrumb to match. The PR also adds a dark left border to the block and block toolbar, to improve accessibility there:

hover-focus-progression

Screenshots

Here's a GIF of this in action:

hover-select

And an example on mobile:
(there are no hover states on mobile, but the left border is now present here)

screen shot 2019-02-27 at 10 44 38 am


Some considerations:

  • In general, this makes the whole interface feel a little heavy on the left side. This might be fine, but it was a little weird to get used to.
  • When blocks are set to alignfull, the left border only appears on the block toolbar (unless you're on mobile, in which case it does not appear at all in this case).
  • We can likely do some iteration on the thickness of the left border. I chose 3px to start with.
  • As per @afercia's clarification in Consider a subtler hover state for blocks #14095 (comment), the blue color in our current hover state was not chosen due to accessibility concerns. Since we may have some flexibility on that color, I suggest we try a slightly lighter gray color for the hover indicator instead.
  • As raised by @LukePettway in Try a darker selected block outline with animation #13700 (comment), this hover treatment does not outline the entire interactive area. It also may be lost on very large screens. We should try this out and assess the impact of this.
  • When scrolling, the left border of the block can overlap with that of the block toolbar. Since they're using opacity, the overlap is visible if you look closely. We may want to try preventing that.

@kjellr kjellr added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. Needs Accessibility Feedback Need input from accessibility labels Feb 27, 2019
@kjellr kjellr self-assigned this Feb 27, 2019
@kjellr
Copy link
Contributor Author

kjellr commented Feb 27, 2019

... I suggest we try a slightly lighter gray color for the hover indicator instead.

For reference, here's how this PR looks with $light-gray-500 hovers, instead of the blue:

hover-select-gray-single


hover-select-gray


It feels nice and light to me, negating a bit of the left-heaviness I experienced in the original PR. It does make the hover less prominent, but that may be workable.

@chrisvanpatten
Copy link
Contributor

As you noted on the other ticket it is super hard to crawl through the history of this — I was even involved in some of these conversations and don't remember exactly where/when they happened 😂

However, I'm like 80% sure there was a reason that the block name label was moved to the top right, instead of top left. I think it was blocking interaction with the previous block, or maybe causing issues with the adjacent inserter?

cc @jasmussen or @ZebulanStanphill who I think were also in those issues, and might remember better! 🧠

@mapk
Copy link
Contributor

mapk commented Feb 27, 2019

I really appreciate the hover states not interfering with my view of the document as a whole. The heaviness on the left felt fine to me.

Initially, I thought it might look odd with the quote block, but it seemed fine.

screen shot 2019-02-27 at 8 29 35 am

I did notice some rough parts around the columns block, but what isn't rough there right now?

screen shot 2019-02-27 at 8 34 34 am

For reference, here's how this PR looks with $light-gray-500 hovers, instead of the blue:

Did you happen to test the a11y on the block label? Does the black on grey contrast work?

@kjellr
Copy link
Contributor Author

kjellr commented Feb 27, 2019

I did notice some rough parts around the columns block, but what isn't rough there right now?

Ah, good catch. Yeah, I need to adjust this for that block (and probably other blocks that support nesting, too).

Did you happen to test the a11y on the block label? Does the black on grey contrast work?

Yep, $dark-gray-900 text against a $light-gray-500 background has a pretty high 13.17:1 contrast ratio. That's actually a better ratio than what we have against the blue.

@ZebulanStanphill
Copy link
Member

@chrisvanpatten I don't recall anything specific for why the hover label was on the right, but it is important to note that in this PR, the label is shown above the block border, rather than within it. As long as the label doesn't remain on-screen when you hover over it, it shouldn't cause any issues with selecting the block above.

@jasmussen
Copy link
Contributor

However, I'm like 80% sure there was a reason that the block name label was moved to the top right, instead of top left. I think it was blocking interaction with the previous block, or maybe causing issues with the adjacent inserter?

The very first hover labels were horrendous. I can say that because I designed them, so I'm only offending myself. They were all sorts of in the way, covering sibling inserters and, yeah, it really helped making them small.

I believe we moved them to the right with the intent of making them into the block drag handle. But since then, obviously, the actual drag handle has landed. On the left. So I think it's okay to explore moving that label around.


@kjellr Thank you so very much for exploring this, head on. Building a block editor has definitely been a challenge, and although it's thrilling to see a thousand blocks actually blooming (this was always our greatest hope), along the way it's been incredibly arduous to try and find the right balance between being an editor, and being a block editor. I love that we are continuing to explore that, it can only improve things.

In this case, I also deeply love how you state at the top what problems you are trying to solve with this: the heaviness of the UI, and at the same time the lack of contrast with the selected block.

Having just played around with this a little bit in the branch, I feel you do accompolish the contrast issue, but that the heaviness is less successful. Like Mark says, it's nice for the document to feel continuous, but the wider border actually feels heavier to me than the border all the way around. It's hard to put into words, but it almost feels like the document is leaning towards the left now.

But the ingredients seem right. Specifically the left border on hover, sans color.

How would it look if the hover border remained 1px, but was $dark-gray-150 instead, and remained $dark-gray-150 when selected?

@afercia
Copy link
Contributor

afercia commented Feb 28, 2019

and remained $dark-gray-150 when selected?

🤔

@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Feb 28, 2019

@afercia Is that an "I'm interested to see how this would look" or a "This is probably a bad idea" thinking emoji?

Thanks for the history @jasmussen. I agree it feels a little heavy on the left on focus. I like the thick border on hover for sure… less sure about the focus state.

@afercia
Copy link
Contributor

afercia commented Feb 28, 2019

It's a thinking emoji. As in: it's basically what the accessibility feedback proposed since the beginning. As long as $dark-gray-150 gives a contrast of at least 3:1 with the background and it stays $dark-gray-150 without fading, the thinking emoji thinks it's interesting.

@folletto
Copy link
Contributor

folletto commented Mar 1, 2019

FWIW I appreciated the line-on-the-side idea even in the early explorations even if not all the pieces had yet fallen into place, so this might be a good time.

I also feel that this approach with the label on the left looks almost like that the label "expands" into the toolbar, which feels a subtle yet good correlation to me.

I'd also +1 to have the border gray (whichever specific gray is evaluated as appropriate). I agree the blue for hover feels off, as it signals "selected" more than "hover". I vaguely recall why it was a good idea at the time, and again I think now might be the right moment to switch :)

@kjellr
Copy link
Contributor Author

kjellr commented Mar 1, 2019

A couple quick notes:

Based on the feedback in this thread, I've updated this PR to use a $light-gray-500 hover state, as mocked up above:

53505809-9f0b7b00-3a82-11e9-8a3d-100e20d15df3

Give the PR another test and please share your impressions.


How would it look if the hover border remained 1px, but was $dark-gray-150 instead, and remained $dark-gray-150 when selected?

I've opened #14197 with a version of this idea. It uses a $dark-gray-300 left border (I had to darken it from your suggestion in order to get the breadcrumb to pass AA), alongside $dark-opacity-200 borders for the focus state. I adjusted the borders so that they surround the block and the block toolbar, but do not effect the dividers inside of the toolbar. Compared to earlier explorations, I think this works nicely to retain the dark focus state, while keeping the icons strong enough to not compete with the borders.

hover-focus-progression-current

Please head over to #14197 and give that a spin too. 👍

@afercia
Copy link
Contributor

afercia commented Mar 5, 2019

A more in depth discussion on the block breadcrumb is happening on #14095 and I'd agree it would be good to understand what problem these breadcrumbs solve in the first place. If they're going to stay, I'd like to share a couple thoughts:

When exploring the "navigation / edit mode" in #3195 / #5694 at some point it was clear there was the need for a <button> element in each block to switch between the two modes. This exploration hasn't seen much progress in the last months but it still might be one of the things to try to improve the overall keyboard interaction with the blocks. There's a chance something like that might be implemented in the future and just wanted to mention the "breadcrumb" could be used as the button we need to switch mode. Just a consideration I wanted to share, nothing actionable here at the moment.

Personally, I've never fully understood why the breadcrumbs are so small :) The look like something unfinished, stuck between the attempt to communicate the block type and the will to not "disturb" users too much. Being so small, their white space is inconsistent with the one used across the UI and they feel a bit extraneous to the overall UI look and feel. Why not consider to make them bigger? Other applications use similar overlays and they're not afraid to temporarily hide other parts of the UI. For example, even if it's a slightly different case, here's what Squarespace does:

squarespace quote

@youknowriad
Copy link
Contributor

I pushed a tweak to use a SASS variable to configure the size of the left border easily. I have a small preference for this approach personally.

The darker border around the toolbars and the blocks seem odd to me as the inner border of the toolbar are lighter.

Anyway, happy to go with what you all feel is best.

@kjellr
Copy link
Contributor Author

kjellr commented Mar 15, 2019

The permalink box is a bit offset. But you don't have to fix this, as the permalink box is going to be revisited separately. But figured I'd mention it regardless:

Yeah, I noticed that in Master too. 😕 I haven't looked into it yet, but I wonder if it has to do with Twenty Nineteen's editor styles. We can fix separately.

Screen Shot 2019-03-15 at 8 20 47 AM

I don't think we account for the hover state in master so this is technically not a regression. Can you think of a good way to make that hover state work better for inverted styles? If not, then it's not a blocker.

I've pushed c3b6c3b, which uses an alternate dark color for the hover + breadcrumb for dark themes:

dark-theme

There's a teensy bit of funkiness going on at the mobile breakpoint. Here's TwentyNineteen

Ah, right. Usually, the block borders extend to the screen edges on mobile. That doesn't happen on Twenty Nineteen (If I recall correctly, it's because Twenty Nineteen includes some margins around the editing area or something like that). Anyway, thanks for the heads up. This should be fixed in 9b5325f:

Screen Shot 2019-03-15 at 8 40 17 AM

@kjellr kjellr merged commit 08bf9e1 into master Mar 15, 2019
@kjellr kjellr added this to the 5.3 (Gutenberg) milestone Mar 15, 2019
@kjellr kjellr deleted the try/left-block-borders branch March 15, 2019 15:27
@jasmussen
Copy link
Contributor

🆒

@youknowriad
Copy link
Contributor

I just noticed that the post title hover and selected styles are not great in "Spotlight Mode"

@kjellr
Copy link
Contributor Author

kjellr commented Mar 19, 2019

@youknowriad can you share what you're seeing? I don't think there are supposed to be any hover style there in Spotlight Mode, and the select style should be just like all the other blocks in that mode:

hover-title

@youknowriad
Copy link
Contributor

Oh I have blue outlines, maybe it's related to the branch I was in. Sorry for the ping, I'll check deeper.

youknowriad pushed a commit that referenced this pull request Mar 20, 2019
* Add thick borders to the left of blocks when they're hovred + selected.

* Add thick left border to the page title.

* Turn off block toolbar centering for alignwide blocks.

This splits the left border in two, which looks a bit weird.

* Move block breadcrump to the left side, position it on top of the block.

* Clean up the block toolbar's left border.

* Use inset borders on mobile.

* Prevent inset borders from overlapping with full-bleed content.

* Use a gray border instead of a blue one on hover.

* Use a sass variable to define the left block border width

* Fix breadcrumb potision for alignfull blocks.

* Clean up breadcrumb position for left & right-aligned blocks.

* Sync block mover animation up with the hover state.

* Darken focused block borders slightly.

From $light-gray-500 to $light-gray-800.

* Switch to using border instead of outline for block borders.

Also, change the thick left border to a solid color, to prevent weird overlap.

* Make this work better with Windows High Contrast Mode

* Adjust z-index of border + breadcrumb for child blocks.

So that they're not overlapped by the parent block's border + toolbar.

* Remove extra z-index rule from the block border.

Turns out this wasn't needed anyway.

* Remove extra z-index rule from the block border. Minor description cleanup.

* Ensure these styles are compatible with Top Toolbar mode.

* Use the new gray value for the mobile toolbar border.

* Add a matching left border to the post permalink area above the title.

* Improve border position for mobile screens, especially for elements that float left/right.

* Remove a couple unnecessary border updates from 047e1e4.

Turns out these styles can be preserved on all screen sizes with no ill effect.

* Clean up bugs related to the hover + focus states of the classic editor block.

* Classic Block toolbar icon cleanup.

Even out margins, remove white background.

* Reusable Block border cleanup.

* Keeping a light border on the classic block when it's inactive.

* Clean up borders on warning blocks.

* Switch to a solid color border color for the permalink box.

This mirrors the approach we use for block toolbars, and also ensures that we don't layer opacities and make the permalink toolbar darker than intended.

* Update z-index rule name to match the one used in the latest merge.

* Combine full-wide toolbar centering rules.

Previously, these were declared in two separate palces.

* Add a darker hover state for dark themes.

* Remove the left toolbar border on mobile screens.

This fixes some visual bugs with themes like TwentyNineteen, which include margins on either side of the block on mobile.
youknowriad pushed a commit that referenced this pull request Mar 20, 2019
* Add thick borders to the left of blocks when they're hovred + selected.

* Add thick left border to the page title.

* Turn off block toolbar centering for alignwide blocks.

This splits the left border in two, which looks a bit weird.

* Move block breadcrump to the left side, position it on top of the block.

* Clean up the block toolbar's left border.

* Use inset borders on mobile.

* Prevent inset borders from overlapping with full-bleed content.

* Use a gray border instead of a blue one on hover.

* Use a sass variable to define the left block border width

* Fix breadcrumb potision for alignfull blocks.

* Clean up breadcrumb position for left & right-aligned blocks.

* Sync block mover animation up with the hover state.

* Darken focused block borders slightly.

From $light-gray-500 to $light-gray-800.

* Switch to using border instead of outline for block borders.

Also, change the thick left border to a solid color, to prevent weird overlap.

* Make this work better with Windows High Contrast Mode

* Adjust z-index of border + breadcrumb for child blocks.

So that they're not overlapped by the parent block's border + toolbar.

* Remove extra z-index rule from the block border.

Turns out this wasn't needed anyway.

* Remove extra z-index rule from the block border. Minor description cleanup.

* Ensure these styles are compatible with Top Toolbar mode.

* Use the new gray value for the mobile toolbar border.

* Add a matching left border to the post permalink area above the title.

* Improve border position for mobile screens, especially for elements that float left/right.

* Remove a couple unnecessary border updates from 047e1e4.

Turns out these styles can be preserved on all screen sizes with no ill effect.

* Clean up bugs related to the hover + focus states of the classic editor block.

* Classic Block toolbar icon cleanup.

Even out margins, remove white background.

* Reusable Block border cleanup.

* Keeping a light border on the classic block when it's inactive.

* Clean up borders on warning blocks.

* Switch to a solid color border color for the permalink box.

This mirrors the approach we use for block toolbars, and also ensures that we don't layer opacities and make the permalink toolbar darker than intended.

* Update z-index rule name to match the one used in the latest merge.

* Combine full-wide toolbar centering rules.

Previously, these were declared in two separate palces.

* Add a darker hover state for dark themes.

* Remove the left toolbar border on mobile screens.

This fixes some visual bugs with themes like TwentyNineteen, which include margins on either side of the block on mobile.
@irishetcher
Copy link

It's a bit better, but still not user friendly. On testing I accidentally dropped a Kadence row inside a Kadence row and was wondering why I couldn't make the inner one go fullscreen. It wasn't evident what had happened until I used the document drop down to see where I had gone. Then when I tried to move the row block down out of the outer row it stubbornly wouldn't nudge down the theme footer and ended up floating around after my cursor (with no click down on my mouse button). To resolve I had to scrap the rows and start over.

getdave added a commit that referenced this pull request Mar 28, 2019
As discussed this imposes on the Theme choices. We will make the Block more perceivable via:

* #14241
* #14145
getdave added a commit that referenced this pull request Mar 28, 2019
As discussed this imposes on the Theme choices. We will make the Block more perceivable via:

* #14241
* #14145
getdave pushed a commit that referenced this pull request Apr 1, 2019
* Add initial container block

Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>

* Add background color controls to the container block

Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>

* Add `anchor` support to container block

Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>

* Add option for editing container block padding

Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>

* Add padding with preset narrow and wide options

Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>

* Add padding toggle

Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>

* Ensure background color class name is set in block edit

Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>

* Switch padding option to disable padding instead of enabling

Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>

* Set a default background color

Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>

* Add e2e test for adding container block

* Add e2e tests for the container block

* updated the default toggle text and behaviour for the container's padding and fixed a class naming problem on the front end

* removed the padding toogle and added default padding to the container block wrapper class

* Remove container block margin when a background is set. Allows consecutive container blocks to have no gap between them

* Adds keywords to aid in discoverability

* Implement same padding rules as used in columns block for container block. Ensures block mover controls remain visible.

* Removes default background

As discussed this imposes on the Theme choices. We will make the Block more perceivable via:

* #14241
* #14145

* Renames Block from Container to Section

This is in response to the wealth of feedback (particularly from the Design team) that “Container” is not a good reflection of the purpose. Rather Section is the prefered option.

See #13964 (comment)

* Utilise correct spacing variable for consistency

* Resolve block validation error when custom background color used. Caused by customBackgroundColor not being defined as an attribute.

* Feature flag the section block

* Update e2e tests with renamed section block

* Add fixture for section block for full content test

* Adds alignment for inner blocks that support alignment controls

As per this comment #13964 (comment) the Block needs the ability to align the blocks within the Section. Currently only a few Blocks support this but a wider change can be added that will enable all inner blocks to be aligned. The alignment rules are tested against the [Gutenberg Starter Theme](https://github.com/WordPress/gutenberg-starter-theme) as this enqueues no editor styles and conforms to the guidlines laid out [here](#13964 (comment)).

@kjell has noted that Twenty Nineteen will need a patch for this, the styles for which have already been scoped out.
Note that this removes the previous fix for the block mover controls which are now (again) hidden when Blocks are full width. @kjell has confirmed this is a known issue that needs to be solved globally and should not be addressed in this PR.

* Fixes width of wide Media text Block within full width Section Block

* Removes Block default padding

Remove padding to ensure alignment is consistent with canvas by default. Plan to introduce an attribute to control this later.

* Fixes alignment layout within Seciton Blocks for all alignable Block types

Previously alignment CSS was too focused on images and had too many edge cases. Refactor to cater for all Block types and in turn simplified the CSS.

* Adds specificity required to only target immediate child Blocks of Section

* Reintroduces padding on request

* Adds fix to images as edge case

Previously a 1px gap was seen on the sides of images nested within Section.

* Removes unwanted whitespace top/bottom of Block

* Fixes wide child Block alignment within full width Section

Resolves issue reported at

#13964 (comment)

Also variablises the values.

* Fixes full width image alignment within full width Section.

Alters values required to ensure full width image Block meets edges of editor canvas when within a full width Section Block.

This removes the need for the “edge case” hack in the Image Block CSS which is good as this was never a good idea.

Resolves #13964 (comment)

* Removes usage of Block name as Sass variable

Not required.

Resolves #13964 (comment)

* Updates hard coded spacing to variables

* Fixes excessive Section Block padding on tiny screens

Resolves #13964 (comment)

* Updates to add padding only when background is added to Block

Addresses #13964 (comment)

* Fixes Block overlap between adjacent Blocks when no background applied

Resolves issue highlight at #13964 (comment)

* Restores fix for Columns Block to ensure move/drag handles visible in full width Section

Resolves #13964 (comment)

* Fixes 1px of overflow on full width blocks

Resolves #13964 (comment)

* Applies another fix to 1px overflow issue

Full width Blocks were causing a 1px overflow and thus a hoz scrollbar to appear. Required adjustments to margins and the “pull/push” values (left/width) appleid to full width child Blocks

* Makes Block reusable

Resolves part of #13964 (comment)

* Removes feature flag

Resolves #13964 (comment)

* Fixes overflow issues

Utilising width’s over 100% and negative left offsets were triggering hoz scrollbars.

Also added a patch for the Block insertion point offsets which were triggering scrollbars due to their overhanging -1px indent.

* Fix width of context toolbar causing overflow-x

When the Section Block is highlighted so that the contextual toolbar is displayed it can cause hoz scrollbars to appear when the viewport becomes narrow. This is a very difficult bug to spot but careful testing will reveal it.

The -1px compensates for the `left: 1px` value on the child element `.block-editor-block-toolbar`which is causing the scrollbar to appear.

* Updates Section padding values to match paragraph Block

Addresses #13964 (comment)

* Fixes failing Block transforms e2e test

* Fixes Block overflowing off screen on <600px full width Section

This was due to negative margins being applied too early. Turns out these were mirroring a rule already in place within the editor and didn’t need to be there anyway. Removing fixes the issue as the core rules are applied at the correct media query.

* Fix to make full width children full width when background padding added

When Section has a background padding is added meaning that full width child Blocks no longer span edge-to-edge. This fix adds a compensating factor to ensure an edge-to-edge layout within the editor.

* Remove superflous Section keyword

Resolves #13964 (comment)

* Removes superflous resuable setting

It’s already the default!

Resolves #13964 (comment)

* Updates to use `block-editor` package and avoid proxy

Resolves #13964 (comment)
@@ -424,9 +468,9 @@

// Full-wide
&[data-align="full"] {
// Position hover label on the right for the top level block.
// Position hover label on the left for the top level block.
Copy link
Member

@aduth aduth Apr 17, 2019

Choose a reason for hiding this comment

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

Should it be on the left, or on the right?

There are still existing styles which assume right-placement for the wide/full hover label:

// Position hover label on the right
> .block-editor-block-list__breadcrumb {
right: -$border-width;
}

I'm supposing that left placement is reasonable only in part due to the regression of #14817

Fixing #14817 introduces an undesirable overlap:

image

Copy link
Member

Choose a reason for hiding this comment

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

Fixing #14817 introduces an undesirable overlap:

Follow-up: #15022 (proposes moving to the right)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. [Release] Do Not Punt Used to indicate the issue or pull request must not be moved from the assigned milestone [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.