WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#30056 closed defect (bug) (fixed)

Twenty Fifteen: Menu hiccups

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

Description

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 6 years ago.
Independent JS check inside head to avoid flashing child menus.
30056.1.diff (1.1 KB) - added by mattwiebe 6 years ago.
30056.2.diff (1.2 KB) - added by afercia 6 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 6 years ago.

Download all attachments as: .zip

Change History (19)

#1 @nhuja
6 years ago

  • Focuses ui javascript added

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

#2 @DrewAPicture
6 years ago

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

#3 @iandstewart
6 years ago

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

@iamtakashi
6 years ago

Independent JS check inside head to avoid flashing child menus.

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

@mattwiebe
6 years ago

#6 @iamtakashi
6 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.


6 years ago

#8 @iandstewart
6 years ago

  • Keywords commit added; needs-testing removed

#9 @afercia
6 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:
https://github.com/Modernizr/Modernizr/pull/358/files

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

For reference, it was changed again here:
https://github.com/Modernizr/Modernizr/commit/71894c33b00082a52a9474a07751b3290576abf7
to add the ability to prefix that class, not useful in our case.
Current version source:
https://github.com/Modernizr/Modernizr/blob/master/src/setClasses.js

@afercia
6 years ago

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

#10 @afercia
6 years ago

  • Keywords 2nd-opinion added; commit removed

#11 @mattwiebe
6 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
6 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
6 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.

@mattwiebe
6 years ago

#14 @iandstewart
6 years ago

  • Keywords commit added; 2nd-opinion removed

#15 @iandstewart
6 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.