Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#30056 closed defect (bug) (fixed)

Twenty Fifteen: Menu hiccups

Reported by: nhuja's profile nhuja Owned by: iandstewart's profile iandstewart
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Bundled Theme Keywords: has-patch commit
Focuses: ui, javascript Cc:


The no-js class is triggering the sub menus to show for a brief period of time until JS removes that class. I couldn't find the another best alternative to this but it feels a bit annoying when you have that brief flashing of sub menus and collapsing. The scroll bar appears for a brief period if the menu/sidebar is long.

Attachments (4)

30056.diff (2.0 KB) - added by iamtakashi 10 years ago.
Independent JS check inside head to avoid flashing child menus.
30056.1.diff (1.1 KB) - added by mattwiebe 10 years ago.
30056.2.diff (1.2 KB) - added by afercia 10 years ago.
Updated patch with new regex, rebuilt from the root. Would like a second opinion though.
30056.3.diff (1.1 KB) - added by mattwiebe 10 years ago.

Download all attachments as: .zip

Change History (19)

#1 @nhuja
10 years ago

  • Focuses ui javascript added

Oh, Can't edit the ticket. This is Twenty Fifteen theme.

#2 @DrewAPicture
10 years ago

  • Component changed from General to Bundled Theme
  • Summary changed from Menu hiccups to Twenty Fifteen: Menu hiccups

#3 @iandstewart
10 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.1

10 years ago

Independent JS check inside head to avoid flashing child menus.

#4 @iamtakashi
10 years ago

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

What about doing the same js check with non-jQuery way? We will need to have one more file but if we link it inside <head> with wp_enqueue_script(), we don't see the flash anymore. Does it worth it?

#5 @mattwiebe
10 years ago

It's generally accepted (ref) to do this inline in the document's <head> so that it takes effect instantly before anything is loaded. 30056.diff actually makes things worse by loading an extra javascript file, slowing down the site load.

Patch incoming for the inline approach.

10 years ago

#6 @iamtakashi
10 years ago

The only reason I enqueued was that I've been told all scripts have to be linked with wp_enqueue_script() before, but it makes more sense to make this an exception.

This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.

10 years ago

#8 @iandstewart
10 years ago

  • Keywords commit added; needs-testing removed

#9 @afercia
10 years ago

Worth noting that in 2011 Paul Irish, by suggestion of Dan Beam, changed that regular expression that used word boundaries with a new one to avoid some (rare) edge cases, namely \b also matches - or +.

For example, it fails if there's already a class like, say, myplugin-no-js as first class.

The change is here:

(/(^|\s)no-js(\s|$)/, '$1$2')

For reference, it was changed again here:
to add the ability to prefix that class, not useful in our case.
Current version source:

10 years ago

Updated patch with new regex, rebuilt from the root. Would like a second opinion though.

#10 @afercia
10 years ago

  • Keywords 2nd-opinion added; commit removed

#11 @mattwiebe
10 years ago

The updated patch is fine, but really unnecessary since it runs before wp_head() (and therefore any 3rd-party scripts) and we have the no-js document class hardcoded. We don't need to handle edge cases that won't happen. :)

#12 @afercia
10 years ago

Nice point @mattwiebe, you're right :) So if your assumption is correct, we don't need to match word boundaries either. Just a simple replace "no-js" with "" ?
Btw I would be more careful, assumptions are the root of all evil :) even if the only thing that comes to my mind now is what child themes could do hardcoding additional CSS classes. Should we take that into account?

#13 @mattwiebe
10 years ago

Yep, could ditch word boundaries too. As for child themes, if they change header.php, they're responsible to make it work.

Adding a simplified patch that merely sets the documentElement class to "js" for the simplest possible form.

10 years ago

#14 @iandstewart
10 years ago

  • Keywords commit added; 2nd-opinion removed

#15 @iandstewart
10 years ago

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

In 30211:

Twenty Fifteen: prevent a flash of visible sub menus before scripts load.

Props mattwiebe, aferica, fixes #30056.

Note: See TracTickets for help on using tickets.