394771 - Lightning calendar view context menus use wrong entities

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Rimas Kudelis [use needinfo to ping]]

Calendar context menus in Lightning currently use calendar.prev*.button.tooltip and calendar.next*.button.tooltip, but they use accesskeys from a completely different file, that are defined for different entities: goPreviousCmd.accesskey and goNextCmd.accesskey.

I think these context menus should either use their own strings, or at least use goPreviousCmd.*.label and goNextCmd.*.label, which are also used for application menus in Sunbird.

Comment 1

[Martin Schröder [:mschroeder]]

see http://mxr.mozilla.org/seamonkey/source/calendar/sunbird/base/content/calendar-menubar.inc#346 for Sunbird behavior

Comment 2

[Simon Paquet [:sipaq]]

Rimas, you filed this as a trunk-only bug. Is this really not happening on the 1.8 branch as well?

Comment 3

[Rimas Kudelis [use needinfo to ping]]

Simon, I filed it as a trunk bug because 0.7 is trunk AFAIK, and there is no such choice as "Lightning 0.7" anyway. Actually, I've only tested it with a 1.8 build.

Comment 4

[Martin Schröder [:mschroeder]]

(In reply to comment #2)
> Rimas, you filed this as a trunk-only bug. Is this really not happening on the
> 1.8 branch as well?

Simon, the Version field is not useful for Calendar at the moment. We have options Trunk, unspecified and some released versions. What is the correct Version setting for nightlys from branch?

Comment 5

[Simon Paquet [:sipaq]]

If a bug is reproducible on the Branch, then use "unspecified". If it is reproducible on both branch and trunk, then use "unspecified" as well.

If it is only reproducible on the trunk then use "Trunk".

Comment 6

[Michael Wolf]

There seems to be a further superfluous entity. The entity 

<!ENTITY calendar.context.deleteitem.label "Delete Item">

in calendar.dtd has an own accesskey entity: 

<!ENTITY calendar.context.deleteitem.accesskey "l">

But this entity isn't used. It is used

<!ENTITY lightning.context.deleteitem.accesskey  "D">

from lightning.dtd instead. What is <!ENTITY calendar.context.deleteitem.accesskey "l"> for? Is it superfluous?

Comment 7

[Michael Wolf]

To #Comment 6:

Created separate bug 396159 for that because it concerns another entity.

Comment 8

[Stefan Sitter [:ssitter]]

Does the issue still exists in recent 0.9pre nightly builds after the context menu consolidation in Bug 430430?

Comment 9

[Martin Schröder [:mschroeder]]

Rimas, can you check if this issue still exists in Lightning 1.0pre, please?

Comment 10

[Rimas Kudelis [use needinfo to ping]]

I think it's obvious from just looking at the sources:
http://hg.mozilla.org/comm-central/file/e328e7f11872/calendar/base/content/calendar-common-sets.xul#l238

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

This should take care

Comment 12

[Stefan Sitter [:ssitter]]

Comment on attachment 432620 [details] [diff] [review]
Fix - v1


>+++ b/calendar/base/content/calendar-common-sets.xul
>+                label-day="&calendar.prevday.label;"
>+                label-week="&calendar.prevweek.label;"
>+                label-multiweek="&calendar.prevweek.label;"
>+                label-month="&calendar.prevmonth.label;"
>+                accesskey-day="&calendar.prevday.accesskey;"
>+                accesskey-week="&calendar.prevday.accesskey;"
>+                accesskey-multiweek="&calendar.prevday.accesskey;"
>+                accesskey-month="&calendar.prevday.accesskey;"/>

Use the correct entity for prevday/prevweek/prevmonth access key

>+++ b/calendar/locales/en-US/chrome/calendar/calendar.dtd
>-<!ENTITY calendar.prevmonth.button.tooltip      "Previous Month" >
>+<!ENTITY calendar.nextday.label                            "Next Day" >
>+<!ENTITY calendar.prevday.label                            "Previous Day" >
>+<!ENTITY calendar.nextday.accesskey                        "x" >
>+<!ENTITY calendar.prevday.accesskey                        "s" >
>+<!ENTITY calendar.nextweek.label                           "Next Week" >
>+<!ENTITY calendar.prevweek.label                           "Previous Week" >
>+<!ENTITY calendar.nextweek.accesskey                       "x" >
>+<!ENTITY calendar.prevweek.accesskey                       "s" >
>+<!ENTITY calendar.nextmonth.label                          "Next Month" >
>+<!ENTITY calendar.prevmonth.label                          "Previous Month" >
>+<!ENTITY calendar.nextmonth.accesskey                      "x" >
>+<!ENTITY calendar.prevmonth.accesskey                      "s" >
> 
> <!ENTITY calendar.navigation.nextday.tooltip    "One Day Forward" >

Please get rid of the superfluous whitespace you added.

>+++ b/calendar/sunbird/base/content/calendar-menubar.inc
>+                     accesskey=""
>+                     label-day="&calendar.prevday.label;"
>+                     label-week="&calendar.prevweek.label;"
>+                     label-multiweek="&calendar.prevweek.label;"
>+                     label-month="&calendar.prevmonth.label;"
>+                     accesskey-day="&calendar.prevday.accesskey;"
>+                     accesskey-week="&calendar.prevday.accesskey;"
>+                     accesskey-multiweek="&calendar.prevday.accesskey;"
>+                     accesskey-month="&calendar.prevday.accesskey;"
>                      observes="calendar_view_prev_command"/>

Use the correct entity for prevday/prevweek/prevmonth access key.

>            <menuitem id="calendar-go-menu-next"
>                      label=""
>-                     label-day="&goNextCmd.day.label;"
>-                     label-week="&goNextCmd.week.label;"
>-                     label-multiweek="&goNextCmd.week.label;"
>-                     label-month="&goNextCmd.month.label;"
>-                     accesskey="&goNextCmd.all.accesskey;"
>+                     accesskey=""
>+                     label-day="&calendar.nextday.label;"
>+                     label-week="&calendar.nextweek.label;"
>+                     label-multiweek="&calendar.nextweek.label;"
>+                     label-month="&calendar.nextmonth.label;"
>+                     accesskey-day="&calendar.nextday.accesskey;"
>+                     accesskey-week="&calendar.nextweek.accesskey;"
>+                     accesskey-multiweek="&calendar.nextweek.accesskey;"
>+                     accesskey-month="&calendar.nextmonth.accesskey;"/>
>                      observes="calendar_view_next_command"/>

Close the menuitem element only once.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

Man I wonder why the previous patch even worked! Sorry about the bad state of the patch, I hope this one does better.

Comment 14

[Stefan Sitter [:ssitter]]

Comment on attachment 432911 [details] [diff] [review]
Fix - v2

r=ssitter

Nit: Should we file a followup bug to unify the wordings? For example the Go menu and the view context menu use "Next Month" but the view navigation button and the minimonth use "One Month Forward". Same for "Go to Today" vs. "Go to today".

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Yes, I think we should. If you have a moment, feel free to file it, otherwise I'll try to do so this week.

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/98e96c27cb83>

-> FIXED