Make WordPress Core

Opened 14 years ago

Closed 13 years ago

Last modified 6 years ago

#18758 closed task (blessed) (fixed)

Click empty admin bar to scroll to top

Reported by: tjnowell's profile TJNowell Owned by: ericmann's profile ericmann
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.3
Component: Toolbar Keywords: 3.4-early has-patch
Focuses: Cc:

Description

I've attached a patch that will duplicate behaviour from the Google+ black bar at the top of the page in the admin bar.

Apply patch, scroll to the bottom of a long page, and click a vacant area of the admin bar. The page will scroll to the top when clicked.

Tested on IE7/8/9/Chrome/Firefox/Safari, includes JQuery version and fall-back javascript version

For those still unsure what this patch does:

http://www.howtonew.com/google-trick-click-the-upper-black-bar-to-go-to-the-top

Attachments (6)

patch.diff (6.1 KB) - added by TJNowell 14 years ago.
Patch remade relative to wordpress root folder
19534.diff (1.8 KB) - added by ericmann 13 years ago.
Adding a patch that had been applied to an (apparently) duplicate ticket.
18758.diff (1.9 KB) - added by ericmann 13 years ago.
New patch cleans up some code and makes a few conditional statements easier to follow.
18758.2.diff (1.7 KB) - added by koopersmith 13 years ago.
18758.3.diff (1.8 KB) - added by ericmann 13 years ago.
Patch fully compatible with IE7-9
18758-4.patch (1.9 KB) - added by azaozz 13 years ago.

Download all attachments as: .zip

Change History (36)

#1 @scribu
14 years ago

Interesting idea.

Minor note: Please make patches relative to the root WP directory.

@TJNowell
14 years ago

Patch remade relative to wordpress root folder

#2 @jane
14 years ago

I also think the idea is interesting.

#3 @cais
14 years ago

Even if the idea was not in use with G+ it still has great UX potential.

#4 @TJNowell
14 years ago

It also matches the touch status bar to scroll to top behaviour in iOS

#5 @ocean90
13 years ago

Duplicate #19534

@ericmann
13 years ago

Adding a patch that had been applied to an (apparently) duplicate ticket.

#6 @ericmann
13 years ago

  • Component changed from UI to Admin Bar
  • Owner set to ericmann
  • Status changed from new to assigned
  • Version set to 3.3

My patch (tagged 19534.diff) adds a jQuery scroll-to-top function if jQuery is enabled, or a smooth scroll that works in every browser (tested in Firefox, IE 7/8/9, Chrome at least) if jQuery is not being used.

The patch has been tested by a handful of people already and is verified to work with WP 3.3.

#7 @andrea_r
13 years ago

  • Cc andrea@… added

#8 @DrewAPicture
13 years ago

  • Cc xoodrew@… added

I was sorry to see this didn't make it into 3.3.

+1

#9 @Ipstenu
13 years ago

  • Cc ipstenu@… added

#10 @jane
13 years ago

  • Keywords 3.4-early added

#11 @jane
13 years ago

  • Type changed from enhancement to task (blessed)

#12 @jane
13 years ago

  • Milestone changed from Awaiting Review to 3.4

@ericmann
13 years ago

New patch cleans up some code and makes a few conditional statements easier to follow.

#13 @ericmann
13 years ago

I realized that a couple of my conditional statements were tricky to follow: if (leapY < stopY) leapY = stopY; timer++; actually caused me a couple of issues just reading back my code ... so they're now broken up and fixed a bit.

I've also removed the arbitrary stopY variable. Since we're always scrolling to the top, there's no reason to set that separately.

New patch tested against trunk. Only adding the unminified version in case we need to make further changes.

#14 @ryan
13 years ago

Working well so far for me with latest Chrome and Firefox on Mac OS.

#15 @mbijon
13 years ago

  • Cc mike@… added

+1

And, since I happen to have some VMs up: Eric's latest version works well in IE8 and IE9

#16 @koopersmith
13 years ago

  • Keywords needs-refresh added

I like the concept and it works. That said, the code could be a bit cleaner (both stylistically and logically). I'll post up a revised patch.

#17 @koopersmith
13 years ago

  • Keywords needs-refresh removed

Revised patch posted. Haven't tested IE yet.

@koopersmith
13 years ago

#18 @ericmann
13 years ago

Works fine on IE8 and 9. Doesn't seem to work on IE7 (clicking the admin bar doesn't scroll to the top) but nothing breaks, either (no errors are thrown).

#19 @ericmann
13 years ago

Scratch that. The jQuery version works in IE8 and IE9 (not IE7).

The non-jQuery version works in IE9 but not in IE8 or IE7.

#20 @ericmann
13 years ago

With IE7, the problem seems to be with e.srcElement. In both IE7 and 8, e.target is undefined, so e.srcElement is passed to the function instead. Unfortunately, it's passing the wrong target ID to IE7 ("wp-admin-bar-top-secondary") so it fails the initial conditional check.

In IE8, it seems that startY is evaluating to 0 no matter what the scroll position seems to be.

@ericmann
13 years ago

Patch fully compatible with IE7-9

#21 @ericmann
13 years ago

Did some work on the patch and fixed it to work with IE7-9. If we're not supporting IE7 (I missed the dev chat, so I'm not sure if we're still supporting that browser or not) then we can lose the && t.id != 'wp-admin-bar-top-secondary' part of the id selector conditionals. The patch will function just fine in IE8-9 without that, but it's needed for IE7 because of the way the right-hand side of the toolbar is presented on the page.

Also, the fix for IE8 is to use document.documentElement.scrollTop to get a value for startY. I've added that fix as well.

Fully tested in every browser I have access to (IE7-9, Chrome latest, FF latest, Safari) on Windows 7. Works both with and without jQuery.

#22 follow-up: @azaozz
13 years ago

Replying to ericmann:

Yes, there are other browser differences too. Perhaps best would be to load jQuery on the front end when the toolbar is shown. Can namespace it completely with wpjq = jQuery.noConflict(true); or similar so it doesn't conflict with another jQuery version.

#23 in reply to: ↑ 22 @ericmann
13 years ago

Replying to azaozz:

Yes, there are other browser differences too. Perhaps best would be to load jQuery on the front end when the toolbar is shown. Can namespace it completely with wpjq = jQuery.noConflict(true); or similar so it doesn't conflict with another jQuery version.

I think loading jQuery on the front end, just to scroll the page when you click on the admin bar, is a bit overkill. If it's already loaded by the theme, we use it. If not, we should have a reliable, browser-agnostic fallback. The new patch I've posted should handle that.

#24 @azaozz
13 years ago

Not just to scroll, would make the drop-downs consistent by using hoverIntent too. Were planning to try that in 3.3 but ran out of time at the end.

#25 @nacin
13 years ago

I am still on the fence as to whether loading jQuery on the frontend is more trouble than it is worth. Namespacing should help, but there's a lot of opportunity for conflict here.

#26 @mbijon
13 years ago

I'm in support of loading jQuery on the frontend, but the risk from loading a duplicate copy bothers me. On top of conflicts it's a small extra server load + bandwidth, and would be even worse for mobile sites/themes. If we do include any scripts then better de-duplication in wp_enqueue would be my pick (not straightforward though) over namespacing.

+1 for Eric's patch though. The browser-to-browser differences are very minor, and if/when jQuery becomes universal those will disappear.

#27 @nacin
13 years ago

Tested the patch. Looks good. It's a bit slower in non-jQuery mode.

@azaozz
13 years ago

#28 @azaozz
13 years ago

18758-4.patch is about the same as 18758.3.diff but adjusted the speed in non-jQuery mode. Also removed some redundant logic ([document/window/body].scrollTop cannot be < 0).

#29 @azaozz
13 years ago

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

In [19846]:

Click empty toolbar to scroll to top, props ericmann, fixes #18758

#30 @Clorith
6 years ago

#46956 was marked as a duplicate.

Note: See TracTickets for help on using tickets.