Make WordPress Core

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#46371 closed defect (bug) (fixed)

Make sure in-page anchors are not hidden behind top bars

Reported by: afercia's profile afercia Owned by: joedolson's profile joedolson
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Toolbar Keywords: has-screenshots has-patch has-dev-note
Focuses: ui, accessibility, css Cc:

Description

Moved from https://github.com/WordPress/gutenberg/issues/10101

This issue always existed but it's more evident now that the new Block Editor (Gutenberg) supports in-page anchors for headings.

When the admin bar is visible, following a link to an in-page anchor works but the targeted element is hidden behind the WordPress admin bar (see attached screenshot).

Similarly, bundled themes that have a sticky top bar should take care of this.

As mentioned in the Gutenberg GitHub issue, there's a simple CSS technique to address this problem (using the :target selector) and turns out it's already used on wp.org. For example, follow this link: https://make.wordpress.org/core/handbook/best-practices/commit-messages/#guidelines

Attachments (6)

anchor link behind toolbar.png (272.1 KB) - added by afercia 6 years ago.
Example of a heading with anchor hidden behind the WP admin bar
46371.diff (2.6 KB) - added by kingkero 5 years ago.
46371.1.diff (703 bytes) - added by audrasjb 5 years ago.
Don’t hide content anchors with WP Admin Bar
46371.2.diff (506 bytes) - added by sabernhardt 4 years ago.
adding scroll-padding-top in the stylesheet
43671.3.diff (1.8 KB) - added by sabernhardt 3 years ago.
using toolbar height variable and adjusting scroll padding for bundled themes
43671.4.diff (3.1 KB) - added by sabernhardt 3 years ago.
removing extra padding from Twenty Fourteen

Download all attachments as: .zip

Change History (50)

@afercia
6 years ago

Example of a heading with anchor hidden behind the WP admin bar

#1 @desrosj
5 years ago

  • Keywords good-first-bug added

I'm marking this as good-first-bug.

This ticket was mentioned in Slack in #core-committers by kingkero. View the logs.


5 years ago

@kingkero
5 years ago

#3 @kingkero
5 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

In the latest Gutenberg master version, Headings and Groups can have anchors. So I added rules for those two via the CSS directive :target. Needs to be in 2 places for each files as the admin bar has different heights for different screens. For smaller screens the bar is not fixed.

#4 @SergeyBiryukov
5 years ago

  • Milestone changed from Future Release to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@audrasjb
5 years ago

Don’t hide content anchors with WP Admin Bar

#5 @audrasjb
5 years ago

  • Keywords needs-refresh added; needs-testing removed

#6 @audrasjb
5 years ago

  • Keywords needs-refresh removed

Hi,

I refreshed the initial patch.
@kingkero there's no need to update RTL stylesheets since they are generated automatically.

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


5 years ago

#8 @audrasjb
5 years ago

Hi @SergeyBiryukov :) Do you think this one is ready be committed in 5.3?

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


5 years ago

#10 @audrasjb
5 years ago

  • Keywords commit added

#11 @afercia
5 years ago

  • Keywords commit removed

the admin bar has different heights for different screens. For smaller screens the bar is not fixed.

@kingkero good point thanks. Between 782 pixels and 600 pixels, the admin bar is taller (46 pixels vs. 32) and still fixed at the top. We should add a rule also in the related media query.

#12 @afercia
5 years ago

Testing a bit the patch I'm not fully convinced it's there :) The main concern is that targeted elements will have an altered top margin and padding. This value will likely alter the one provided by the team in use, potentially introducing breakages.

To recap:

  • the added spacing should be _additional_ to the one provided by the active theme: not sure how to implement it but I'd recommend to explore a new approach
  • with a viewport width of max 600 pixels, the patch shouldn't do anything
  • between 600 and 782 pixels the added spacing should be 46 pixels
  • above 782, it should be 32 pixels
  • maybe consider to use just .entry-content :target to set the rule to any anchor, also the ones manually added within the content

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


5 years ago

#14 @audrasjb
5 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from 5.3 to Future Release

This ticket still needs some work and improvements. Moving it to future release.

#15 follow-up: @dufresnesteven
5 years ago

The main concern is that targeted elements will have an altered top margin and padding.

What about using something like:

....:target:before {
   content: '';
   display block;  
   height: 32px; // Height of the header
}

It won't collide with element styles.

#16 @kingkero
5 years ago

Recently we used scroll-padding-top (MDN, caniuse) for a similar scenario.

While it does not have as great support as :target does, it at least solves the problem with the extra space (as there is none). I could work up some examples with that if you're interested.

#17 in reply to: ↑ 15 @thimalw
4 years ago

Replying to dufresnesteven:

It won't collide with element styles.

Good way of thinking, but unfortunately this still collides with the element styles. Say a theme has a style applied to the :before pseudo element of the target element, then this would cause a problem.

scroll-padding-top as suggested above seems to do the trick. But I'm not sure whether it has enough support to be used in this case.

#18 @sabernhardt
4 years ago

Should the following two tickets be closed in favor of this one?

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


4 years ago

@sabernhardt
4 years ago

adding scroll-padding-top in the stylesheet

#20 @sabernhardt
4 years ago

  • Focuses css added
  • Keywords good-first-bug 2nd-opinion removed

The scroll-padding-top value should be better within the stylesheet to give spacing on the admin side, too. Currently, clicking the "Skip to main content" link hides the Screen options and Help buttons behind the toolbar. (I realized that after @ryelle mentioned moving the height variable in #52623.)

So 46371.2.diff revises @Nyiriland's patch from #51910.

Firefox scrolls a little extra for the Continue reading (read more) link targets. I also checked in Chrome and Edge (Windows 10).

The scroll padding seems to work with most bundled themes. It does not have enough spacing for link targets in Twenty Sixteen because of the 21px pseudo-element "border" on larger screens, but it's closer. The CSS variable from #52623 might help with that and similar themes later.

#21 @sabernhardt
4 years ago

Before testing these links on the front end, make sure the admin toolbar shows. If not, view your profile to check the "Show Toolbar when viewing site" setting checkbox.

Link targets within the page/post

(adapted from #43452 description - props @kokkieh and @designsimply)

  1. Create a new post (or page).
  2. Add some text, at least a few paragraphs.
  3. Add a Group or Heading block and assign an HTML anchor under "Additional" section in sidebar menu. (If using the Classic editor, you could add an id attribute to some text that appear below the fold. If you have a plugin for inserting footnotes, that would create both this target and the link in step 4.)
  4. Add a link in one of the earlier paragraphs targeting the id.
  5. Publish the post, view, and click the link.
  6. The target (whether Group, Heading, footnote, etc.) should appear directly below the admin toolbar.

Continue Reading links (More block/tag)

  1. If using Twenty Twenty or Twenty Twenty-One, check your Theme Options ("Excerpt Settings" in TT1) to make sure the archive pages show Full Text instead of Summary.
  2. Create a new post.
  3. Add some text, at least a few paragraphs.
  4. Add a More block (or Read More tag in the Classic editor) between two elements of the content.
  5. View either the blog page or a category archive.
  6. Click the Continue Reading link (it may show different text).
  7. On the post page, the element immediately after the More tag should appear directly below the admin toolbar.

Skip to Content link on the front end

  1. On any front-end page, press the tab key until you see your theme's "Skip to content" link. If your theme does not have such a link, you could try any theme with the Accessibility Ready tag (or you could try creating one with the WP Accessibility plugin).
  2. Click the Skip to content link.
  3. The main content area should appear directly below the admin toolbar.

"Skip to main content" link in the admin

  1. On any admin page, press the tab key until you see the "Skip to main content" link.
  2. Click the Skip to main content link (or press the Enter key).
  3. The main content area should (continue to) appear directly below the admin toolbar, as it had appeared before using the link. This includes any "Screen options" and/or "Help" buttons at the top.
Last edited 3 years ago by sabernhardt (previous) (diff)

#22 @sabernhardt
4 years ago

#43452 was marked as a duplicate.

#23 @sabernhardt
4 years ago

#51910 was marked as a duplicate.

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


3 years ago

#25 @joedolson
3 years ago

  • Milestone changed from Future Release to 5.9
  • Owner changed from SergeyBiryukov to sabernhardt

#26 @sabernhardt
3 years ago

  • Keywords needs-testing added

#27 @SergeyBiryukov
3 years ago

#53788 was marked as a duplicate.

#28 @sabernhardt
3 years ago

  • Keywords needs-refresh added; needs-testing removed
  • Status changed from reviewing to accepted

@sabernhardt
3 years ago

using toolbar height variable and adjusting scroll padding for bundled themes

#29 @sabernhardt
3 years ago

  • Keywords needs-testing added; needs-refresh removed

The latest patch adjusts scroll padding for bundled themes:

  • Twenty Fourteen has a fixed header of (at least) 48px. This adds scroll padding for most in-page anchors. However, the "more" tag already had padding from r46429, so this adds extra space to that when clicking "Continue reading" links. Reverting r46429 might be appropriate when including this.
  • Twenty Sixteen has a 21px visual border on larger screens.
  • Twenty Seventeen has a fixed header of 72px. The "more" tag seems to have a JS solution, so this also would add extra padding to those anchors and enough for others.

Another idea I had considered was creating an additional variable for themes to set their own fixed header heights, which might not be any better than the way the patch does it.

html {
	--wp-admin--admin-bar--height: 32px;
	scroll-padding-top: calc( var(--wp-admin--admin-bar--height) + var(--wp-theme--header--height, 0px) );
}

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


3 years ago

#31 @audrasjb
3 years ago

  • Keywords needs-refresh added

The patch looks good to me, but we need to remove the padding added in r46429

@sabernhardt
3 years ago

removing extra padding from Twenty Fourteen

#32 @sabernhardt
3 years ago

  • Keywords needs-refresh removed

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


3 years ago

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


3 years ago

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 years ago

#37 follow-up: @costdev
3 years ago

  • Keywords needs-testing removed

Test Report

TL;DR

Patch works as expected on all four testing scenarios provided.

Env

  • WordPress 5.9-alpha-20211116.134713
  • Chrome 95.0.4638.69
  • Windows 10
  • Theme: Twenty Twenty
  • Gutenberg Editor
  • Plugin: None activated

Steps to test

See this comment above.

Before patch

  1. In-page anchor: The heading is partially hidden by the admin toolbar.
  2. Continue Reading link: The content is partially hidden by the admin toolbar.
  3. Skip to content on frontend: The content is partially hidden by the admin toolbar.
  4. Skip to main content in admin: The content is partially hidden by the admin toolbar. (this was supposed to be fine before the patch, according to the testing instructions).

After patch

  1. In-page anchor: The heading appears below the admin toolbar and is fully visible.
  2. Continue Reading link: The content appears below the admin toolbar and is fully visible.
  3. Skip to content on frontend: The content appears below the admin toolbar and is fully visible.
  4. Skip to main content in admin: The content appears below the admin toolbar and is fully visible.

Comments

  • Very nice work on resolving this issue.

#38 in reply to: ↑ 37 @sabernhardt
3 years ago

Replying to costdev:

  1. Skip to main content in admin: The content is partially hidden by the admin toolbar. (this was supposed to be fine before the patch, according to the testing instructions).

Words are hard :) By "continue to" in that step, I meant before and after using the link, not before applying the patch. Hopefully that's clearer now.

Marking for commit consideration.

#39 @sabernhardt
3 years ago

  • Keywords commit added

#40 @joedolson
3 years ago

  • Owner changed from sabernhardt to joedolson

#41 @joedolson
3 years ago

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

In 52198:

Toolbar: Prevent adminbar from hiding in-page anchors.

Use scroll-padding-top to offset scroll position on in-page anchors when adminbar is active. Also adjusts existing scroll padding for core themes that implement it.

Props afercia, kingkero, audrasjb, dufresnesteven, thimalw, sabernhardt, costdev.
Fixes #46371.

#42 @sabernhardt
3 years ago

  • Keywords needs-dev-note added; commit removed

reminder: include this in the dev note for the CSS custom property #52623

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


3 years ago

#44 @sabernhardt
3 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.