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: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (19)
#2
@
9 years ago
- Milestone changed from Awaiting Review to 4.4.1
- Owner set to afercia
- Status changed from new to assigned
#3
@
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
@
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
@
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:
↓ 12
@
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:
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()
This ticket was mentioned in Slack in #core-editor by afercia. View the logs.
9 years ago
#9
@
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
@
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
@
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.
#13
@
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/
Use find in autosave.js as well, in line with how it was before.