Try alternate style to indicate selected blocks #3068

jasmussen · 2017-10-19T13:39:58Z

Description

This PR takes a first stab at some of the changes discussed/mocked up in #2983.

Basically it removes both the hover and selected styles for blocks, in favor of little floating mover/settings blobs on the side, and some subtler selection styles for a few blocks (separator, more, button).

Additionally, it changes the visual style of multi-selected blocks. Given incoming keyboard-multi-select features, this changes it so a multi selected block has the same color as highlighted text does. This uses a kind of magical CSS value, Highlight, which is documented in https://www.sitepoint.com/css-system-styles/. Video:

This property is quite magical, and actually changes along with whatever choice you make at the operating system level:

It's a CSS 2.1 property and I've tested this color and found it working all the way back to IE9 (didn't test further).

I'm marking this as "In Progress", as I'd like both some code reviews, as well as some experience reviews. How does this feel? What feels good? What feels not so good? What else needs to be done in this branch? Any experience regressions?

Keep in mind that this is only one aspect of the visual refresh, you can see a list of other complimentary tasks here: #2983 (comment)

How Has This Been Tested?

Tested the color effect in Safari, Firefox, Chrome both on Mac and Windows, as well as Edge and IE9-11 on Windows. Not that we need 9 support, I was just impressed that the CSS property worked all the way back for that one.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

youknowriad · 2017-10-19T14:33:07Z

editor/modes/visual-editor/style.scss

-		outline: 1px solid $light-gray-500;
-		transition: 0.2s outline;
+	// simpler style for a block that has cursor focus (but hasn't been selected)
+	&.is-selected .editor-block-mover:before,


We may want to unify the className used for the hover and selected, maybe something like is-ui-visible

youknowriad

My experience with this:

It feels and looks nice,
I don't feel the heavy blocks anymore.
It looks like it's one and unique content editable (even if it's not)
Sometimes it hard to know what are the limits where the inspector/block settings will apply. I don't feel it's blocking but maybe just something we'll get used to over time.

afercia · 2017-10-19T15:00:13Z

To my knowledge, system colors are deprecated in CSS3 and there's no guarantee they will work in the future.

https://www.w3.org/TR/css3-color/#css-system

CSS2 system colors
Deprecated.

Note. The CSS2 System Color values have been deprecated in favor of the CSS3 UI ‘appearance’ property. If you want to emulate the look of a user interface related element or control, please use the ‘appearance’ property instead of attempting to mimic a user interface element through a combination of system colors.

mtias · 2017-10-19T15:14:38Z

editor/modes/visual-editor/style.scss

@@ -284,6 +297,29 @@
 	}
 }

+// show a separate cursor/caret highlight style for when an item was selected by the mouse or keyboard (but isn't multi selected)


How would this scale? Is this something a block author should be able to opt into? What are the reasons for a block to be part of this group?

mtias · 2017-10-19T15:17:07Z

Let's try this. I think we can get it for 1.5 and get some feedback. I'd like to test it before getting the docked toolbar PR merged.

afercia · 2017-10-19T15:24:26Z

Let's try this.

Sorry I can't understand why you want to merge an officially deprecated CSS feature.

jasmussen · 2017-10-19T15:29:57Z

Thanks for the reviews.

Sometimes it hard to know what are the limits where the inspector/block settings will apply. I don't feel it's blocking but maybe just something we'll get used to over time.

How would this scale? Is this something a block author should be able to opt into? What are the reasons for a block to be part of this group?

Answering these together, because they are related. The answer is it doesn't scale well, and I'm still thinking about the best way to do this.

The idea is that when you have the cursor in text, the cursor itself is an indication of what is your focus. We need something like that, for when your "focus selection" is on something non text, like a separator — just to help you keep your place as to what's selected.

There's also the thing that when we get across block keyboard selection with shift, the boundaries between paragraph blocks become less important.

Whether the block itself is important to show the boundaries of, remains to be seen, which is why I asked for your thoughts. I think if we can do some improvements around multi selection (see the recent keyboard selection discussion), perhaps clicking the ellipsis or mover also selects the block (as in multi select indication), which will show the boundaries clearly.

jasmussen · 2017-10-19T15:36:45Z

To my knowledge, system colors are deprecated in CSS3 and there's no guarantee they will work in the future.

I added a fallback color. If for whatever reason the Highlight color won't work, the background will be used instead.

Alternatively we can change the color of the selection using ::selection? But this seems like a better approach.

pento · 2017-10-19T16:16:17Z

I rebased this, because it happened to be branched when master was going through an awkward buggy phase.

afercia · 2017-10-19T17:18:33Z

I added a fallback color. If for whatever reason the Highlight color won't work, the background will be used instead.

That's better 🙂 However, in my mind "deprecated" means "no use", especially in a new software. Note the Sitepoint post you linked to is from August 11, 2009.

jasmussen · 2017-10-20T10:50:57Z

I understand the concern with using deprecated properties, but in this case we have fallback plans and alternatives, should they stop working. And while they are working, it seems like a nice usability win to use system colors.

jasmussen · 2017-10-23T13:09:25Z

With the recent changes above, I believe this PR is ready for final review. As it won't go in 1.5, and folks are all busy getting that ship shape, I won't ping anyone, but anyone receiving these notifications, feel free to review when you're ready!

ellatrix

This looks good, after a rebase :)

jasmussen · 2017-10-24T13:47:02Z

Thanks Ella for the review and thumbs up.

The rebase was a bit tricky — my own fault for making so many commits — but could I ask for your eyes on this another time just to verify it's okay?

It's important for some blocks, if we are to look at removing this entirely, let's do that as a separate PR. This does tweak the width to 1px though.

mtias · 2017-10-25T13:45:29Z

Rebased to pull #2988.

codecov · 2017-10-25T13:54:53Z

Codecov Report

Merging #3068 into master will decrease coverage by 0.84%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3068      +/-   ##
==========================================
- Coverage   31.66%   30.81%   -0.85%     
==========================================
  Files         217      217              
  Lines        6263     6529     +266     
  Branches     1112     1199      +87     
==========================================
+ Hits         1983     2012      +29     
- Misses       3600     3756     +156     
- Partials      680      761      +81

Impacted Files	Coverage Δ
editor/modes/visual-editor/block.js	`0% <0%> (ø)`	⬆️
blocks/library/code/index.js	`50% <0%> (-7.15%)`	⬇️
blocks/editable/format-toolbar/index.js	`5.19% <0%> (-1.19%)`	⬇️
editor/block-toolbar/index.js	`0% <0%> (ø)`	⬆️
blocks/library/shortcode/index.js	`50% <0%> (ø)`	⬆️
editor/writing-flow/index.js	`0% <0%> (ø)`	⬆️
blocks/library/image/block.js	`0% <0%> (ø)`	⬆️
blocks/editable/index.js	`11.96% <0%> (+1.53%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e984b0...118f489. Read the comment docs.

mtias · 2017-10-25T13:56:59Z

editor/modes/visual-editor/block.js

@@ -283,6 +283,7 @@ class VisualEditorBlock extends Component {
 			], this.props.order + 1 );
 		}
 		this.removeOrDeselect( event );
+		this.maybeStartTyping( event );


This is just to see how it feels—I like that arrow keys trigger "typing" mode now. But we'd want to exclude modifier keys, etc. cc @iseulde @mcsf

Pushed 118f489 to consolidate keydown handling, targeting arrow keys explicitly for triggering typing mode.

This reverts some previous refactoring that had split out removeOrDeselect to a separate function, but I think the switch is a bit easier to reason about consistently. Otherwise maybe some mapping of keys and conditions (isTarget) via something like cond but seems overkill and prone to a performance impact on this performance-critical code.

Looks good to me.

mtias · 2017-10-27T13:23:52Z

Let's 🚢 this.

aduth · 2017-11-09T15:13:01Z

Noted today that the multi-select system highlight color is pretty extreme in Firefox (56.0.2, macOS 10.12.6)

jasmussen · 2017-11-09T16:30:44Z

That is pretty funky indeed. Is that the stable channel? What's the normal selection color?

afercia · 2017-11-09T16:46:09Z

Firefox on Windows (IE 11 and Edge use the same color, Chrome a lighter one):

aduth · 2017-11-09T16:47:39Z

The normal selection color in Firefox is more reasonable, only the darker blue when multi-select takes effect.

jasmussen · 2017-11-09T17:21:36Z

I'll revisit. Thanks for the update.

jasmussen added the [Status] In Progress Tracking issues with work in progress label Oct 19, 2017

jasmussen self-assigned this Oct 19, 2017

jasmussen requested review from karmatosed, youknowriad, mtias, aduth and ellatrix October 19, 2017 13:39

youknowriad reviewed Oct 19, 2017

View reviewed changes

mtias reviewed Oct 19, 2017

View reviewed changes

pento force-pushed the try/alternate-block-style branch from 5da160b to c3b07eb Compare October 19, 2017 16:15

jasmussen removed the [Status] In Progress Tracking issues with work in progress label Oct 20, 2017

ellatrix approved these changes Oct 24, 2017

View reviewed changes

jasmussen force-pushed the try/alternate-block-style branch from c312dd1 to c48d4cb Compare October 24, 2017 13:46

karmatosed approved these changes Oct 24, 2017

View reviewed changes

Joen Asmussen added 5 commits October 25, 2017 15:15

First pass at alternate block style.

b5036a3

Polish the hovered mover and settings.

728c2d4

Try a subtle "text focus" color for some blocks.

ed0bf6a

Add normal color as fallback for Highlight.

f47f3b9

First pass at alternate block style.

f50e4ba

Joen Asmussen added 5 commits October 25, 2017 15:15

Polish the hovered mover and settings.

e5b91ab

Restore the boundary for a selected block.

a42d5a6

It's important for some blocks, if we are to look at removing this entirely, let's do that as a separate PR. This does tweak the width to 1px though.

Improve and polish the position of movers and settings.

b48efaa

Fix issues with prompt and full-wide ellipsis.

fa3a1bf

Fix rebase issues.

00366f7

mtias force-pushed the try/alternate-block-style branch from c48d4cb to 00366f7 Compare October 25, 2017 13:15

Count arrow keys for isTyping.

3332ed8

mtias reviewed Oct 25, 2017

View reviewed changes

Block: Consolidate keydown event handling

118f489

aduth approved these changes Oct 27, 2017

View reviewed changes

mtias added [Feature] Blocks Overall functionality of blocks General Interface Parts of the UI which don't fall neatly under other labels. Design labels Oct 27, 2017

mtias merged commit efa08a0 into master Oct 27, 2017

mtias deleted the try/alternate-block-style branch October 27, 2017 14:04

jasmussen mentioned this pull request Nov 10, 2017

Try alternate highlight color #3425

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try alternate style to indicate selected blocks #3068

Try alternate style to indicate selected blocks #3068

jasmussen commented Oct 19, 2017 •

edited

Loading

youknowriad Oct 19, 2017

youknowriad left a comment

afercia commented Oct 19, 2017

mtias Oct 19, 2017

mtias commented Oct 19, 2017

afercia commented Oct 19, 2017

jasmussen commented Oct 19, 2017

jasmussen commented Oct 19, 2017

pento commented Oct 19, 2017

afercia commented Oct 19, 2017

jasmussen commented Oct 20, 2017

jasmussen commented Oct 23, 2017

ellatrix left a comment

jasmussen commented Oct 24, 2017

mtias commented Oct 25, 2017

codecov bot commented Oct 25, 2017 •

edited

Loading

mtias Oct 25, 2017

aduth Oct 26, 2017

mtias Oct 27, 2017

mtias commented Oct 27, 2017

aduth commented Nov 9, 2017

jasmussen commented Nov 9, 2017

afercia commented Nov 9, 2017

aduth commented Nov 9, 2017

jasmussen commented Nov 9, 2017

Try alternate style to indicate selected blocks #3068

Try alternate style to indicate selected blocks #3068

Conversation

jasmussen commented Oct 19, 2017 • edited Loading

Description

How Has This Been Tested?

Checklist:

youknowriad Oct 19, 2017

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

afercia commented Oct 19, 2017

mtias Oct 19, 2017

Choose a reason for hiding this comment

mtias commented Oct 19, 2017

afercia commented Oct 19, 2017

jasmussen commented Oct 19, 2017

jasmussen commented Oct 19, 2017

pento commented Oct 19, 2017

afercia commented Oct 19, 2017

jasmussen commented Oct 20, 2017

jasmussen commented Oct 23, 2017

ellatrix left a comment

Choose a reason for hiding this comment

jasmussen commented Oct 24, 2017

mtias commented Oct 25, 2017

codecov bot commented Oct 25, 2017 • edited Loading

Codecov Report

mtias Oct 25, 2017

Choose a reason for hiding this comment

aduth Oct 26, 2017

Choose a reason for hiding this comment

mtias Oct 27, 2017

Choose a reason for hiding this comment

mtias commented Oct 27, 2017

aduth commented Nov 9, 2017

jasmussen commented Nov 9, 2017

afercia commented Nov 9, 2017

aduth commented Nov 9, 2017

jasmussen commented Nov 9, 2017

jasmussen commented Oct 19, 2017 •

edited

Loading

codecov bot commented Oct 25, 2017 •

edited

Loading