317311 - Add calendar widget to places page

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Brett Wilson]

The places pane should support a calendar widget for viewing history by date.

Comment 1

[Brett Wilson]

Attachment: Implement calendar widget in XBL

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 203814 [details] [diff] [review]
Implement calendar widget in XBL

go go gadget calendar!

r=[e-mail]

Comment 3

[Mike Connor [:mconnor]]

I'd really love to avoid implementing/supporting yet another calendar widget.  There's a longstanding bug on adding date/time pickers to toolkit, can we do that here?

Comment 4

[Brett Wilson]

There's no calendar widget at all in toolkit? Are you recommending that this get moved there? I'm not up for supporting all the features that everybody is going to ask for if this goes in toolkit, but I guess this might be a useful starting place.

Comment 5

[Mike Connor [:mconnor]]

The toolkit lacks a datepicker widget, and yes I'd like to see the Places datepicker use a toolkit version, since that means we'll get more consumers and testing (and create more value in the toolkit).  I'm sure you wouldn't have to carry that load yourself to make it full-featured if this impl isn't enough for other consumers.  There's also existing implementations floating around (ccing some calendar-saavy people to see where this impl works in relation to other calendar-like widgets).

I think as soon as we're looking at implementing something reusable, we need to make sure we're not putting it in /browser if there's other consumers of it.  This is a general rule that we've been good at following to date, and I don't want to see us move away from that.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

The calendar datepicker is pretty standalone.
There are two problems with moving it to toolkit:
- The code style likely doesn't match toolkit standards
- It makes it harder to change the datepicker while doing calendar work. We would need review from a toolkit peer etc.

Comment 7

[Benjamin Smedberg]

1) It's not a good idea to significantly slow down Ffox development of specialized widgets just because we do want a generic solution, if nobody has stepped up to provide that generic solution or if that generic solution doesn't fit the problem at hand
2) We need to be careful that we're not reinventing the wheel and especially that accessibility experts on in on the loop when we're adding new widgets (cc'ing aaronlev)
3) We need to make sure that specialized widgets don't make it harder to add a better generic widget in the future

So I'd like aaronlev to comment, and also this should be named with a firefox-specific prefix, so instead of class="calendar" use class="ff-places-calendar" or something similar.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Brett, can you put up some screenshots of this picker so we can see what it looks like in action?

Comment 9

[Brett Wilson]

Attachment: Calendar widget image

This is what the calendar widget looks like. It allows you to click and drag over a range of days (the darker square is where the cursor is, the mouse cursor itself disappears in screenshots). It will be used to specify which part of history you are interested in viewing. It may also be incorporated into some kind of query builder.

I think it is very important that this widget be as simple as possible. This is a pretty limited application and I'm trying to avoid adding all kinds of buttons and stuff. For example, it's impossible to make selections spanning more than one "screen." Solving this well is very difficult, and I don't imagine it mattering for the normal use case.

Note there is currently no keyboard accessability. This will need to be added.

Comment 10

[Mike Connor [:mconnor]]

style can be tweaked fairly easily, we're working on the review piece of the puzzle too.  There's a decent number of potential toolkit reviewers, and there's room for more active reviewers (working on it!).

My concern is that we can take the opportunity now to refactor and get the datepicker code in toolkit, or we can do it a harder way later, if ever, and continue to duplicate effort.

Brett, I don't think we'd want a more complex datepicker widget (visually) than what you already have.  There might be a more extensive implementation that extends the binding via inheiritance or properties, but this is exactly the type of thing we need in the toolkit.  You may want to talk to Aaron Leventhal about what's needed to make it properly accessible.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 203814 [details] [diff] [review]
Implement calendar widget in XBL

>Index: content/places.xml
>===================================================================

>+  <binding id="calendar">
>+    <content>
>+      <xul:vbox class="calendar-box">
>+        <xul:hbox class="calendar-header">
>+          <xul:label anonid="prevmonth" class="calendar-month-jump">&#xab;</xul:label>
>+          <xul:label anonid="monthtitle" class="calendar-month-title" flex="1"/>
>+          <xul:label anonid="nextmonth" class="calendar-month-jump">&#xbb;</xul:label>
>+        </xul:hbox>
...
>+        <xul:grid anonid="calendargrid" class="calendar-grid">
>+         <xul:columns><xul:column flex="1"/><xul:column flex="1"/><xul:column flex="1"/><xul:column flex="1"/><xul:column flex="1"/><xul:column flex="1"/><xul:column flex="1"/></xul:columns>
>+          <xul:rows>
>+            <xul:row class="calendar-day-header">
>+              <xul:label anonid="calendarhead0"></xul:label>
>+              <xul:label anonid="calendarhead1"></xul:label>
...
>+            </xul:row>

Is this keyboard accessible? (also wondering about screen readers).

>+            <xul:row>
>+              <xul:label anonid="calendar0" class="calendar-day" tooltip="calendartooltip">00</xul:label>
>+              <xul:label anonid="calendar1" class="calendar-day" tooltip="calendartooltip">00</xul:label>
>+              <xul:label anonid="calendar2" class="calendar-day" tooltip="calendartooltip">00</xul:label>

How about adding a sub-widget for those? It is esp. needed for screen readers.

>+    <implementation>
>+      <constructor><![CDATA[
>+        var grid = document.getAnonymousElementByAttribute(this, "anonid",
>+                                                           "calendargrid");
>+        this._numCells = 42; // max number of cells displayable in the calendar
>+        this._cellPrefix = "calendar"; // value before the number in the ID of cells
>+
>+        this._currentMonth = -1;
>+        this._currentYear = -1;
>+        this._cell0Date = null; // date for top left of calendar
>+        this._selectBegin = null;
>+        this._selectEnd = null;
>+
>+        // localized stuff, FIXME: move somewhere else
>+        this._pref_firstDayOfWeek = 0; // 0 = Sunday, 1 = Monday
>+        this._pref_dayHeaders = ["S", "M", "T", "W", "T", "F", "S"];
>+        this._pref_shortMonthNames = ["Jan", "Feb", "Mar", "Apr", "May", "Jun",
>+                            "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"];
>+

Could you add <field>s for those? We should also expose currentMonth, currentYear (and currentDay?) as properties if we do make the widget reusable.

>+        // cell item
>+        var calendargrid = document.getAnonymousElementByAttribute(this, "anonid", "calendargrid");
>+        this._days = new Array(this._numCells);
>+        this._selected = new Array(this._numCells);
>+        for (var i = 0; i < this._numCells; i ++) {
>+          this._days[i] = document.getAnonymousElementByAttribute(this, "anonid", this._cellPrefix + i);
>+          this._selected[i] = false;
>+        }
>+
>+        // month navigation hooks
>+        var myself = this;
>+        document.getAnonymousElementByAttribute(this, "anonid", "prevmonth").
>+          addEventListener("click", function() { myself.jumpMonth(-1); }, false);
>+        document.getAnonymousElementByAttribute(this, "anonid", "nextmonth").
>+          addEventListener("click", function() { myself.jumpMonth(1); }, false);
>+
>+        // day selection hooks
>+        calendargrid.addEventListener("mousedown", function(event) { myself.mouseDown(event); }, false);
>+        calendargrid.addEventListener("mouseup", function(event) { myself.mouseUp(event); }, false);
>+        calendargrid.addEventListener("mousemove", function(event) { myself.mouseMove(event); }, false);
>+

You could simply use onclick and onmouse* attributes on these elements.

>+        this.visibleMonth = new Date(); // today
>+      ]]></constructor>
>+
>+      <property name="visibleMonth">
>+        <getter>
>+          return new Date(this._visibleMonth);
>+        </getter>
>+        <setter>
>+          this._visibleMonth = new Date(val.getFullYear(), val.getMonth(), 1);

Add a <field> please.

>+          this.drawMonth();
>+        </setter>
>+      </property>


Setters are supposed to |return val| (see various widgets in toolkit/content/widgets).

>+      <property name="beginrange">
>+        <getter>
>+          if (! this._selectBegin)
>+            return null;
>+          else
>+            return new Date(this._selectBegin);

No |else| after return please.


>RCS file: /cvsroot/mozilla/browser/components/places/skin-win/places.css

Why do the style rules live in content?



>Index: content/places.css
>===================================================================
>Index: skin-win/places.css
>===================================================================
>+.calendar {
>+  -moz-binding:url("chrome://browser/content/places/places.xml#calendar");
>+  display:-moz-box;
>+}

Could add some spcaes in the css files.

Comment 12

[Brett Wilson]

(In reply to comment #11)
> You could simply use onclick and onmouse* attributes on these elements.

Yes, I was trying to avoid event.target.parent.parent.parent stuff in many of the XBL bindings. This way I can just have a reference the correct object which I think it more maintainable.

> >RCS file: /cvsroot/mozilla/browser/components/places/skin-win/places.css
> Why do the style rules live in content?

Because there hasn't been another place created for it yet. skin-win will be moved somewhere else.

Thanks for the comments. We'll have another pass in the future to make this thing accessable via screen readers and keyboard, which is obviously required for release. I'll address your style stuff either then or before then if I have time.

I'd also like to make sure there won't be a better calendar going into toolkit before I spend time improving this one.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

It seems like there's a fair bit of overlap here (ie the widget appearance of both the places and calendar implementation today seems just about identical).  Unless there's a non-trivial amount of features or integration points that aren't shared by both calendar and places, it seems like it would make sense to have a single shared implementation in toolkit.  Given how simple this widget is, I'll be surprised if there's really room for a lot of feature creep or conflict between "generic" and "app-specific" versions.  Landing it in toolkit would also make it available for (eg) addressbook use, which would be handy for things like birthdays.

The one feature that the calendar widget has now that the place widget doesn't is that the month and year bits are actually dropdown menus (though you wouldn't know it unless you mouse over them).  The places widget has the "drag-to-select days" which the calendar widget doesn't.  

So presumably whatever ends up in toolkit should support those features.  mvl mentioned that he might want to make it possible to highlight certain dates if there are already events on those days.  Are there other features or integration points that people are likely to want in the next year or two?

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Added. See 317828 for hooking it up. 

Comment 15

[Brett Wilson]

Marking this fixed because it has landed. We'll make localization/ accesabilityization/merge-with-another-calendar-ization another bug.

Comment 16

[gekacheka]

For reference, it looks like Andrew Douglas <[e-mail]> has been working on integrating the calendar datepicker for XForms/Webforms2.
A Feb 2005 start to move calendar datepickers to toolkit was in bug 92174, and work continued in Bug 199779, bug 260120, bug 293390.   But the final version of the bug 283344 patch implements its own datepicker, not toolkit.

The calendar datepicker provides a menupopup which can parse a text date in the current short locale format, as well as popping up a date picker widget.  Typing or pasting a date is often easier than manipulating the picker (considering accessibility issues).

(In reply to comment #9)
> This is what the calendar widget looks like. It allows you to click and drag
> over a range of days (the darker square is where the cursor is, the mouse
> cursor itself disappears in screenshots). It will be used to specify which part
> of history you are interested in viewing. It may also be incorporated into some
> kind of query builder.
> 
> I think it is very important that this widget be as simple as possible. This is
> a pretty limited application and I'm trying to avoid adding all kinds of
> buttons and stuff. For example, it's impossible to make selections spanning
> more than one "screen." Solving this well is very difficult, and I don't
> imagine it mattering for the normal use case.

(This sounds like it's saying a range of dates is always within one month in the normal use case --- I don't understand how that is a reasonable assumption.

Handling shift-click might take care of that issue.)

Comment 17

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv