Closed Bug 394771 Opened 12 years ago Closed 10 years ago

Lightning calendar view context menus use wrong entities

Categories

(Calendar :: Lightning Only, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rimas, Assigned: Fallen)

References

()

Details

(Whiteboard: [good first bug])

Attachments

(2 files)

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.
Rimas, you filed this as a trunk-only bug. Is this really not happening on the 1.8 branch as well?
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.
(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?
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".
Version: Trunk → unspecified
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?
To #Comment 6:

Created separate bug 396159 for that because it concerns another entity.
Does the issue still exists in recent 0.9pre nightly builds after the context menu consolidation in Bug 430430?
Rimas, can you check if this issue still exists in Lightning 1.0pre, please?
Severity: normal → minor
Whiteboard: [good first bug]
Attached patch Fix - v1Splinter Review
This should take care
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #432620 - Flags: review?(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.
Attachment #432620 - Flags: review?(ssitter) → review-
Attached patch Fix - v2Splinter Review
Man I wonder why the previous patch even worked! Sorry about the bad state of the patch, I hope this one does better.
Attachment #432911 - Flags: review?(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".
Attachment #432911 - Flags: review?(ssitter) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b2
You need to log in before you can comment on or make changes to this bug.