Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#17832 closed defect (bug) (fixed)

Don't default to random header image rotation when HEADER_IMAGE = ''

Reported by: ryan Owned by: ryan
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.2
Component: Themes Keywords: has-patch commit
Focuses: Cc:

Description (last modified by ryan)

Themes that define HEADER_IMAGE = '' as well as register multiple default images now default to random image rotation in 3.2. This can be very unexpected and produce poor results. A theme that supports header images but defaults to text only probably has good reason to do so. Showing images where before there were non can make text unreadable in themes that place text over the header image. Perhaps defaulting to random should be opt-in. Maybe an add_theme_support() call.

Attachments (2)

17832.diff (2.1 KB) - added by ryan 4 years ago.
17832-2.diff (3.5 KB) - added by lancewillett 4 years ago.

Download all attachments as: .zip

Change History (16)

#1 @ryan
4 years ago

  • Description modified (diff)

#2 @ryan
4 years ago

The simplest approach would have HEADER_IMAGE = '' continue meaning no default image ( including random ones). A theme that specifies an actual image in HEADER_IMAGE and registers multiple default images would get random turned on by default.

4 years ago

#3 @ryan
4 years ago

Messing with HEADER_IMAGE proved more painful that supporting add_theme_support(). Patch adds add_theme_support( 'custom-header', array( 'random' => true ) ).

#4 @ryan
4 years ago

  • Keywords has-patch added

#5 @ryan
4 years ago

  • Keywords commit added

#6 @azaozz
4 years ago

Commit +1

#7 @ryan
4 years ago

  • Description modified (diff)

#8 @lancewillett
4 years ago

I like the idea of moving towards add_theme_support() instead of the definitions, but... I'd argue that the opt-in already exists: registering multiple header images.

I can't think of a reason (or an existing example in the wild) where a theme registers multiple header images but defaults to text only display. Did you find a case like that? Seems very much like an edge case.

See the original discussion and implementation in #17240. I'd originally intended to use HEADER_IMAGE value of "random" to trigger this, but it seemed like leaving it empty meant the same thing.

The argument Aaron Campbell made on the original ticket is that the user should choose whether to enable random or not, not the theme itself. The theme should always have the randomness available if multiple images exist.

To me that's the one thing I don't like about this feature, there isn't a way for a theme to enable randomness and turn it off by default and let the user choose. It's either on or off from the theme level.

It's also confusing what purpose define( 'HEADER_IMAGE', '' ); would serve now.

4 years ago

#9 @lancewillett
4 years ago

OK, after testing this for a while I see how it works. And it works great!

The random option still exists in the UI regardless of the add_theme_support() call, which is what we want. If the random rotation is turned on by default with add_theme_support() the radio button for random default images is on, which is what we want for the UI.

Attached is a refreshed patch that adds better commenting to both theme.php and the Twenty Eleven functions file.

#10 follow-up: @ryan
4 years ago

We could call it random-default instead of random. Or use default = random with the notion of being able to handle other defaults this way later. Although I'd rather not open that up right now.

This came up when I added multiple headers to p2. It went from a default of no image to a default of random with no way to go back to a default of no image.

#11 in reply to: ↑ 10 @lancewillett
4 years ago

Ryan, I like your implementation -- let's do it. See my updated patch for some small changes to comments to make things more clear.

#12 @nacin
4 years ago

Having the same thoughts as Ryan. 'default' => 'random' might mean that 'random' shares the same namespace as the other images. Would be perfectly comfortable with default-random => true (or random-default, the wording there being default-to-random or random-by-default).

#13 @ryan
4 years ago

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

In [18325]:

Add theme support option for turning on random default headers. fixes #17832

#14 @ryan
4 years ago

Went with random-default and Lance's updated patch.

Note: See TracTickets for help on using tickets.