Skip to content

Calendar: fix month jumping WIP #1519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 25, 2015
Merged

Calendar: fix month jumping WIP #1519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec0d679
Calendar: Remove selected property from $.date
fnagel Jan 24, 2015
4c2229f
Calendar: Fix month jumping WIP
fnagel Jan 24, 2015
d9ee3a1
Calendar tests: Fix dateFormat option test
fnagel Mar 24, 2015
a3df6dd
Calendar: Add comment in _needsRefresh method
fnagel Apr 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions external/date.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ $.date = function( date, globalFormat ) {

this.dateObject = this.dateObject || new Date();
this.globalFormat = globalFormat;
this.selected = null;
};

$.date.prototype = {
Expand Down Expand Up @@ -152,8 +151,9 @@ $.date.prototype = {
var day = week.days[ week.days.length ] = {
lead: printDate.getMonth() != date.getMonth(),
date: printDate.getDate(),
month: printDate.getMonth(),
year: printDate.getFullYear(),
timestamp: printDate.getTime(),
current: this.selected && this.selected.equal( printDate ),
today: today.equal( printDate )
};
day.render = day.selectable = !day.lead;
Expand All @@ -180,13 +180,6 @@ $.date.prototype = {
result[ result.length - 1 ].last = true;
return result;
},
select: function() {
this.selected = this.clone();
return this;
},
selectedDate: function() {
return this.selected.date();
},
clone: function() {
var date = this.dateObject;
return new $.date( new Date( date.getFullYear(), date.getMonth(),
Expand Down
9 changes: 4 additions & 5 deletions tests/unit/calendar/calendar_options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,11 @@ test( "buttons - advanced", function() {

test( "dateFormat", function() {
expect( 2 );
var element = $( "#calendar" ).calendar({
value: "1/1/14"
}),
firstDayLink = element.calendar( "widget" ).find( "td[id]:first button" );
var element = $( "#calendar" ).calendar();

firstDayLink.trigger( "mousedown" );
element.calendar( "value", "1/1/14" );

element.calendar( "widget" ).find( "td[id]:first button" ).trigger( "mousedown" );
equal( element.calendar( "value" ), "1/1/14", "default formatting" );

element.calendar( "option", "dateFormat", { date: "full" } );
Expand Down
91 changes: 54 additions & 37 deletions ui/calendar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,26 +61,27 @@ return $.widget( "ui.calendar", {
this.labels = Globalize.translate( "datepicker" );
this.buttonClickContext = this.element[ 0 ];

this.date = $.date( this.options.value, this.options.dateFormat ).select();
this.date.eachDay = this.options.eachDay;
this.date = $.date( this.options.value, this.options.dateFormat );
this.viewDate = this.date.clone();
this.viewDate.eachDay = this.options.eachDay;

this._on( this.element, {
"click .ui-calendar-prev": function( event ) {
event.preventDefault();
this.date.adjust( "M", -this.options.numberOfMonths );
this.refresh();
this._refresh();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So changing the month still changes both this.date and this.viewDate. That seems wrong, it should only change the viewDate, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we want to change the current view as well as the working date. Don't confuse this.date with the currently selected date (which is set here: https://github.com/fnagel/jquery-ui/blob/datepicker-fix-jumping/ui/calendar.js#L374-L376).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this.date is just a temporary variable? And go away, if we go with "Shouldn't we pass date as a parameter to build* fns instead of setting this.date?"? Anyway, should't block this PR, can deal with that later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it would not go away if this comment has been implemented. It's a helper variable like this.viewDate.

},
"click .ui-calendar-next": function( event ) {
event.preventDefault();
this.date.adjust( "M", this.options.numberOfMonths );
this.refresh();
this._refresh();
},
"mousedown .ui-calendar-calendar button": function( event ) {
event.preventDefault();

// TODO Exclude clicks on lead days or handle them correctly
// TODO Store/read more then just date, also required for multi month picker
this._select( event, $( event.currentTarget ).data( "timestamp" ) );
this._setOption( "value", new Date( $( event.currentTarget ).data( "timestamp" ) ) );
this.refresh();
this._trigger( "select", event );
this.grid.focus();
},
"mouseenter .ui-calendar-header button": "_hover",
Expand All @@ -98,10 +99,6 @@ return $.widget( "ui.calendar", {
},

_handleKeydown: function( event ) {
var oldMonth = this.date.month(),
oldYear = this.date.year();

// TODO: Handle for pickers with multiple months
switch ( event.keyCode ) {
case $.ui.keyCode.ENTER:
this.activeDescendant.mousedown();
Expand Down Expand Up @@ -135,16 +132,29 @@ return $.widget( "ui.calendar", {
return;
}

if ( this.date.month() !== oldMonth || this.date.year() !== oldYear ) {
this.refresh();
if ( this._needsRefresh() ) {
this._refresh();
this.grid.focus();
}

this._setActiveDescendant();
},

_needsRefresh: function() {
if ( this.date.month() !== this.viewDate.month() || this.date.year() !== this.viewDate.year() ) {

// Check if the needed day is already present in our grid due
// to eachDay option changes (eg. other-months demo)
return !this.grid.find(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this checking?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does a basic checking if anything has changed at all (as before, see fnagel@4c2229f#diff-563a5392a566d1a29a68b1b5a37c4cb6L139) and if so, checks if the wanted date is already available within our grid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but why is this extra check needed? That is, under what circumstance do this.date andthis.viewDate` match, but the day isn't rendered?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what is meant here. The find() part is needed to make sure we do not have the day available due to eachDay changes (like in other-months demo).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Can you add that explanation as a comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

this._sanitizeSelector( "#" + this._getDayId( this.date ) )
).length;
}

return false;
},

_setActiveDescendant: function() {
var id = this.id + "-" + this.date.day();
var id = this._getDayId( this.date );

this.grid
.attr( "aria-activedescendant", id )
Expand Down Expand Up @@ -183,14 +193,14 @@ return $.widget( "ui.calendar", {
_buildMultiplePicker: function() {
var headerClass,
html = "",
currentDate = this.date,
months = this.date.months( this.options.numberOfMonths - 1 ),
currentDate = this.viewDate,
months = this.viewDate.months( this.options.numberOfMonths - 1 ),
i = 0;

for ( ; i < months.length; i++ ) {

// TODO: Shouldn't we pass date as a parameter to build* fns instead of setting this.date?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still relevant? Should at least update it, since you now set viewDate

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this comment was intended as a "fix me" or a "improve me" type of a comment. Could be removed if we are fine with the current implementation from a code quality perspective.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try the suggested approach, in a separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've added this to my todo right after this PR has been merged.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should I presume with PR #1341 so we can finally merge this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's land this, then update #1341.

this.date = months[ i ];
this.viewDate = months[ i ];
headerClass = "ui-calendar-header ui-widget-header ui-helper-clearfix";
if ( months[ i ].first ) {
headerClass += " ui-corner-left";
Expand All @@ -211,7 +221,7 @@ return $.widget( "ui.calendar", {

html += "<div class='ui-calendar-row-break'></div>";

this.date = currentDate;
this.viewDate = currentDate;

return html;
},
Expand Down Expand Up @@ -259,17 +269,17 @@ return $.widget( "ui.calendar", {

_buildTitle: function() {
return "<span class='ui-calendar-month'>" +
this.date.monthName() +
this.viewDate.monthName() +
"</span> " +
"<span class='ui-calendar-year'>" +
this.date.year() +
this.viewDate.year() +
"</span>";
},

_buildGrid: function() {
return "<table class='ui-calendar-calendar' role='grid' aria-readonly='true' " +
"aria-labelledby='" + this.id + "-month-label' tabindex='0' " +
"aria-activedescendant='" + this.id + "-" + this.date.day() + "'>" +
"aria-activedescendant='" + this._getDayId( this.date ) + "'>" +
this._buildGridHeading() +
this._buildGridBody() +
"</table>";
Expand All @@ -278,7 +288,7 @@ return $.widget( "ui.calendar", {
_buildGridHeading: function() {
var cells = "",
i = 0,
weekDayLength = this.date.weekdays().length;
weekDayLength = this.viewDate.weekdays().length;

if ( this.options.showWeek ) {
cells += "<th class='ui-calendar-week-col'>" + this._getTranslation( "weekHeader" ) + "</th>";
Expand All @@ -303,7 +313,7 @@ return $.widget( "ui.calendar", {
_buildGridBody: function() {

// this.date.days() needs caching as it has O(n^2) complexity.
var days = this.date.days(),
var days = this.viewDate.days(),
i = 0,
rows = "";

Expand Down Expand Up @@ -332,12 +342,12 @@ return $.widget( "ui.calendar", {
var content = "",
attributes = [
"role='gridcell'",
"aria-selected='" + ( day.current ? true : false ) + "'"
"aria-selected='" + ( this._isCurrent( day ) ? true : false ) + "'"
],
selectable = ( day.selectable && this._isValid( new Date( day.timestamp ) ) );

if ( day.render ) {
attributes.push( "id='" + this.id + "-" + day.date + "'" );
attributes.push( "id='" + this.id + "-" + day.year + "-" + day.month + "-" + day.date + "'" );

if ( !selectable ) {
attributes.push( "aria-disabled='true'" );
Expand All @@ -350,20 +360,23 @@ return $.widget( "ui.calendar", {
return "<td " + attributes.join( " " ) + ">" + content + "</td>";
},

_getDayId: function( date ) {
return this.id + "-" + date.year() + "-" + date.month() + "-" + date.day();
},

_buildDayElement: function( day, selectable ) {
var attributes, content,
classes = [ "ui-state-default" ];

if ( day === this.date && selectable ) {
classes.push( "ui-state-focus" );
}
if ( day.current ) {
if ( this._isCurrent( day ) ) {
classes.push( "ui-state-active" );
}
if ( day.today ) {
classes.push( "ui-state-highlight" );
}
// TODO Explain and document this
if ( day.extraClasses ) {
classes.push( day.extraClasses.split( " " ) );
}
Expand All @@ -383,6 +396,10 @@ return $.widget( "ui.calendar", {
return content;
},

_isCurrent: function( day ) {
return this.options.value && day.timestamp === this.options.value.getTime();
},

_createButtonPane: function() {
this.buttonPane = $( "<div>" )
.addClass( "ui-calendar-buttonpane ui-widget-content ui-helper-clearfix" );
Expand Down Expand Up @@ -434,6 +451,11 @@ return $.widget( "ui.calendar", {
this.buttonPane.appendTo( this.element );
},

_refresh: function() {
this.viewDate.setTime( this.date.date().getTime() );
this.refresh();
},

// Refreshing the entire calendar during interaction confuses screen readers, specifically
// because the grid heading is marked up as a live region and will often not update if it's
// destroyed and recreated instead of just having its text change. Additionally, interacting
Expand Down Expand Up @@ -464,9 +486,9 @@ return $.widget( "ui.calendar", {
for ( ; i < this.options.numberOfMonths; i++ ) {
this.element.find( ".ui-calendar-title" ).eq( i ).html( this._buildTitle() );
this.element.find( ".ui-calendar-calendar" ).eq( i ).html( this._buildGrid() );
this.date.adjust( "M", 1 );
this.viewDate.adjust( "M", 1 );
}
this.date.adjust( "M", -this.options.numberOfMonths );
this.viewDate.adjust( "M", -this.options.numberOfMonths );

// TODO: This assumes focus is on the first grid. For multi pickers, the widget needs
// to maintain the currently focused grid and base queries like this off of it.
Expand All @@ -488,11 +510,6 @@ return $.widget( "ui.calendar", {
});
},

_select: function( event, time ) {
this.valueAsDate( new Date( time ) );
this._trigger( "select", event );
},

value: function( value ) {
if ( arguments.length ) {
this.valueAsDate( Globalize.parseDate( value, this.options.dateFormat ) );
Expand Down Expand Up @@ -551,14 +568,14 @@ return $.widget( "ui.calendar", {
});

if ( refresh ) {
this.refresh();
this._refresh();
}
},

_setOption: function( key, value ) {
if ( key === "value" ) {
if ( this._isValid( value ) ) {
this.date.setTime( value.getTime() ).select();
this.date.setTime( value.getTime() );
this._super( key, value );
}
return;
Expand All @@ -584,7 +601,7 @@ return $.widget( "ui.calendar", {
}

if ( key === "eachDay" ) {
this.date.eachDay = value;
this.viewDate.eachDay = value;
}

if ( key === "dateFormat" ) {
Expand Down