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

Fixes #15270. Removes the custom highlight on selected text. #15640

Closed
wants to merge 1 commit into from

Conversation

mapk
Copy link
Contributor

@mapk mapk commented May 14, 2019

Description

Fixes #15270. Removes the custom light blue highlight on selected text.

How has this been tested?

Tested locally on both Chrome and Firefox.

Screenshots

CURRENT

highlight-old

PROPOSED

highlight-new

Types of changes

Removed the CSS that added a custom highlight to selected text. Now Gutenberg just relies on the native browser highlight.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@mapk mapk self-assigned this May 14, 2019
@mapk mapk added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label May 14, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It looks like it covers what was discussed in the parent issue 👍

@gziolo gziolo added this to the 5.8 (Gutenberg) milestone May 15, 2019
@jasmussen
Copy link
Contributor

The unfortunate side-effect is that multi-selection now doesn't match the selection of text. Please also test how this looks in dark themes or themes with different background colors.

@mapk
Copy link
Contributor Author

mapk commented May 15, 2019

The unfortunate side-effect is that multi-selection now doesn't match the selection of text.

I noticed this as well, @jasmussen. When I removed the light blue from the multi-selected styles, I didn't see a default highlight from the browser, so I left it in. I can change the color to a "grey" but not sure if all browsers use the same grey highlight.

Do you have some thoughts on this?

@mapk
Copy link
Contributor Author

mapk commented May 15, 2019

Please also test how this looks in dark themes or themes with different background colors.

Thanks for the reminder! I tested in both Chrome and FF with a dark theme.

screencast 2019-05-15 07-49-07

@jasmussen
Copy link
Contributor

When I removed the light blue from the multi-selected styles, I didn't see a default highlight from the browser, so I left it in. I can change the color to a "grey" but not sure if all browsers use the same grey highlight.

Correct, because this is a selection that we create using JavaScript.

Do you have some thoughts on this?

This was discussed when we initially worked on this, and it was the reason why we chose to implement using the ::selection property. The intention wasn't to be fancy, or to break accessibility, it was to provide a consistent selection experience across text and blocks.

The only other way to accomplish this is to use a deprecated CSS 2.1 property called Highlight, whic doesn't work in Firefox. Here's a codepen showing that you can actually tap into the operating system provided highlight color and use it in CSS. But as mentioned, it won't work in Firefox, and it's also a deprecated property.

Those are the two approaches that I'm aware of, that allows us to have consistent select-across-blocks styles.

@ellatrix
Copy link
Member

If I remember correctly, the selection was set to transparent when there is multi selection. In the long term, we should try to avoid leaving any selection at all when there is multi selection.

@mapk
Copy link
Contributor Author

mapk commented May 15, 2019

Correct, @ellatrix. I still left the transparent selection there in this PR.

&.is-multi-selected *::selection { 
 		background-color: transparent; 
} 

For reference, here is the Sass (prior to this PR) being discussed.

/**
* Cross-block selection
*/
.block-editor-block-list__layout .block-editor-block-list__block {
::-moz-selection {
background-color: $blue-medium-highlight;
}
::selection {
background-color: $blue-medium-highlight;
}
// Selection style for multiple blocks.
&.is-multi-selected *::selection {
background-color: transparent;
}
&.is-multi-selected .block-editor-block-list__block-edit::before {
background: $blue-medium-highlight;
// Use opacity to work in various editor styles.
mix-blend-mode: multiply;
// Collapse extra vertical padding on selection.
top: -$block-padding;
bottom: -$block-padding;
.is-dark-theme & {
mix-blend-mode: soft-light;
}
}
}

@mapk
Copy link
Contributor Author

mapk commented May 15, 2019

Those are the two approaches that I'm aware of, that allows us to have consistent select-across-blocks styles.

Well, let's definitely not use deprecated CSS properties.

So the main issue outlined appears to be the custom highlight doesn't show in Windows High Contrast mode in Firefox.

[comment]

For Firefox users who have low-vision and use a Windows high-contrast theme, custom selection colors are not honoured

But the default browser highlight might work? I think that's what's being said?

[comment]

Leaving selection to the default will Just Work.

So if we accept this PR and use the browser default for highlighting text, it should solve this a11y issue, but it introduces a new issue that highlighting text (grey background) has a different selection color than when highlighting blocks (light blue background). And browsers don't have a default for this action so we need to provide some sort of indicator.

@enriquesanchez and @LukePettway can either of you provide some suggestions from an a11y viewpoint? How can we keep both text highlight and block highlight colors the same?

@jasmussen
Copy link
Contributor

Took a brief look at this. It seems that as soon a ::selection style is made, Firefox in Windows 10 bows out in high contrast mode. The media query that indicates whether high contrast mode is enabled or not has been deprecated, so while it would work today, that's also not a solid option. Is it possible to get the selection color via JavaScript so we can apply that to multi-selections? Don't think it is. Alternately instead of applying a CSS class, is-multi-selected to cross-block selections, can we actually in JS apply a selection? I.e. selection.setStart() etc around selected blocks? Or would that provide incorrect copy/delete data? CC: @ellatrix, any ideas?

Plan C is to redesign the block selection style to be less that of text, and perhaps more that of selecting objects. Think the old hover style.

@enriquesanchez
Copy link
Contributor

enriquesanchez commented May 20, 2019

can either of you provide some suggestions from an a11y viewpoint? How can we keep both text highlight and block highlight colors the same?

@mapk It's generally not recommended to change the text selection color, as some users may customize this to suit their specific needs. I would advise in favor of using browser defaults.

As for keeping both text and block selection colors the same, I think @jasmussen's idea of applying selection with JS is a good approach. I assume we would be able to set all of the text/contents inside a block to be selected and still leave the styling to the browser.

@jasmussen
Copy link
Contributor

To clarify, the js selection idea is a last ditch attempt, an I'm honestly a little skeptical that it will work, unfortunately.

@ellatrix
Copy link
Member

I don't fully get the problem. I don't see anything wrong with this patch.

If you'd like to change the colour of multi selection as well, you could try taking the text colour in JS and calculate the inverse colour. Currently there's no logic in place to set selection across blocks, but sure it could be done. The selection will be "read-only". Copy is overridden to set block content.

I think this patch is good on its own though?

@jasmussen
Copy link
Contributor

If you'd like to change the colour of multi selection as well, you could try taking the text colour in JS and calculate the inverse colour

I love the sound of that. Is it possible to explore this in a codepen or something, before we merge this? While I wouldn't fundamentally object to merging this PR, and I acknowledge the ticket needs fixing, I would love for us to have some idea of how we can make some connection with the multi select style before we do so. The plan B I outlined, a different multi select style, is also not the end of the world, but what you describe here sounds compelling.

@mapk
Copy link
Contributor Author

mapk commented May 24, 2019

Would either of you (@ellatrix @jasmussen) want to pair on this with me? I'm not sure how inversing the text color would match the default browser selection color. Maybe I'm missing something?

@jasmussen
Copy link
Contributor

I just realized that the following might not be what I thought it was:

If you'd like to change the colour of multi selection as well, you could try taking the text colour in JS and calculate the inverse colour

The inverse text color would definitely be sufficient contrast for multi selection. But it would not be the selection color, which is what I would hope we could calculate in JS. Thinking about it more, this is probably not possible, simply.

Which means the next step, likely, is to redesign the multi selection style.

I will put on my todo list to think about how that could look, and as it stands I'll probably look at how Figma selects multiple blocks for inspiration. Which will feel substantially less like selecting text, but — it seems like a total redesign is necessary unless we can grab the text selection color in JS.

@jasmussen
Copy link
Contributor

Here's a quick stab. Selected text:

Text Selection

Multi selection:

New Multiselect

Don't mind the different select style — these are based on an older mockup template. But the idea is that, since we can't grab the selection color of the system. we have to have a different selection style for blocks. This style is inspired by Figma, where a blue border is painted around the selected block, and any text inside is underlined in blue as well.

@ellatrix
Copy link
Member

@jasmussen You're right, I was thinking about something else. :) Not sure if it's possible to calculate the colour.

In that case I like your earlier suggestion of letting native selection happen and also drawing the block boundaries around the selected block. Perhaps that's a bit too big of a jump to take in one go, so we could just try to programatically set browser selection after the blocks have been selected. (I think this is what you meant as well.) I think this is easily doable, but I don't have the time right now to try it. Can maybe look into it next week.

@jasmussen
Copy link
Contributor

In that case I like your earlier suggestion of letting native selection happen and also drawing the block boundaries around the selected block. Perhaps that's a bit too big of a jump to take in one go, so we could just try to programatically set browser selection after the blocks have been selected. (I think this is what you meant as well.) I think this is easily doable, but I don't have the time right now to try it. Can maybe look into it next week.

🎉

All of the above sounds superb. I would be very grateful if you could take a stab.

@nerrad nerrad removed this from the Gutenberg 5.9 milestone Jun 10, 2019
@ellatrix
Copy link
Member

@jasmussen I quickly tried what I described above. It kind of works...

Screenshot 2019-07-31 at 13 44 03

The problem is that it's a big buggy across different browsers. Some browsers like Chrome don't paint any selection inside contenteditable child elements. The only solution I can think of is to turn off contenteditable during multi-selection, and restore it with normal selection. I have a PR coming up which we can test a bit.

@mapk
Copy link
Contributor Author

mapk commented Jan 7, 2020

Long time, no see. Looks like that last merge fixed the original issue. I'm closing this one out.

@mapk mapk closed this Jan 7, 2020
@youknowriad youknowriad deleted the update/remove-custom-highlight branch May 27, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blocks use custom selection color
6 participants