WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#16155 closed defect (bug) (fixed)

"Shortlink" button in admin bar relies on shortlink meta tag to work

Reported by: solarissmoke Owned by:
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch dev-feedback
Focuses: Cc:

Description

If you remove the remove the action that adds <link rel="shortlink" ... into the head of a post, the shortlink button in the admin bar says "No shortlink available for this page" when you click it, even though the href for the link is a valid shortlink.

IMO the absence of the <link> does not mean that one doesn't exist (it could be announced in other ways, e.g., a HTTP header), and so the admin bar button needs to take this into account.

Maybe the clickShortlink Javascript function should check the href of the shortlink button in cases where there is no <link> tag?

Attachments (6)

16155.patch (1.1 KB) - added by SergeyBiryukov 5 years ago.
16155.2.patch (1.6 KB) - added by SergeyBiryukov 5 years ago.
16155.diff (6.6 KB) - added by koopersmith 5 years ago.
16155.2.diff (6.7 KB) - added by koopersmith 5 years ago.
16155.fixes.diff (1.6 KB) - added by koopersmith 5 years ago.
16155.remove.script.r17263.patch (682 bytes) - added by ocean90 5 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 @nacin5 years ago

  • Milestone changed from Awaiting Review to 3.1

Since it actually links to the shortlink, we should simply have it echo its own link.

@SergeyBiryukov5 years ago

comment:2 @SergeyBiryukov5 years ago

  • Keywords has-patch added

comment:3 follow-up: @nacin5 years ago

Look at all that red :-)

At that point, we don't even need var shortlink, right? Can just do t.href?

comment:4 follow-up: @nacin5 years ago

Looks like we can also remove the noShortlink bit from script-loader, as the Shortlink menu item will only exist when there's a shortlink.

comment:5 in reply to: ↑ 3 @SergeyBiryukov5 years ago

Replying to nacin:

At that point, we don't even need var shortlink, right? Can just do t.href?

I guess not, since after t = t.parentNode it's not a link anymore.

comment:6 @nacin5 years ago

Ah, never mind on that point then.

@SergeyBiryukov5 years ago

comment:7 in reply to: ↑ 4 @SergeyBiryukov5 years ago

Replying to nacin:

Looks like we can also remove the noShortlink bit from script-loader, as the Shortlink menu item will only exist when there's a shortlink.

Done.

comment:8 @solarissmoke5 years ago

Just wondering, is there a reason why we don't give the shorlink <a> tag an id (e.g., wp-admin-bar-shortlink), and use that instead of class sniffing? I'm guessing there is, because otherwise it would be a lot simpler.

If there was an id, it would also mean we don't need to monitor ALL clicks in the admin bar area, even though the only one we do anything with is the shortlink button.

Version 0, edited 5 years ago by solarissmoke (next)

comment:9 @filosofo5 years ago

Not putting id attributes on the admin menu elements avoids potential conflicts.

Also, note that the class attribute is on the li element, not the a element. The current admin bar view provides a way to put classes (and not ids) on the li elements but not the a elements.

comment:10 @sillybean5 years ago

Related: #14760. (The Shortlink button appears only on posts.)

@koopersmith5 years ago

comment:11 @koopersmith5 years ago

This patch makes the shortlink menu item expand when clicked (like a sub-menu when a menu item is hovered over), and automatically highlights the shortlink.

@koopersmith5 years ago

comment:12 @ryan5 years ago

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

(In [17263]) Expand shortlink menu when clicked. Fix displayed of shortlinks when rel shortlink is missing. Props koopersmith. fixes #16155

@koopersmith5 years ago

comment:13 @koopersmith5 years ago

Patch reselects shortlink when a user clicks on it, increases the shortlink container's width, and fixes a bug where hovering can be undefined.

comment:14 @koopersmith5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:15 @nacin5 years ago

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

(In [17265]) Admin bar shortlink improvements. props koopersmith, fixes #16155.

comment:16 @ocean905 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

16155.remove.script.r17263.patch: In my mind it's now cruft, so we can remove it or is there something against this?

comment:17 @nacin5 years ago

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

(In [17313]) Stop localizing the admin bar. props ocean90, fixes #16155.

Note: See TracTickets for help on using tickets.