The pane that contains the minimonth has a "nothing" accessible node. It is therefore not clear for everyone what the buttons in the minimonth actually do. We need to give the minimonth pane the correct accessible name and a role, see the linked URL. While we are at it, we should also make the minimonth days keyboard navigable. It probably makes sense to use the arrow keys to navigate between the days and give each day an accessible name like "31st, busy" or "20th, free", depending on free-busy status.

Additional info in case someone else wants to fix this: The minimonth should probably have a few tabstops: Year dropdown - Month dropdown - one month back - today - one month forward - (the month grid) To do this, use the -moz-user-focus css attribute where appropriate. When tabbed to the month grid, it should be possible to use the arrow keys to navigate between the days. As an extra treat, we may want to use Pg Up/Pg Down to quickly change the month while in the month grid. Alternatives are "Ctrl+Left/Right" for month navigation and maybe "Ctrl+Alt+Left/Right" for navigating by a full year. These handlers should be on the minimonth binding at [1]. Each day of the current month should have an accessible name that gives info about the day, as described in comment #0. When moving to a day from the previous month, the month (and even the year if that has changed) should also be spoken, like "December 29th, 2008 is free" when viewing January 2009. The month grid itself needs to get the right accessible roles. This is quite easily done by setting the right attributes on the xul elements. See the aria specs (see URL field) or mozilla documentation on how to do that. * grid - The container for the month grid. Its name should include the current month and year. * row - The container for each week * columnheader - Weekday names * gridcell - An individual day in the month grid. [1] http://hg.mozilla.org/comm-central/annotate/b32a33ebb0a1/calendar/base/content/widgets/minimonth.xml#l352

A group of three students from Brazil in a course about open source is going to be working on this bug as a student-project. Looking forward to your patch(es), thanks Eduardo!

Hi Philipp, I read your description, but I didn't understand exactly. The bug is to make the minimonth(the widget) more key accessible, making the arrows and short-cuts things, Am I right?

Hello Glaucus, This bug is about two things actually. One is making it keyboard navigable, i.e adding tabs allowing to navigate via arrow keys. The other is making it more accessible for screenreaders. I'd suggest fixing the keyboard navigation issue first. Afterwards the accessibility tree might look different. If you open the DOM inspector and then "File > Inspect a Chrome Document > Mozilla Thunderbird - Calendar" (the last option is the title of the window you want to inspect) you can switch it to accessible mode by clicking on the dropdown left of "Document - DOM Nodes", and changing this to "Accessible Tree". This way you get a slightly different structure, which is what screenreaders see. Then you navigate to the minimonth ( pane (xul:tabbox) > propertypage (vbox) > nothing (modevbox) ). All entries with "nothing" are bad since the screenreaders can't see them. What you need to do is give each "nothing" element a sensible role. If you need some help on this, you might want to ask people on irc in the channel #accessibility. See the URL field of this bug for some info on the different roles.

How about this: no tabstops except for the main calendar bit, and the following shortcuts: * arrow keys for days * page up/down for prev/next month * shift + page up/down for prev/next year * home for "today" * enter to select the date (for the menulist dropdown only) That minimizes the amount of tabbing around you have to do while still letting you do everything you can do with the mouse.

(In reply to comment #5) > How about this: no tabstops except for the main calendar bit, and the following > shortcuts: > That minimizes the amount of tabbing around you have to do while still letting > you do everything you can do with the mouse. While this definitely makes sense for a power user, I fear it might be to complicated for the casual user. I'd suggest to still use multiple tab stops (as mentioned in comment 2) but in addition allow the keyboard shortcuts you propose. Also, only focusing the month grid might also cause a11y issues, especially since keyboard shortcuts other than arrows are rather trial and error. Also, if you can't focus the year/month dropdown via keyboard, then users with a screen reader won't have access to it.

In comment 1, you mentioned that the calendar itself is a grid, but at least for the minimonth, it's a vbox containing hboxes. Should I change this to a grid while I'm there?

(In reply to comment #7) > In comment 1, you mentioned that the calendar itself is a grid, but at least > for the minimonth, it's a vbox containing hboxes. Should I change this to a > grid while I'm there? Grid was rather in a visual sense, not codewise. I guess it can be changed to a grid, but unless it brings some performance improvement or other bonus, I don't think this is necessary.

A <xul:grid> might be a tiny bit faster, and it'd be more "semantic", but it's probably not worth the trouble. Attached is a patch that does the keyboard navigation part of this bug (so no accessible names yet). One problem is that pressing "up" on the first element of the year dropdown just cycles to the last element instead of scrolling. I'm not entirely sure what to do there, since menupopups seem to really enjoy eating up all the events I care about.

Ok, I added accessibility info, though I'm not sure I did it right. I had to add some strings for the ARIA date labels since I couldn't find anything suitable that was already there. Maybe I just missed something though... :Fallen, what do you think about this?

Comment on attachment 512647 [details] [diff] [review] [after beta][l10n] Add ARIA stuff As discussed, the patch in general looks fine, but due to the string changes I'll have to review/commit this after the release.

Comment on attachment 512647 [details] [diff] [review] [after beta][l10n] Add ARIA stuff Go ahead and push this now. I assume the merge just happened, so please also transplant to aurora.

I tested this out again and noticed a somewhat-important issue: if you change the month by clicking, the date doesn't update (this is how it behaved pre-patch too). However, this means that you can't select a new date with the keyboard, since the selected date isn't in the current month. What should we do here?

(In reply to Jim Porter (:squib) from comment #13) > I tested this out again and noticed a somewhat-important issue: if you > change the month by clicking, the date doesn't update (this is how it > behaved pre-patch too). However, this means that you can't select a new date > with the keyboard, since the selected date isn't in the current month. What > should we do here? But if you change the month and then select a day via clicking, the view does move to the selected day. Isn't this feasible for keyboard navigation too? Or do you mean a different issue?

Jim, any updates here?

Here's a patch for this that worked at one point, but bitrot has broken it. If someone wants to fix the bitrot, they can go ahead and commit it, but unfortunately, my backlog is pretty big at the moment...

Unbitrotted patch. I changed on every touched file the licence block to MPL 2.0. Additionally I removed a css rule because this pushed the month names to the right. Before this patch they are centered. I let the other changes in the patch untouched.

Comment on attachment 598321 [details] [diff] [review] Patch Thanks for unbitrotting the patch. A few things I've noticed in the accessible tree: * #minimonth-pane has a load of pushbuttons, one for each month, one for the current year, and two menupopups. Shouldn't the pushbuttons be in the menupopups somehow? * The minimonth grid cells wouldn't make clear to a screen reader that a day is in a different month. * The minimonth days are xul:text, I'm not sure its obvious to a screenreader that they are clickable. This could work without changes though, not sure. * When tabbing through the interface, the minimonth grid doesn't have a focus rectangle when selected. * Maybe it would make sense for the arrow keys to not change the day yet (might cause screenreaders to start speaking other things?) but to make the user confirm the selection with <Enter>. A focus rectangle or similar could be displayed. Please check with accessibility experts and/or UX experts on this. * As an added bonus, the #minimonth-pane itself is a "nothing" element. If you could change this and the #calendar-list-pane to be a propertypage, that would be swell. It may just be a matter of making the modevbox and modehbox a propertypage. r- to look into those issues. If you need help testing this patch, I'd suggest installing a screenreader and/or pinging MarcoZ. Ubuntu has the screen

I'm not actively working on this.

I'll try and work on this if it's allright

Hi cwendling, sure please do! The patch is fairly old so it might need another round of unbitrotting and testing. Please also go through the issues I mentioned. You can use the DOM inspector add-on to check the accessible tree. Let me know if you have questions or if you need help setting things up.

I already unbitrotted the patch, which wasn't much fun but went out fine. I'm seeing several issues with it (incl. not being able to change the year/month with the keyboard), but I'm not really sure if they are inherent to the patch or come from more subtle bitrot, not having actually tested the original patch in its expected conditions. Anyway it's a good start. > * #minimonth-pane has a load of pushbuttons, one for each month, one for the > current year, and two menupopups. Shouldn't the pushbuttons be in the > menupopups somehow? I don't see this with Accerciser and my restored version of the patch. > * The minimonth grid cells wouldn't make clear to a screen reader that a day > is in a different month. As I currently see it, it tries adding the month name to the day. However the aria-label population is delayed which means it's no properly set for all elements. I've fixed that. > * The minimonth days are xul:text, I'm not sure its obvious to a > screenreader that they are clickable. This could work without changes > though, not sure. Me neither. I'm not sure what the implications are if the ATs don't see it as activatable, but it might be a problem indeed. I'll check. > * Maybe it would make sense for the arrow keys to not change the day yet > (might cause screenreaders to start speaking other things?) but to make the > user confirm the selection with <Enter>. A focus rectangle or similar could > be displayed. Please check with accessibility experts and/or UX experts on > this. I'd agree, changing the selected day straight when navigating seems suboptimal.

Unbitrotted patch, with a fair share of improvements: * Give proper a11y names to the day names (full name instead of 2 letter abbreviation) * Fix initial a11y names of all days (populate them all, instead of only doing it lazily) * Don't change the actually selected date when navigating with the keyboard, and wait for explicit selection with Return * Add shortcut Escape to focus back the selected day * Don't navigate through the whole 42 table cells with Tab, so it behaves like a single element Tab-wise. I'm not sure about the implementation of this part, but it works fine. * Fix selecting the month and year with the keyboard from the buttons/popups * Fix focus after selecting a month though the popup * Various small other focus fixes * Move day-specific event handlers on the day element themselves (implementation change only) * Try to hint ATs about the currently selected/displayed day (doesn't do too much with Orca though). What's left to do IMO is a proper way to announce the selected date to ATs, so they announce it when it changes. I'm not really sure how to do that, changing the aria-label on the parent panel/table doesn't make Orca announce it, even if setting aria-live=polite. I'll investigate this, but I'm also interested with opinions on what *should* happen, both technically and from the user POV.

According to an Orca developer, to have the table properly presented it requires the element to implement the Table interface, not merely have a "grid" role. Having a proper table would allow screen readers to present the columns and rows, allowing better context. However, which element should be used? `listbox` doesn't seem to allow cell selection, and `tree` seems interesting but doesn't seem to have row headers, and for the moment I fail to force all columns to be equally sized. So I'm really not sure what should be used here. Does someone have insight into this? Something that is semantically exposed and behaving as a table with rows and columns to the ATs would be needed.

Thanks for working on the patch, I'm very excited! I will take a look at this soon. Aren't there other roles that would work to make it have the table interface, or does grid maybe do that internally? I'm reluctant to change the UI element into something different to accomodate for this. I'm sure there are other examples of grid-like things in Firefox somewhere that would also be making use of this. You should try asking in #accessibility on IRC, I'm sure someone will be able to help out. If you want to catch me on IRC, my nickname is Fallen.

(In reply to Philipp Kewisch [:Fallen] from comment #25) > Aren't there other roles that would work to make it have the > table interface, or does grid maybe do that internally? I'm afraid maybe not. xul:grid seems to rather be a layout-only element rather than a table with headings and all, so I'm not too surprised it doesn't implement nsIAccessibleTable. It's a bit confusing however as https://wiki.mozilla.org/Accessibility/TreeGrid might be suggesting simply setting proper roles should work, I'm not positive. Alternatively, maybe the minimonth could implement nsIAccessibleTable itself? I'm not sure if it's doable or how complex that would be? > I'm reluctant to > change the UI element into something different to accomodate for this. I'm > sure there are other examples of grid-like things in Firefox somewhere that > would also be making use of this. I didn't find any unfortunately. FTR, currently it uses a bunch of xul:hbox (rows) inside an xul:vbox. But using xul:grid didn't seem to help at all. > You should try asking in #accessibility on IRC, I'm sure someone will be > able to help out. I'll try again, even if the other day when I tried that it was quite silent :) > If you want to catch me on IRC, my nickname is Fallen. On which channels are you? I didn't find you on #accessibility and the other ones I though of.

Okay, Jamie on #accessibility suggested to try whether the Table interface would appear if using the proper roles on HTML divs, and it does. So simply switch from xul:vbox/xul:hbox to html:div makes the Table interface appear. That's great, but again using HTML elements seems to affect the code a lot, so I'm not sure what to do. I'll try and investigate if there's another way to hint in XUL, or if there's a bug there somewhere, but all pointers are more than welcome.

New version using an HTML table, which properly presents the Table properties to ATs, allowing them to understand columns and rows, and their headers. It requires some changes, but in the end less than I expected, although it was a rough road to adapt some things. For the moment ATs see both column (day name) and row (week number) headers. I added full-length day names as the aria-label for the column headers, but nothing yet for the rows. What should we do here? It's probably not a good idea to just keep the number, which is likely more confusing than helpful. I guess it needs either to be ignored somehow (no row header form ATs perspective), or labeled more clearly (maybe "week N", or "Nth week"). --- Irrespective of these new changes, I see that the datepicker-forever selection is broken at least since revamping the patch. Not sure if it was forgotten before, or if it's something new since the last iteration, but I have to fix this.

New version of the patch, fixing the datepicker-forever. This simplifies the whole diff, reverting some refactoring from the original patches, that don't seem required, and that unfortunately remove the possibility for mValue to be null, which datepicker-forever requires. But as a bonus the diff is simpler, so likely easier to review. This also adds a more explicit accessible name on the row headers ("Week N" instead of just "N"). This version should be OK for proper review, AFAIK it's good and works great.

Tiny update of the patch to properly focus the minimonth inside the Today pane's popup when it gets shown.

Tiny update explicitly setting table headers scope (as requested by @Fallen)

Thanks for the update! I'll complete the review on this today so we can get this pushed. Thanks so much for your hard work on this bug.

Comment on attachment 8844052 [details] [diff] [review] Add keyboard navigation and accessibility to minimonth Review of attachment 8844052 [details] [diff] [review]: ----------------------------------------------------------------- Great work on this bug. Here a few minor nits to look into, I'd appreciate a new patch ready for checkin. r=philipp ::: calendar/base/content/widgets/minimonth.xml @@ +736,5 @@ > + <parameter name="aRow"/> > + <parameter name="aCol"/> > + <body><![CDATA[ > + if (! this.mCalBox) > + this.mCalBox = document.getAnonymousElementByAttribute(this, "anonid", "minimonth-calendar"); Can you make sure to use brackets even for one-line ifs, and also remove the space after the operator? (here and in other places in the patch) @@ +738,5 @@ > + <body><![CDATA[ > + if (! this.mCalBox) > + this.mCalBox = document.getAnonymousElementByAttribute(this, "anonid", "minimonth-calendar"); > + // there is a Text node between each node, so skip them > + return this.mCalBox.childNodes[1 + aRow*2].childNodes[1 + aCol*2]; You can use https://developer.mozilla.org/en-US/docs/Web/API/ParentNode/children here to not be dependent on whitespace nodes. ::: calendar/locales/en-US/chrome/calendar/calendar.properties @@ +572,5 @@ > +# LOCALIZATION NOTE (dayInMonth): > +# used for display of Month-dates like 'December 12' > +# %1$S will be replaced with the day-index > +# %2$S will be replaced with name of the month > +dayInMonth=%2$S %1$S Is there a possiblity to use methods from calIDateTimeFormatter here instead of creating new strings used exclusively in the views? @@ +585,5 @@ > +# used for display of Month-dates like 'December 12, 2008' > +# %1$S will be replaced with the day-index > +# %2$S will be replaced with name of the month > +# %3$S will be replaced with the year > +dayInYear=%2$S %1$S, %3$S Same for this one, it sounds like we might already have a formatter for this.

(In reply to Philipp Kewisch [:Fallen] from comment #33) > Can you make sure to use brackets even for one-line ifs, and also remove the > space after the operator? (here and in other places in the patch) done > > + // there is a Text node between each node, so skip them > > + return this.mCalBox.childNodes[1 + aRow*2].childNodes[1 + aCol*2]; > > You can use > https://developer.mozilla.org/en-US/docs/Web/API/ParentNode/children here to > not be dependent on whitespace nodes. Oh, indeed. Done. > ::: calendar/locales/en-US/chrome/calendar/calendar.properties > @@ +572,5 @@ > > +# LOCALIZATION NOTE (dayInMonth): > > +# used for display of Month-dates like 'December 12' > > +# %1$S will be replaced with the day-index > > +# %2$S will be replaced with name of the month > > +dayInMonth=%2$S %1$S > > Is there a possiblity to use methods from calIDateTimeFormatter here instead > of creating new strings used exclusively in the views? > > @@ +585,5 @@ > > +# used for display of Month-dates like 'December 12, 2008' > > +# %1$S will be replaced with the day-index > > +# %2$S will be replaced with name of the month > > +# %3$S will be replaced with the year > > +dayInYear=%2$S %1$S, %3$S > > Same for this one, it sounds like we might already have a formatter for this. Admittedly I didn't search much initially as it comes from the original patch from Richard Marti, but no I can't find something really suitable. For dayInMonth there is calIDateTimeFormatter::formatDateWithoutYear(), but it gives a short format, which seems suboptimal here but could be used. But I couldn't find anything for the dayInYear. However, maybe I'm missing something not being familiar enough with this?

Ah yes, I see what you mean, there are indeed no relevant functions in the date time formatter. I'm surprised we don't have one for just "December 12, 2008". I think we should add your code to that service and provide relevant methods. Maybe like this: AString formatDateWithoutYearLong(in calIDateTime aDate); -> "December 12" (maybe even using the ordinals like formatDayWithOrdinal, maybe with an [optional] parameter aWithOrdinal. Up to you :) For dayInYear we could add a parameter [optional] in boolean aOmitWeekday to formatDateLong. What do you think?

(In reply to Philipp Kewisch [:Fallen] from comment #35) > AString formatDateWithoutYearLong(in calIDateTime aDate); -> "December 12" > (maybe even using the ordinals like formatDayWithOrdinal, maybe with an > [optional] parameter aWithOrdinal. Up to you :) Yeah, or add `[optional] in bool aLongMonthName` to `formatDateWithoutYear()`, but I guess that would be inconsistent with the rest of the methods. Whether to use ordinals or not, I don't have a strong opinion there, whatever is natural seems good, so following `formatDateWithoutYear()` is probably best if adding a similar method. > For dayInYear we could add a parameter [optional] in boolean aOmitWeekday to > formatDateLong. By default `formatDateLong()` also uses short month name, so the same problem arises than with `formatDateWithoutYear()`, with probably the same possible solutions.

Just a heads-up: please note that the effective output generated by calIDateTimeFormatter will be unified and changed within the next days by bug 1229684. Long Format will be formatted with long weekday and month names, a 2-digit day and 4-digit year on all platforms then. The ability to addd an ordinal parameter will be locale specific then as we will rely on ICU formatting in future and will likely remove the calendar specific formatting strings at all later on. If you add new methods there, these must use the ICU formatting.

Okay, so maybe I should just use Intl.DateTimeFormat directly then? Something like that: > - if (aDate.getMonth() == date.getMonth()) { > - day.setAttribute("aria-label", date.getDate()); > - } else if (aDate.getFullYear() == date.getFullYear()) { > - let monthName = cal.formatMonth(date.getMonth() + 1, > - "calendar", "dayInMonth"); > - let label = calGetString("calendar", "dayInMonth", > - [date.getDate(), monthName]); > - day.setAttribute("aria-label", label); > - } else { > - let monthName = cal.formatMonth(date.getMonth() + 1, > - "calendar", "dayInYear"); > - let label = calGetString("calendar", "dayInYear", > - [date.getDate(), monthName, date.getFullYear()]); > - day.setAttribute("aria-label", label); > - } > + let labelDateOptions = { day: 'numeric' }; > + if (aDate.getMonth() != date.getMonth()) { > + labelDateOptions.month = 'long'; > + } > + if (aDate.getFullYear() != date.getFullYear()) { > + labelDateOptions.year = 'numeric'; > + } > + let labelTimeFormatter = new Intl.DateTimeFormat(undefined, labelDateOptions); > + day.setAttribute("aria-label", labelTimeFormatter.format(date)); All we need here is something natural, so any correct localized string would do.

Yes, this sounds reasonable to me. Given Intl.DateTimeFormat is standardized we might eventually want to get rid of calIDateTimeFormatter. Lets do it that way.

Updated patch using Intl.DateTimeFormat instead of adding new strings

Comment on attachment 8846727 [details] [diff] [review] Add keyboard navigation and accessibility to minimonth Review of attachment 8846727 [details] [diff] [review]: ----------------------------------------------------------------- Looks great now, thanks for the patch! I will set checkin-needed so this gets pushed. If you are interested in looking into some more accessibility issues you can find them at https://bugzilla.mozilla.org/buglist.cgi?quicksearch=keyword%3Aaccess%20prod%3Acalendar&list_id=13492396 (some side notes regarding bugzilla: when you upload a new patch, please request review from me or a suggested reviewer to make sure it is in a queue. It also helps to give each new patch a number, I usually add " - v1". This helps with the interdiff you can find on the [diff] link)

Patch for checkin that includes reviewer in commit message

Comment on attachment 8848190 [details] [diff] [review] Add keyboard navigation and accessibility to minimonth Review of attachment 8848190 [details] [diff] [review]: ----------------------------------------------------------------- Sorry about the drive-by comments, I was looking at this before checking it in. ::: calendar/base/content/widgets/minimonth.xml @@ +761,4 @@ > dayList[i] = tempDate.toLocaleFormat("%a"); > useOSFormat = true; > } > + longDayList[i] = tempDate.toLocaleFormat("%A"); toLocaleFormat() is deprecated. You should use toLocaleDateString() and above, too. @@ +1084,5 @@ > + var label; > + if (this.mValue) { > + let labelDateOptions = { day: 'numeric', month: 'long', year: 'numeric' }; > + let labelTimeFormatter = new Intl.DateTimeFormat(undefined, labelDateOptions); > + label = labelTimeFormatter.format(this.mValue); You can do this a little more elegantly using toLocaleDateString(): label = this.mValue.toLocaleDateString(undefined, { day: 'numeric', month: 'long', year: 'numeric' });

Clearing checkin-needed for now in case you want to address my suggestions above.

(In reply to Philipp Kewisch [:Fallen] from comment #41) > (some side notes regarding bugzilla: when you upload a new patch, please > request review from me or a suggested reviewer to make sure it is in a > queue. Okay, hope it's good now. > It also helps to give each new patch a number, I usually add " - v1". > This helps with the interdiff you can find on the [diff] link) Fair enough, I locally have versioned patches myself for those I submitted here :) BTW, if you're interested I have a mostly sensible set of 30 (Git) commits locally I can provide if wanted. It still starts with the original fairly large patch I based my changes off, but my subsequent modifications are split fairly decently. (In reply to Jorg K (GMT+1) from comment #43) > ::: calendar/base/content/widgets/minimonth.xml > @@ +761,4 @@ > > dayList[i] = tempDate.toLocaleFormat("%a"); > > useOSFormat = true; > > } > > + longDayList[i] = tempDate.toLocaleFormat("%A"); > > toLocaleFormat() is deprecated. You should use toLocaleDateString() and > above, too. Done. However, for the short name (which existed before) it might be better to start using `weekday:'narrow'` and drop the messy further shortening in the `if (useOSFormat)` right below. I don't really know how to test that, and I think it's way outside the scope of this patch, but it would likely be better now the feature seem readily available properly. > @@ +1084,5 @@ > > + var label; > > + if (this.mValue) { > > + let labelDateOptions = { day: 'numeric', month: 'long', year: 'numeric' }; > > + let labelTimeFormatter = new Intl.DateTimeFormat(undefined, labelDateOptions); > > + label = labelTimeFormatter.format(this.mValue); > > You can do this a little more elegantly using toLocaleDateString(): > label = this.mValue.toLocaleDateString(undefined, > { day: 'numeric', month: 'long', year: 'numeric' }); Oh, nice. Done in the 2 places that created new Intl.DateTimeFormat instances.

Comment on attachment 8848478 [details] [diff] [review] Add keyboard navigation and accessibility to minimonth - v8 Review of attachment 8848478 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adjusting this, one last nit. ::: calendar/base/content/widgets/minimonth.xml @@ +1082,5 @@ > <body><![CDATA[ > + var label; > + if (this.mValue) { > + let labelDateOptions = { day: 'numeric', month: 'long', year: 'numeric' }; > + label = this.mValue.toLocaleDateString(undefined, labelDateOptions); Sorry to be a terrible nit-picker, but you could lose the variable 'labelDateOptions'.

I think the variable is just fine, as it keep line length under control.

Comment on attachment 8848478 [details] [diff] [review] Add keyboard navigation and accessibility to minimonth - v8 From my side this is good to go

Just to be sure before checking this in I did a try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e3bd2bafbfa95996a923a0fd1770038349c99d56 Sadly, there are a bunch of Mozmill failures now: TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cal-recurrence/testAnnualRecurrence.js | testAnnualRecurrence.js::testAnnualRecurrence TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cal-recurrence/testBiweeklyRecurrence.js | testBiweeklyRecurrence.js::testBiweeklyRecurrence TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cal-recurrence/testDailyRecurrence.js | testDailyRecurrence.js::testDailyRecurrence TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cal-recurrence/testLastDayOfMonthRecurrence.js | testLastDayOfMonthRecurrence.js::testLastDayOfMonthRecurrence TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cal-recurrence/testWeeklyNRecurrence.js | testWeeklyNRecurrence.js::testWeeklyNRecurrence TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cal-recurrence/testWeeklyUntilRecurrence.js | testWeeklyUntilRecurrence.js::testWeeklyUntilRecurrence TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cal-recurrence/testWeeklyWithExceptionRecurrence.js | testWeeklyWithExceptionRecurrence.js::testWeeklyWithExceptionRecurrence TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest which are not seen on trunk, in fact, I picked OS X 10.10 opt since it's known to be green currently. I understand that these tests exercise the Minimonth panel, so if the internal structure has changed, the tests need to be adjusted. Markus, could you kindly take a look. Maybe you can provide a separate patch to address the test failures.

From a quick look at the patch, the datepicker changes the popup from a <menulist> to a <panel>, which is a good candidate for this failure. I take a look...

This fixes the tests. Try Run looks promising after most of the platforms: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=daf2d565178521d567e31d941d78044b705838ba

Comment on attachment 8848822 [details] [diff] [review] Adapt Mozmill tests for changes in minimonth V1 Interesting, thanks for looking into it quickly. I'll get this landed. Just for the education of the reviewer ;-) - Could you explain what those lookups do? And another thing: I've I were to eliminate the variable I mentioned in comment #46, what line-length restriction do I have to obey in Calendar? In minimonth.xml there are already lines with are 131 characters long. Eliminating the variable, would give me a 117 character long line. + label = this.mValue.toLocaleDateString(undefined, { day: 'numeric', month: 'long', year: 'numeric' }); What are the rules for breaking lines in Calendar, like this: + label = this.mValue.toLocaleDateString(undefined, + { day: 'numeric', month: 'long', year: 'numeric' }); or this: + label = this.mValue.toLocaleDateString(undefined, + { day: 'numeric', month: 'long', year: 'numeric' }); or this: + label = this.mValue.toLocaleDateString(undefined, + { day: 'numeric', month: 'long', year: 'numeric' }); or only this: + label = this.mValue.toLocaleDateString(undefined, + { day: 'numeric', month: 'long', year: 'numeric' }); which is still 107 characters long.

Comment on attachment 8848478 [details] [diff] [review] Add keyboard navigation and accessibility to minimonth - v8 Thanks, I might remove the variable when landing this.

(In reply to Jorg K (GMT+1) from comment #52) > And another thing: I've I were to eliminate the variable I mentioned in > comment #46, what line-length restriction do I have to obey in Calendar? > > In minimonth.xml there are already lines with are 131 characters long. > Eliminating the variable, would give me a 117 character long line. There is no strict rule (hence the longer lines), but the target is under 100 characters. Exceptions can be made if splitting the line would make it hard to read (as often with long for (;;) loops, or when using long function names like getAnonymousElementByAttribute. There are also various lines that have grown historically. Regardless, the line length and variable changes are issues to be taken care of by calendar reviewers, please don't make extra changes to the patches without review.

If you want another review look for a trivial change, here you go. It would have been helpful to indicate how you wanted this indented, now I'm following: https://hg.mozilla.org/try-comm-central/rev/5a220d0092d70d66540d2681b0a4804d29efb990#l3.538 If it's not right, I can send another patch with some white-space changes :-( Please let me know if you're happy with the r+ for the test fixes or whether you want to review them yourself. As a responsible reviewer you should have assured that there are no test failures in the first place before marking the patch "checkin-needed". So I'm a little disappointed that instead of valuing my initiative here ("say five nice things first"), I get told not even to remove a superfluous variable without review. Given the dire situation as far as calendar reviews are concerned, I just wanted to speed things up.

Comment on attachment 8848860 [details] [diff] [review] Add keyboard navigation and accessibility to minimonth - v8b As previously mentioned I do not agree with the variable change. I am fine with pushing v8.

Jörg, from a sheriffing perspective, is this patch good to go? If I understand correctly we have a green try run with the latest patches and no new bustages should be caused by it.

Comment on attachment 8848860 [details] [diff] [review] Add keyboard navigation and accessibility to minimonth - v8b (In reply to Philipp Kewisch [:Fallen] from comment #56) > As previously mentioned I do not agree with the variable change. I am fine > with pushing v8. I'm surprised about this. I can understand that we don't want to "torture" new contributors with fixing nits. If however, someone suggests to make an improvement, however small it may be, and provides a patch for it, that should be accepted. So please reconsider your decision. It's always good practice to convince rather then impose, and "keep line length under control" (comment #47) doesn't appear to be a good reason for an otherwise unused variable. I certainly accept your module ownership, but as a Thunderbird and Mailnews peer I think I have enough judgement to suggest such a change which is in no way calendar-specific. The amicable solution would have been to advise me of your indentation requirement following my comment #52.

Comment on attachment 8848860 [details] [diff] [review] Add keyboard navigation and accessibility to minimonth - v8b Jörg, I appreciate your commitment, but is it really worth the back and forth for this one line change? I don't agree that keeping the line length under control is a bad reason and would prefer to use a variable, which is coincidentally also what the patch author did. The preferred option for indent is the last one, but given the function name and leading text is so long I think it just looks so much more readable with an extra variable. Please accept my review and let it go.

https://hg.mozilla.org/comm-central/rev/5c6b4b0d0451f43ba0ea0921f833c488fb3f4928 https://hg.mozilla.org/comm-central/rev/1df067640ceebf58b01ab1705b0496c43f732d57 I can't set the target here since 5.7 is not available.

Thanks for the reminder!

(In reply to Jorg K (GMT+1) from comment #52) > Comment on attachment 8848822 [details] [diff] [review] > Adapt Mozmill tests for changes in minimonth V1 > > Interesting, thanks for looking into it quickly. I'll get this landed. > > Just for the education of the reviewer ;-) - Could you explain what those > lookups do? Sure. ;-) Basically they retrieve the years and month popups of minimonth (since these do not have ids to get them) and then click them to go to the desired date. The most cryptical one: controller.click(lookup(` ${miniMonth}/anon({"anonid":"minimonth-calendar"})/[${(dateRow + 1) * 2 + 1}]/ [${(dateColumn + 1) * 2 + 1}] `)); Before we calculated the 0-based position of the Date to be clicked in the days matrix. Since in the DOM-Tree, we have one table cell and one text node alternating, it is not a 1-to-1 transliteration from the visible matrix. The above calculation does exactly that - although it is not easily conceivable...

Comment on attachment 8848822 [details] [diff] [review] Adapt Mozmill tests for changes in minimonth V1 Review of attachment 8848822 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for not having run tests myself in the first place, and to be late with my comments. But I'm afraid 2 of the test changes are a little too fragile: ::: calendar/test/mozmill/shared-modules/test-calendar-utils.js @@ +278,5 @@ > } > > let lastDayInFirstRow = lookup(` > + ${miniMonth}/anon({"anonid":"minimonth-calendar"})/[3]/[15] > + `).getNode().getAttribute("aria-label"); It probably isn't a good idea to rely on the aria-label value: it can contain the month, or even the year, in addition to the day. The more canonical value here would be the textContent. ::: calendar/test/mozmill/testBasicFunctionality.js @@ +42,4 @@ > controller.assertNode(lookup(` > ${path}/id("ltnSidebar")/id("minimonth-pane")/{"align":"center"}/ > id("calMinimonthBox")/id("calMinimonth")/ > + anon({"anonid":"minimonth-calendar"})/[3]/{"aria-label":"1"} And same here

(In reply to Colomban Wendling from comment #63) > It probably isn't a good idea to rely on the aria-label value: it can > contain the month, or even the year, in addition to the day. The more > canonical value here would be the textContent. I will change that I could do that as a seperate patch here, or put in in a patch for bug 1329957

(In reply to Markus Adrario [:Taraman] from comment #64) > (In reply to Colomban Wendling from comment #63) > > It probably isn't a good idea to rely on the aria-label value: it can > > contain the month, or even the year, in addition to the day. The more > > canonical value here would be the textContent. > I will change that > I could do that as a separate patch here, or put in in a patch for bug > 1329957 So there are more cases than covered in the patch here?

I'd suggest to open a new bug for followup changes, or if you want fold them into bug 1329957.

Comment on attachment 8848822 [details] [diff] [review] Adapt Mozmill tests for changes in minimonth V1 No point to set r? on a landed patch ;-) If it's just a two line change modifying the change from this (landed) patch, I suggest to add it here to keep it all in the same bug. That's why I asked in comment #65: "So there are more cases than covered in the patch here?" That was of course nonsense, it should have been: So there are more cases than the two mentioned in comment #63. If it's more and unrelated to the patch already landed, it should go elsewhere.

(In reply to Jorg K (GMT+1) from comment #65) > So there are more cases than covered in the patch here? No, the two things from comment #63 are the only ones like this. But since I am about to prepare another patch for Bug 1329957, it would be handy to put this in there - since this bug landed already. And it also fits to the topic there.

That's a matter or taste ;-) I frequently land "follow-ups" in a resolved bug to keep the issue together instead of spreading it over other bugs and mixing it with other unrelated issues.

Like so, this is what was suggested, but I don't know whether it's right.

I don't know, if the textContent is accessible this way. Did you try it already?

No, I didn't. I just typed in what Colomban had said.

Then I have a look the next days....

(In reply to Markus Adrario [:Taraman] from comment #71) > I don't know, if the textContent is accessible this way. Yes, you're right, looks like my patch is wrong. In the first instance it should be: - `).getNode().getAttribute("aria-label"); + `).getNode().textContent; but in the second I don't even know since I don't know how your lookup magic works: controller.assertNode(lookup(` ${path}/id("ltnSidebar")/id("minimonth-pane")/{"align":"center"}/ id("calMinimonthBox")/id("calMinimonth")/ - anon({"anonid":"minimonth-calendar"})/[3]/{"aria-label":"1"} + anon({"anonid":"minimonth-calendar"})/[3]/{"textContent":"1"} > Then I have a look the next days.... Thanks, I should leave it to the calendar experts ;-)

> Sorry for not having run tests myself in the first place, and to be late > with my comments. But I'm afraid 2 of the test changes are a little too > fragile: > ::: calendar/test/mozmill/testBasicFunctionality.js > @@ +42,4 @@ > > controller.assertNode(lookup(` > > ${path}/id("ltnSidebar")/id("minimonth-pane")/{"align":"center"}/ > > id("calMinimonthBox")/id("calMinimonth")/ > > + anon({"anonid":"minimonth-calendar"})/[3]/{"aria-label":"1"} > > And same here In this second case, changing the test would make it a lot more complicated, since I would have to iterate over the cells, since I can't put the textContent into a Lookup-string. As far as I can see, the aria-label will have Month (& Year) set if it is from another month then the active one? If this is the case, for this test we could stick with the current version, because we just look for the 1st day of the current month which would always have "1" set as aria-label. Colomban, can you confirm that?

(In reply to Markus Adrario [:Taraman] from comment #75) > As far as I can see, the aria-label will have Month (& Year) set if it is > from another month then the active one? Yes. > If this is the case, for this test we could stick with the current version, > because we just look for the 1st day of the current month which would always > have "1" set as aria-label. Yes, if it's indeed always the first day of the *current* month it's alright to check the aria-label.

OK, is it worth filing a new bug for this little fix, or shall we reopen here?

We can land it here since it's the same version 5.7. Well, Philipp might think differently.

So as per the above comments, the maybe critical use of aria-label is eliminated.

Comment on attachment 8862966 [details] [diff] [review] Follow-up for Mozmill tests V2 Review of attachment 8862966 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, indeed now using the HTML content should be better and safer.

pushed to comm-central: -> https://hg.mozilla.org/comm-central/rev/6e7ea319404a0d311705dd3215a70535c48a087a