WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#30297 closed defect (bug) (fixed)

Notice for missing HTTP_USER_AGENT in get_calendar

Reported by: elusiveunit Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version: 1.0
Component: Widgets Keywords: has-patch dev-feedback
Focuses: accessibility, template Cc:

Description

Just like in #27374, a notice can appear if $_SERVER['HTTP_USER_AGENT'] isn't set.

All other checks for HTTP_USER_AGENT I found were preceded by $is_X or wp_is_mobile(), which should stop any notices from happening.

Attachments (6)

get-calendar-http-user-agent.patch (716 bytes) - added by elusiveunit 6 years ago.
30297.patch (748 bytes) - added by 5um17 6 years ago.
Patch after removing unnecessary variable
30297.2.patch (694 bytes) - added by elusiveunit 6 years ago.
Refresh
30297.2-no-var.patch (723 bytes) - added by elusiveunit 6 years ago.
Refresh
30297.3-no-var.patch (723 bytes) - added by elusiveunit 5 years ago.
Refresh
30297.3-no-conditional.patch (994 bytes) - added by elusiveunit 5 years ago.
Removing the conditional entirely

Download all attachments as: .zip

Change History (13)

@5um17
6 years ago

Patch after removing unnecessary variable

#1 @5um17
6 years ago

  • Focuses administration added
  • Keywords has-patch added

I think it does not require any extra variable only placing isset as a first condition will do the job. Added modified version of patch for this.

#2 @elusiveunit
6 years ago

  • Focuses template added; administration removed

Of course, I just think shorter line lengths are more readable. Either way is fine.

@elusiveunit
6 years ago

Refresh

@elusiveunit
6 years ago

Refresh

@elusiveunit
5 years ago

Refresh

@elusiveunit
5 years ago

Removing the conditional entirely

#3 @elusiveunit
5 years ago

Refreshed again, this time with just the simpler version.

Also added patch for just removing the conditional, since it seems to be from IE6 days and is probably pointless now.

#4 @johnbillion
5 years ago

  • Focuses accessibility added
  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 4.4

The title attributes here are used to output the list of post titles for each day in the calendar. They get very unwieldy once there's more than a handful of posts on a given day. Line breaks in attributes are nasty, too.

Accessibility team: what do you think about removing the title attributes here entirely? Could they be improved to provide more useful information, or is that a fruitless endeavour?

#5 @elusiveunit
5 years ago

Yeah, I usually stay away from line breaks in attributes, but thought the title became clearer in this case. One could also use a line feed entity 
 to keep the source output prettier, if that's a concern.

The spec mentions them and also advice caution: https://html.spec.whatwg.org/multipage/dom.html#the-title-attribute

A quick test shows that a title with enough lines will spill outside the screen in Firefox, while Chrome starts to position the tooltip towards the top if needed, so there's that as well.

In the end, removing it would probably be fine, since it's pretty much an inaccessible, mouse-only attribute (?).

#6 @joedolson
5 years ago

From an accessibility perspective, these titles don't really provide a lot of information, since they're available to such a small subset of users.

What would probably be the most useful thing to do here for accessibility would be to remove the title attribute entirely, and add an aria-label attribute with text like "Posts published on {date}". I'm not sure that having the complete list of titles serves any purpose; but knowing the general context of the link would definitely be valuable.

#7 @wonderboymusic
5 years ago

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

In 34463:

Calendar Widget: remove the title attribute for days and replace with aria-label. Spruce up some of this nasty code. Delete unnecessary bits.

Props wonderboymusic, elusiveunit, 5um17.
Fixes #30297.

Note: See TracTickets for help on using tickets.