Minimonth missing accessible name and navigation

RESOLVED FIXED in 5.7

Status

Calendar
Calendar Views
RESOLVED FIXED
9 years ago
4 months ago

People

(Reporter: Fallen, Assigned: Colomban Wendling)

Tracking

(Blocks: 1 bug, {access})

unspecified
access
Bug Flags:
blocking-calendar1.0 +

Details

(URL)

Attachments

(3 attachments, 14 obsolete attachments)

45.98 KB, patch
Fallen
: review+
Jorg K (GMT+2)
: review+
Details | Diff | Splinter Review
3.00 KB, patch
Jorg K (GMT+2)
: review+
Fallen
: review+
Details | Diff | Splinter Review
833 bytes, patch
Colomban Wendling
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
(Reporter)

Comment 1

9 years ago
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
(Reporter)

Updated

8 years ago
Blocks: 242112
(Reporter)

Comment 2

8 years ago
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!
Assignee: nobody → eduardokatayama

Comment 3

8 years ago
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?
(Reporter)

Comment 4

8 years ago
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.

Updated

7 years ago
Assignee: eduardokatayama → nobody
(Reporter)

Updated

7 years ago
Flags: blocking-calendar1.0+
Whiteboard: [not needed beta][has l10n impact]

Comment 5

7 years ago
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.
(Reporter)

Comment 6

7 years ago

(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.

Comment 7

7 years ago
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?
(Reporter)

Comment 8

7 years ago
(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.

Comment 9

7 years ago
Created attachment 512122 [details] [diff] [review]
Get keyboard navigation (mostly) working

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.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED

Comment 10

7 years ago
Created attachment 512647 [details] [diff] [review]
[after beta][l10n] Add ARIA stuff

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?
Attachment #512122 - Attachment is obsolete: true
Attachment #512647 - Flags: feedback?(philipp)
(Reporter)

Comment 11

7 years ago
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.
Attachment #512647 - Attachment description: Add ARIA stuff → [after beta] Add ARIA stuff
Attachment #512647 - Flags: review?(philipp)
Attachment #512647 - Flags: feedback?(philipp)
Attachment #512647 - Flags: feedback+
(Reporter)

Updated

6 years ago
Attachment #512647 - Attachment description: [after beta] Add ARIA stuff → [after beta][l10n] Add ARIA stuff
(Reporter)

Comment 12

6 years ago
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.
Attachment #512647 - Flags: review?(philipp) → review+

Comment 13

6 years ago
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?
(Reporter)

Comment 14

6 years ago
(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?
(Reporter)

Comment 15

6 years ago
Jim, any updates here?

Comment 16

6 years ago
Created attachment 597329 [details] [diff] [review]
Updated (but bitrotted) patch

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...
Attachment #512647 - Attachment is obsolete: true
Created attachment 598321 [details] [diff] [review]
Patch

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.
Attachment #597329 - Attachment is obsolete: true
Attachment #598321 - Flags: review?(philipp)
(Reporter)

Comment 18

6 years ago
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
Attachment #598321 - Flags: review?(philipp) → review-

Comment 19

3 years ago
I'm not actively working on this.
Assignee: squibblyflabbetydoo → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 20

7 months ago
I'll try and work on this if it's allright
(Reporter)

Comment 21

7 months ago
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.
(Reporter)

Updated

7 months ago
Assignee: nobody → cwendling
Whiteboard: [not needed beta][has l10n impact]
(Assignee)

Comment 22

7 months ago

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.
(Assignee)

Comment 23

6 months ago
Created attachment 8835588 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth

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.
Status: NEW → ASSIGNED
(Assignee)

Comment 24

6 months ago
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.
(Reporter)

Comment 25

6 months ago
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.
Flags: needinfo?(philipp)
(Assignee)

Comment 26

6 months ago
(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.
(Assignee)

Comment 27

6 months ago
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.
(Assignee)

Comment 28

6 months ago
Created attachment 8837661 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth

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.
Attachment #8835588 - Attachment is obsolete: true
(Assignee)

Comment 29

6 months ago
Created attachment 8838091 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth

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.
Attachment #8837661 - Attachment is obsolete: true
(Assignee)

Comment 30

6 months ago
Created attachment 8838597 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth

Tiny update of the patch to properly focus the minimonth inside the Today pane's popup when it gets shown.
Attachment #8838091 - Attachment is obsolete: true
(Assignee)

Comment 31

6 months ago
Created attachment 8844052 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth

Tiny update explicitly setting table headers scope (as requested by @Fallen)
Attachment #8838597 - Attachment is obsolete: true
(Reporter)

Comment 32

6 months ago
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.
(Reporter)

Comment 33

6 months ago
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.
Attachment #8844052 - Flags: review+
(Assignee)

Comment 34

6 months ago
Created attachment 8844830 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth

(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?
Attachment #8844052 - Attachment is obsolete: true
(Reporter)

Comment 35

6 months ago
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?
Flags: needinfo?(philipp) → needinfo?(cwendling)
(Assignee)

Comment 36

6 months ago
(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.
Flags: needinfo?(cwendling) → needinfo?(philipp)

Comment 37

5 months ago
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.
(Assignee)

Comment 38

5 months ago
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.
(Reporter)

Comment 39

5 months ago
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.
Flags: needinfo?(philipp)
(Assignee)

Comment 40

5 months ago
Created attachment 8846727 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth

Updated patch using Intl.DateTimeFormat instead of adding new strings
Attachment #8844830 - Attachment is obsolete: true
(Reporter)

Comment 41

5 months ago
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)
Attachment #8846727 - Flags: review+
(Reporter)

Comment 42

5 months ago
Created attachment 8848190 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth

Patch for checkin that includes reviewer in commit message
Attachment #598321 - Attachment is obsolete: true
Attachment #8846727 - Attachment is obsolete: true
Attachment #8848190 - Flags: review+
(Reporter)

Updated

5 months ago
Keywords: checkin-needed

Comment 43

5 months ago
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' });

Comment 44

5 months ago
Clearing checkin-needed for now in case you want to address my suggestions above.
Keywords: checkin-needed
(Assignee)

Comment 45

5 months ago
Created attachment 8848478 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth - v8

(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.
Attachment #8848478 - Flags: review?(philipp)
Attachment #8848478 - Flags: review?(jorgk)

Comment 46

5 months ago
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'.
(Reporter)

Comment 47

5 months ago
I think the variable is just fine, as it keep line length under control.
(Reporter)

Comment 48

5 months ago
Comment on attachment 8848478 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth - v8

From my side this is good to go
Attachment #8848478 - Flags: review?(philipp) → review+

Comment 49

5 months ago
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.
Flags: needinfo?(Mozilla)
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...
Flags: needinfo?(Mozilla)
Created attachment 8848822 [details] [diff] [review]
Adapt Mozmill tests for changes in minimonth V1

This fixes the tests.

Try Run looks promising after most of the platforms:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=daf2d565178521d567e31d941d78044b705838ba
Attachment #8848822 - Flags: review?(makemyday)
Attachment #8848822 - Flags: review?(makemyday) → review?(jorgk)

Comment 52

5 months ago
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.
Attachment #8848822 - Flags: review?(jorgk) → review+

Comment 53

5 months ago
Comment on attachment 8848478 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth - v8

Thanks, I might remove the variable when landing this.
Attachment #8848478 - Flags: review?(jorgk) → review+
(Reporter)

Comment 54

5 months ago
(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.

Comment 55

5 months ago
Created attachment 8848860 [details] [diff] [review]
Add keyboard navigation and accessibility to minimonth - v8b

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.
Attachment #8848190 - Attachment is obsolete: true
Attachment #8848478 - Attachment is obsolete: true
Attachment #8848860 - Flags: review?(philipp)

Updated

5 months ago
Attachment #8848822 - Flags: review?(philipp)
(Reporter)

Comment 56

5 months ago
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.
Attachment #8848860 - Flags: review?(philipp) → review-
(Reporter)

Updated

5 months ago
Attachment #8848822 - Flags: review?(philipp) → review+
(Reporter)

Comment 57

5 months ago
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 58

5 months ago
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.
Attachment #8848860 - Flags: review- → review?(philipp)
(Reporter)

Comment 59

5 months ago
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.
Attachment #8848860 - Flags: review?(philipp) → review-

Updated

5 months ago
Attachment #8848478 - Attachment is obsolete: false

Comment 60

5 months ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED

Updated

5 months ago
Attachment #8848860 - Attachment is obsolete: true
(Reporter)

Comment 61

5 months ago
Thanks for the reminder!
Target Milestone: --- → 5.7
(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...
(Assignee)

Comment 63

5 months ago
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
Attachment #8848822 - Flags: review?(Mozilla)
(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

Comment 65

5 months ago
(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?
(Reporter)

Comment 66

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

Comment 67

5 months ago
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.
Attachment #8848822 - Flags: review?(Mozilla)
(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.

Comment 69

5 months ago
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.

Comment 70

5 months ago
Created attachment 8849646 [details] [diff] [review]
Follow-up for Mozmill tests

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?

Comment 72

5 months ago
No, I didn't. I just typed in what Colomban had said.
Then I have a look the next days....

Comment 74

5 months ago
(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?
Flags: needinfo?(cwendling)
(Assignee)

Comment 76

4 months ago
(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.
Flags: needinfo?(cwendling)
OK, is it worth filing a new bug for this little fix, or shall we reopen here?

Comment 78

4 months ago
We can land it here since it's the same version 5.7. Well, Philipp might think differently.
Created attachment 8862966 [details] [diff] [review]
Follow-up for Mozmill tests V2

So as per the above comments, the maybe critical use of aria-label is eliminated.
Attachment #8849646 - Attachment is obsolete: true
Attachment #8862966 - Flags: review?(cwendling)
(Assignee)

Comment 80

4 months ago
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.
Attachment #8862966 - Flags: review?(cwendling) → review+
pushed  to comm-central:
-> https://hg.mozilla.org/comm-central/rev/6e7ea319404a0d311705dd3215a70535c48a087a
You need to log in before you can comment on or make changes to this bug.