Make WordPress Core

Opened 2 years ago

Closed 11 months ago

Last modified 11 months ago

#56228 closed defect (bug) (fixed)

Add main H1 heading in the site-editor page

Reported by: afercia's profile afercia Owned by: joedolson's profile joedolson
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.3
Component: Editor Keywords: has-screenshots has-patch commit add-to-field-guide
Focuses: accessibility Cc:

Description

See related issue on the Gutenberg repository: https://github.com/WordPress/gutenberg/issues/42373

In the Site Editor page, the main H1 heading is a JavaScript component. It renders after the Navigation sidebar and after the toolbar in the header. See first attached screenshot.

On the PHP side, site-editor.php doesn't output any heading.

This isn't ideal, as the main H1 heading should be the first thing in the main content. See https://github.com/WordPress/gutenberg/issues/42373#issuecomment-1185565284

This is also inconsistent with the way the main H1 heading is output in the Post editor, where edit-form-blocks.php does output the main H1 heading.

Additionally, when JS is off:

  • the Post editor does show the H1 heading and a notice, see second screenshot
  • the Site editor doesn't show any content (besides the WP admin menu), see third screenshot

I'd like to propose to improve consistency and make the Site editor page output the main H1 and the no-js notice. This way consistency and user experience would be improved. More importantly, the main H1 would be placed in a more appropriate place.

Other headings in the Site Editor will be improved in https://github.com/WordPress/gutenberg/issues/42373

Attachments (12)

01 Screenshot 2022-07-15 at 15.33.44.png (75.4 KB) - added by afercia 2 years ago.
Site editor h1
02 Screenshot 2022-07-14 at 14.48.27.png (157.4 KB) - added by afercia 2 years ago.
Post editor no JS
03 Screenshot 2022-07-14 at 14.49.33.png (139.6 KB) - added by afercia 2 years ago.
Site editor no JS
56228.diff (1.6 KB) - added by afercia 2 years ago.
56228.1.diff (1.6 KB) - added by joedolson 17 months ago.
Refreshes patch
56228.2.diff (1.6 KB) - added by joedolson 12 months ago.
Refreshed
56228.3.diff (1.6 KB) - added by joedolson 12 months ago.
Patch that only handles fallback state
wp site editor no js.png (233.8 KB) - added by afercia 12 months ago.
Site Editor page with no JS on latest core/Gutenberg trunk
56228.4.diff (1.5 KB) - added by afercia 12 months ago.
site editor no js.png (86.7 KB) - added by afercia 12 months ago.
Site editor page with JS off
post editor no js.png (108.4 KB) - added by afercia 12 months ago.
Post editor page with JS off
add theme no js.png (86.5 KB) - added by afercia 12 months ago.
For comparison: Add theme page with JS off

Download all attachments as: .zip

Change History (58)

@afercia
2 years ago

Post editor no JS

@afercia
2 years ago

Site editor no JS

@afercia
2 years ago

#1 @afercia
2 years ago

  • Keywords has-patch added

56228.diff

  • adds a visually hidden H1 heading at the beginning of the main content
  • when JavaScript is off, the page will display a heading and a no-js notice
  • adds a filter for the no-js notice message

This is basically the same pattern already used for the Post Editor page.

This change should be coordinated with the changes for Gutenberg:

  • There are some minor CSS changes that needs to be done on the Gutenberg side.
  • The current H1 will be changed to H2.

See https://github.com/WordPress/gutenberg/issues/42373

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-editor by sabernhardt. View the logs.


23 months ago

#4 @sabernhardt
23 months ago

  • Keywords coordinate-with-gutenberg added

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


21 months ago

#6 @joedolson
21 months ago

  • Milestone changed from Awaiting Review to 6.2
  • Owner set to joedolson
  • Status changed from new to accepted

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


17 months ago

#8 @fencermonir
17 months ago

Test report of https://core.trac.wordpress.org/attachment/ticket/56228/56228.diff

Env

WordPress 6.1.1
PHP 7.4
Chrome 108.0.5359.124 (Official Build) (arm64)
macOS 13.1 (Ventura)
Theme: Twenty Twenty-Three
Gutenberg enabled
Plugin: no plugin
In wp-config.php: define( 'WP_DEBUG_LOG', true ); define( 'WP_DEBUG_DISPLAY', true );

Steps to Test

  1. Navigate to Appearance > Editor
  2. Open inspector in chrome browser and enable the Disable JavsScript option from preferences -> Debugger -> Disable JavsScript
  3. reload the editor page and it does not show either any title or notice.
  4. then update the files mentioned in the patch and follow the above 1-3 steps
  5. then it shows the title and a notice.

Expected Results (✅)

Show the title and notice on the editor page

Actual Results

The patch shows the title and notice on the editor page

Screenshot:

  1. before the patch is applied: https://d.pr/i/gayK1d
  2. after the patch is applied: https://d.pr/i/2MD7eJ
Last edited 17 months ago by fencermonir (previous) (diff)

#9 @zebaafiashama
17 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/ticket/56228

Environment

  • OS: Windows 22621.1105
  • Web Server: nginx/1.16.0
  • PHP: 8.1.9
  • WordPress: 6.1.1
  • Browser: Chrome 109.0.5414.74
  • Theme: Twenty Twenty-Two
  • Active plugins: N/A


Actual Results

  • Before adding the code if JavaScript is disabled then on site editor no notice or heading were showing.

-After adding the code problem solved and showing the heading and notice

Before adding code

https://d.pr/i/BrvTyG

After Adding code

https://d.pr/i/C6kbL7

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


17 months ago

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


17 months ago

@joedolson
17 months ago

Refreshes patch

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


17 months ago

#13 @afercia
17 months ago

  • Keywords needs-refresh added

The patch needs some refresh. Some CSS changed on the Gutenberg side after the introduction of the Site editor 'Browse mode'.

Also, it appears that Gutenberg overrides some core styles to change some general layout. It does that by targeting directly some elements, even when JS is off. Not sure that's ideal. for example:

html.wp-toolbar {
	background: $white;
	padding-top: 0;
}

At the very least, this rule should be applied only when JS is on. Same for other CSS rules in Gutenberg. Unfortunately, right now there's no CSS class set on the html element to target whether JS is on or off.

#14 @alexstine
17 months ago

Actually, I do not believe this should be implemented in PHP due to the fact JS is not yet initialized and Gutenberg built-in region navigation does not work until the application receives focus. I think it is okay to add the no JS heading via PHP but the actual heading 1 should be added in React so the proper region navigation hooks get initialized in React. This is currently a problem in the post editor that could use a follow-up TRAC ticket/issue to rethink our approach..

Thanks.

#15 @afercia
17 months ago

Please see my last comment on the related Gutenberg issue at https://github.com/WordPress/gutenberg/issues/42373#issuecomment-1418936670

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


16 months ago

#17 @rudlinkon
16 months ago

Could any tester test please with the refreshed patch?

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


16 months ago

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


16 months ago

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


16 months ago

#21 @hellofromTonya
16 months ago

  • Milestone changed from 6.2 to 6.3

With 6.2 Beta 4 (the last planned beta for 6.2) happening in a few minutes, moving this ticket to 6.3.

However, if this fix is essential, please move it back into the milestone before RC1 to give time to schedule another Beta release.

#22 @alexstine
16 months ago

I am still of the opinion that any UI elements should be handled within Gutenberg React and the no JS stuff can be handled in the core PHP side of things. This would greatly help with the overall accessibility of the editor since the heading 1 would load with React allowing us to control the UX.

Hoping to move this one forward for 6.3.

Thanks.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


15 months ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


12 months ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


12 months ago

#26 follow-up: @joedolson
12 months ago

  • Keywords commit added; coordinate-with-gutenberg needs-refresh removed

Heading hierarchy has been updated in https://github.com/WordPress/gutenberg/pull/51696.

This does not alter the state of the page if JS is disabled or unavailable.

I think that this should be committed as well, as it does no harm to the site editing experience and provides feedback to a user if the site editor fails to load for any reason.

@alexstine This is being implemented in PHP as an error state handler; it's not about navigating the site editor, but about providing context if the site editor does not load.

@joedolson
12 months ago

Refreshed

#27 @joedolson
12 months ago

  • Keywords commit removed

It's already inside hide-if-js; I need to look at this more carefully. Sigh.

Last edited 12 months ago by joedolson (previous) (diff)

@joedolson
12 months ago

Patch that only handles fallback state

#28 @joedolson
12 months ago

  • Keywords needs-testing added

Updated patch only handles fallback if no-js. This way we're completely separating the behavior between no-js and the site editor, but we'll have something if the editor fails.

#29 @rudlinkon
12 months ago

Thank you so much, @joedolson, for refreshing the patch. Could any previous or new testers please test it with the newly submitted patch?

#30 @joedolson
12 months ago

Test instructions:

1) Load the full site editor. Verify that there is one H1 heading.
2) Disable JS in your browser. Load the full site editor. Verify that there is one H1 heading and a notice indicating that JS is required.

#31 in reply to: ↑ 26 @alexstine
12 months ago

Replying to joedolson:

Heading hierarchy has been updated in https://github.com/WordPress/gutenberg/pull/51696.

This does not alter the state of the page if JS is disabled or unavailable.

I think that this should be committed as well, as it does no harm to the site editing experience and provides feedback to a user if the site editor fails to load for any reason.

@alexstine This is being implemented in PHP as an error state handler; it's not about navigating the site editor, but about providing context if the site editor does not load.

@joedolson Sorry, I just checked the patch and see the no JS fallback. I think for the post editor, the main heading 1 is actually outside of React context so it prevents JS events in React from working properly. Just wanted to make sure we were not about to make the same mistake twice. PHP should provide the fallback only, not the heading under all circumstances. My guess is, we did that to have easier access to the $post->post_type.

Line 313: https://core.svn.wordpress.org/trunk/wp-admin/edit-form-blocks.php

That's not just the fallback, both the heading 1 are defined there. With JS and without JS.

#32 follow-up: @joedolson
12 months ago

I think that you're saying that the change as in the latest patch is fine, but I'm not really sure. Can you clarify?

You may also be suggesting that we should change the block editor to match, but if so, that would need a separate ticket.

#33 in reply to: ↑ 32 @alexstine
12 months ago

Replying to joedolson:

I think that you're saying that the change as in the latest patch is fine, but I'm not really sure. Can you clarify?

You may also be suggesting that we should change the block editor to match, but if so, that would need a separate ticket.

@joedolson Yes, this patch is fine. I was simply trying to state why this proposed solution confused me. I should have just looked at the patch in the first place.

I will eventually log another ticket for the post editor so we can figure out a better way to handle the post editor heading 1.

Thanks.

#34 @joedolson
12 months ago

  • Keywords commit added; needs-testing removed

#36 @joedolson
12 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 56025:

Editor: Add no-js fallback for site editor.

Add a fallback condition with heading and error notice to handle a no JavaScript state for the site editor, comparable to what already exists in the post editor.

Props afercia, joedolson, fencermonir, zebaafiashama, alexstine, rudlinkon.
Fixes #56228.

@joedolson commented on PR #4692:


12 months ago
#37

Fixed in r56025

This ticket was mentioned in PR #4695 on WordPress/wordpress-develop by @ramonopoly.


12 months ago
#38

The changes in https://github.com/WordPress/wordpress-develop/commit/aef1cb9847e9e513e0f4a7a4fe93c54ddabf6168 have broken the site editor in view and editor mode.

It added an additional wrapper for the site-editor div, which breaks flexbox styling.

props @kevin940726 for finding the bug and proposing the fix

## Before

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/6458278/25987753-c3fe-4f0a-b954-e34307bf8b2e
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/6458278/4ba25f4f-2d1d-45b4-a540-541a14e7508c

## After
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/6458278/6375d30d-4855-4b6f-b072-6b3ff653025e
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/6458278/b816d86a-86cc-40b3-a4c6-d8fcca15afef

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/6458278/d7753bce-0718-4d31-9a85-a8bf8d8e4649

Trac ticket: https://core.trac.wordpress.org/ticket/56228

@ramonopoly commented on PR #4695:


12 months ago
#40

Is this specific to having gutenberg running? I was only testing against core, and didn't see this. Either way, no problem with it.

Thanks @joedolson!

We first noticed it on Gutenberg, but then tested on Core without the plugin active. I had to clean my env and recompile everything to replicate it.

@isabel_brison commented on PR #4695:


12 months ago
#41

Committed in r56029 / 9d4f4a9.

#42 @afercia
12 months ago

  • Keywords needs-patch added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version set to trunk

Thanks everybody for working on this. Maybe I'm missing something but I'm not sure we're seeing the same things. If you're testing with the Gutenberg wp-env by any chance, please remember that it runs an old version of WordPress so things may look a bit different.

Testing with:

  • latest WP trunk
  • latest Gutenberg plugin trunk

for me, the Site editor page with JS off still needs some work. See screenshot below.
Worth noting the change committed here should have been coordinated with the CSS changes from https://github.com/WordPress/gutenberg/pull/42495

I will take care of submitting a new PR in the Gutenberg repo that will need to be included in WP 6.3. Also core will need some small adjustments. Reopening.

Last edited 12 months ago by afercia (previous) (diff)

@afercia
12 months ago

Site Editor page with no JS on latest core/Gutenberg trunk

@afercia
12 months ago

#43 @afercia
12 months ago

  • Keywords has-patch added; needs-patch removed

56228.4.diff needs to be tested together with the changes from https://github.com/WordPress/gutenberg/pull/52376 in the Gutenberg plugin (those changes will need to be ported to core).

  • Removes a left padding from the Site editor and Post editor page, as it's no longer needed.
  • Removes the notice-alt class from the no-js notices as the page background color is now the default light grey. Instead, the notice-alt style is only meant to be used on pages with a white background.

To test:

  • Disable JS support in your browser.
  • Go to the Site editor page, to the Post editor page, and to the Add new theme page /wp-admin/theme-install.php.
  • Observe they look the same:
    • Visible heading at the top of the page.
    • An error notice below the heading.
    • Check the page footer is visible.

See screenshots below.

@afercia
12 months ago

Site editor page with JS off

@afercia
12 months ago

Post editor page with JS off

@afercia
12 months ago

For comparison: Add theme page with JS off

#44 @joedolson
11 months ago

  • Keywords commit added

This looks good to me, and PR52376 has been merged. Marking for commit.

#45 @joedolson
11 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 56187:

Editor: Fix layout of no-js state in site editor.

Remove padding and adjust classes to match current state of editor.

Props afercia, andraganescu, poena.
Fixes #56228.

#46 @stevenlinx
11 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.