WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 2 months ago

#46371 reviewing defect (bug)

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

Reported by: afercia Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Toolbar Keywords: has-screenshots good-first-bug has-patch 2nd-opinion
Focuses: ui, accessibility Cc:
PR Number:

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 (3)

anchor link behind toolbar.png (272.1 KB) - added by afercia 9 months ago.
Example of a heading with anchor hidden behind the WP admin bar
46371.diff (2.6 KB) - added by kingkero 6 months ago.
46371.1.diff (703 bytes) - added by audrasjb 4 months ago.
Don’t hide content anchors with WP Admin Bar

Download all attachments as: .zip

Change History (17)

@afercia
9 months ago

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

#1 @desrosj
6 months 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.


6 months ago

@kingkero
6 months ago

#3 @kingkero
6 months 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
6 months ago

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

@audrasjb
4 months ago

Don’t hide content anchors with WP Admin Bar

#5 @audrasjb
4 months ago

  • Keywords needs-refresh added; needs-testing removed

#6 @audrasjb
4 months 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.


3 months ago

#8 @audrasjb
3 months 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.


3 months ago

#10 @audrasjb
3 months ago

  • Keywords commit added

#11 @afercia
3 months 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
3 months 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.


2 months ago

#14 @audrasjb
2 months 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.

Note: See TracTickets for help on using tickets.