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

Site editor: show color warning if poor contrast colors are picked #29457

Closed
annezazu opened this issue Mar 1, 2021 · 13 comments · Fixed by #29573
Closed

Site editor: show color warning if poor contrast colors are picked #29457

annezazu opened this issue Mar 1, 2021 · 13 comments · Fixed by #29573
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@annezazu
Copy link
Contributor

annezazu commented Mar 1, 2021

Description

If you select a color for the site title that results in poor contrast, no notice is given. This is feedback from the FSE Outreach Program's second call for testing.

Step-by-step reproduction instructions

Please list the steps needed to reproduce the bug. For example:

  1. Use a block based theme and the latest version of Gutenberg to setup the FSE experience.
  2. Go to "Site Editor (beta)"
  3. Select the Site Title block and change the block settings so that the color contrast is poor for the text (or vice versa for the background).
  4. See no notice.

Expected behaviour

I expect a notice about poor contrast similar to what happens when you do so in the post/page editor:

Screen Shot 2021-03-01 at 3 53 24 PM

Actual behaviour

No warning given.

Screenshots or screen recording (optional)

contrast.warning.mov

WordPress information

  • WordPress version: 5.6.2
  • Gutenberg version: 10.0.2
  • Are all plugins except Gutenberg deactivated? Yes
  • Are you using a default theme (e.g. Twenty Twenty-One)? TT1 Blocks
@annezazu annezazu added [Type] Bug An existing feature does not function as intended [a11y] Color Contrast [Block] Site Title Affects the Site Title Block labels Mar 1, 2021
@Quintis1212
Copy link
Contributor

Hello ) I think I can fix this )

@Quintis1212
Copy link
Contributor

I see that notice about poor contrast and color options not connected , and they are called from react-dom :
Знімок екрана з 2021-03-02 11-09-08
Who can help to fix this ?What i must change in the code ?

@annezazu
Copy link
Contributor Author

annezazu commented Mar 2, 2021

@nosolosw perhaps you can share some insight? If not, no worries :)

@oandregal
Copy link
Member

Thanks for looking into this @Quintis1212 !

I took a quick look at this and I was able to reproduce with any block that uses the color panel we provide (for example, paragraph). Apparently, the post editor and the site editor use the same mechanism (this), so I'm not sure why the color contrast notice doesn't work for the site editor off the top of my head. Would that link be helpful to start to tracking things down, @Quintis1212?

@Quintis1212
Copy link
Contributor

Thank you for help @nosolosw ! I wil continue to fix this bug with help of your info )

@Quintis1212
Copy link
Contributor

Quintis1212 commented Mar 3, 2021

@nosolosw I finded out that there is a function which works incorrect :
path : gutenberg/packages/block-editor/src/utils/dom.js
`
export function getBlockDOMNode( clientId, doc ) {
console.log(clientId, doc )
console.log(document.getElementById( 'block-' + clientId ))
console.log(doc.getElementById( 'block-' + clientId ))
return doc.getElementById( 'block-' + clientId );
}

`

This function can not find element in site editor which must be checked by contrast :
Знімок екрана з 2021-03-03 16-00-03
And I do not know how to fix it ) Can you help me once more ?)

@oandregal
Copy link
Member

Ah, I see. The color contrast feature uses the DOM to find the block node. However, this is problematic here because the editor canvas and the sidebar live in different documents (the editor is iframed) ― that's why the colorsDetectionElement is never found here but it is in the post editor. Does this provide any help to continue investigating the fix? I'm going to pull @ellatrix thoughts on this as well.

@oandregal oandregal added [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") and removed [Block] Site Title Affects the Site Title Block labels Mar 4, 2021
@oandregal oandregal changed the title Site Title: Show color warning if poor contrast colors are picked Show color warning if poor contrast colors are picked Mar 4, 2021
@oandregal oandregal changed the title Show color warning if poor contrast colors are picked Site editor: show color warning if poor contrast colors are picked Mar 4, 2021
@oandregal
Copy link
Member

I've taken the liberty of exchanging "Site Title" for "Site Editor" from the issue title and the label, as I think it reflects the issue is wider and affects all blocks.

@aristath
Copy link
Member

aristath commented Mar 4, 2021

I had another ticket for this on #28782 (for global styles), closed as a duplicate.

@Quintis1212
Copy link
Contributor

Quintis1212 commented Mar 4, 2021

@nosolosw It seems we need to use refs for detecting elements colors , I can not save document object in local storage
path - gutenberg/packages/edit-site/src/components/block-editor/index.js
`

     const thisDOM = window.document;
localStorage.setItem('editor-dom', thisDOM);

`

path : gutenberg/packages/block-editor/src/utils/dom.js
`
export function getBlockDOMNode( clientId, doc ) {

        //setTimeout( ()=>{
	        //console.trace(1)
	        //console.log(clientId, doc )
	        //console.log('block-' + clientId )
	        //console.log(window.document)
	        //console.log(document.getElementById( 'block-' + clientId ))
	        //console.log(editorDOM().getElementById( 'block-' + clientId ))
	        const editorDOM = localStorage.getItem('editor-dom');
        24	console.log(editorDOM+"editor dom")
        25	console.log( document.getElementById( 'block-' + clientId ).bind(editorDOM)  )
	        return doc.getElementById( 'block-' + clientId );
        //}, 1000)
    }

`

Знімок екрана з 2021-03-04 13-36-13

Any ideas ?)

@Quintis1212
Copy link
Contributor

@annezazu , @nosolosw , @aristath please , can any one help me for rewiev for this issue - #29534 , there is no set uped reviewers by default ) This is just one line of code for enhancement accessibility )

@ItsJonQ
Copy link

ItsJonQ commented Mar 4, 2021

However, this is problematic here because the editor canvas and the sidebar live in different documents (the editor is iframed)

@nosolosw I remember seeing some refactors to swap document to something like node.ownerDocument.defaultView.
However, this specific getBlockDOMNode() call doesn't receive a node to check against 🤔 .


Thinking through my previous iframe + React + document related experience...
I believe the iFrame's document was stored somewhere in a root level Context. It would then be accessible to utility functions that needed to reference the (frame) document for DOM things.

Maybe something like this could work?

I'm just spitballing ideas :). I don't mean to step on anyone's toes if they're already working on a solution!

@ellatrix
Copy link
Member

ellatrix commented Mar 4, 2021

Looking into this

@priethor priethor added the [Status] In Progress Tracking issues with work in progress label Mar 18, 2021
@priethor priethor added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed [a11y] Color Contrast labels Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants