Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#35047 closed defect (bug) (fixed)

Notices are not moved to first header when header-elements are nested inside .wrap

Reported by: dvankooten's profile DvanKooten Owned by: afercia's profile afercia
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Administration Keywords: has-patch fixed-major
Focuses: ui, javascript Cc:

Description

Since [35238], notices are no longer inserted after the first header if this header is nested.

It is not uncommon for plugins to wrap the entire settings page in some kind of grid. In those cases, the first heading on that page is not a direct child of the .wrap element, resulting in the notices appearing at the far top of the page.

I propose to use find() instead of children(), as this uses the first heading on the page no matter its depth. This used to be the case before [35238] as well (.wrap h2:first).

Attachments (3)

35047.diff (712 bytes) - added by DvanKooten 9 years ago.
35047.2.diff (1.1 KB) - added by DvanKooten 9 years ago.
Use find in autosave.js as well, in line with how it was before.
35047.3.diff (1.1 KB) - added by afercia 9 years ago.

Download all attachments as: .zip

Change History (19)

@DvanKooten
9 years ago

#1 @DvanKooten
9 years ago

  • Keywords has-patch dev-feedback added

#2 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 4.4.1
  • Owner set to afercia
  • Status changed from new to assigned

@DvanKooten
9 years ago

Use find in autosave.js as well, in line with how it was before.

#3 @afercia
9 years ago

Sounds sensible but I'm not sure it can be done. Some plugins don't have a main heading at all and the first heading is actually somewhere in the middle of the page. In this case, notices would be appended in the middle of the page :(

This is probably one of those cases where having full backwards compatibility is nearly impossible. Thinking also at the new headings hierarchy in the admin screens, many plugins should update the headings in their screens anyways. I'd would strongly encourage plugin authors to provide a main H1 heading right after the .wrap element and if they have specific design needs they can always hide it with .screen-reader-text.

@DvanKooten any example of popular plugins where this is happening? @iseulde hi :) any thoughts?

#4 @DvanKooten
9 years ago

@afercia If a plugin has its heading halfway down the page then that is the problem THEY should fix. Right now, this is how it has always been done which makes it unlikely for this to cause any sudden problems anyway.

As for the h1 right after the .wrap element, in most cases this is the case (as in: the first visible element) but if it's nested inside any other elements the current code won't work.

It is happening in MailChimp for WordPress (mailchimp-for-wp) where the page starts with a full width breadcrumb after which the page is divided in a main column and a sidebar. I don't think this is uncommon for plugins to do at all.

Really hope this can get back in! :)

#5 @DvanKooten
9 years ago

In addition: since this uses :heading it is even less likely to cause problems with plugins without an early heading (which is an accessibility issue) since it responds to all headers.

#6 follow-up: @afercia
9 years ago

Thanks @DvanKooten so here's a screenshot to illustrate the issue, with notices from another popular plugin. WordPress 4.2 above and trunk below:

https://cldup.com/vHrKWQhdVS.png

After all, the previous jQuery selector was $firstHeading = $( 'div.wrap h2:first' ); and as @DvanKooten points out it was getting the first H2 regardless of how many level down in the DOM tree it was. So
$( '.wrap' ).find( ':header' ).first()
would basically restore the same behavior. Or maybe even better, simpler, and faster:
$( '.wrap h1, .wrap h2' ).first()

#7 @johnbillion
9 years ago

  • Version changed from trunk to 4.4

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


9 years ago

#9 @jorbin
9 years ago

  • Priority changed from normal to low

If this is going to be included in 4.4.1, it needs a new patch.

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


9 years ago

#11 @jorbin
9 years ago

  • Milestone changed from 4.4.1 to 4.5
  • Priority changed from low to normal

Moving out of 4.4.1, if a patch surfaces soon, we can consider it for 4.4.2

#12 in reply to: ↑ 6 @azaozz
9 years ago

Replying to afercia:

Thinking that $( '.wrap h1, .wrap h2' ).first() is best, closest to the previous behaviour. Worth a commit for 4.5 and also for 4.4.x.

@afercia
9 years ago

#13 @afercia
9 years ago

Refreshed patch to use $( '.wrap h1, .wrap h2' ).first().

Tested with a bunch of plugins that output notices in uncommon places, looks good to me:
https://wordpress.org/plugins/mailchimp-for-wp/
Event Organiser version 2.13.6
https://downloads.wordpress.org/plugin/event-organiser.2.13.6.zip
https://wordpress.org/plugins/wordpress-seo/
https://wordpress.org/plugins/my-calendar/

#14 @afercia
9 years ago

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

In 36134:

Admin: fix repositioning of notices when the first header is not an immediate children of .wrap.

Props DvanKooten for the initial patch.
Fixes #35047 for trunk.

#15 @afercia
9 years ago

  • Keywords fixed-major added; dev-feedback removed
  • Milestone changed from 4.5 to 4.4.1
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for for 4.4.x (there's no 4.4.2 milestone yet).

#16 @dd32
9 years ago

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

In 36144:

Admin: fix repositioning of notices when the first header is not an immediate children of .wrap.

Merges [36134] to the 4.4 branch.
Props afercia, DvanKooten.
Fixes #35047.

Note: See TracTickets for help on using tickets.