918239 - Calendar list can be collapsed making it impossible to restore

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

Steps:

1. Collapse the calendar list with the mouse
2. Try to uncollapse it - I couldn't find the grippy
3. Try to uncollapse via View > Calendar > Calendar List - doesn't work.

Workaround in Error Console:

top.opener.document.getElementById("ltnSidebar").removeAttribute("collapsed");

It should be possible to uncollapse it somehow, either via grippy or menu.

Paenglab, maybe this is a bug for you?

Comment 2

[Richard Marti (:Paenglab)]

Yes I can try it. I propose to add a menuitem in View > Calendar directly above Mini-Month and Calendar-List. What would be a good name for it, Calendar-Sidebar?

Comment 3

[Richard Marti (:Paenglab)]

Attachment: WIP

This is a first rough patch. The localization is missing, now with hard coded name. It toggles the sidebar but the checkmark isn't updated correctly. How could I add a observer which checks the sidebar's or splitter's state?

Comment 4

[Stefan Sitter [:ssitter]]

Do we need an additional menu entry? Maybe we can extend command handler of the existing menu entries, e.g. if someone enables display of one of the elements in the calendar pane we ensure that the pane will be shown if it was hidden?

Comment 5

[Richard Marti (:Paenglab)]

I think this would be weird. When the sidebar is collapsed by error then it should be better to have a special menu entry for this. It wouldn't be very logical to to click on a menu of a sidebar child to show the sidebar. When the sidebar would be hidden, should then the childs menu entries be unticked?

Because this bug is [wanted-2.6.x] I think it should be made simple and I hope it can made with already existing localization strings.

For the nightly we could maybe move the "Mini-Month" and "Calendar List" in a "Calendar Sidebar" submenu and add a "Show Sidebar" menu entry above the two entries.

Comment 6

[Richard Marti (:Paenglab)]

Attachment: Patch for 2.6.1, Beta and Aurora

This patch is for 2.6.1 up to Aurora. It shouldn't land in c-c. In collapsed state the splitter becomes wider and has it's standard appearance. I had to add a -moz-margin-start: 1px; to be able to move the splitter and not the window border.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: Patch for 2.6.1 to Aurora in action

Comment 8

[Richard Marti (:Paenglab)]

Attachment: Patch for c-c

This patch is for c-c implements a new menu "Calendar Pane" with the sub menus "Show Calendar Pane", "Mini Month" and "Calendar List".

I'm only unsure if we should rename "Mini Month" and "Calendar List" to "Show Mini Month" and "Show Calendar List" like in the Today Pane menu.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: Patch for c-c in action

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 808146 [details] [diff] [review]
Patch for 2.6.1, Beta and Aurora


>+#calsidebar_splitter[state="collapsed"] {
>+  min-width: 5px;
Is min-width really needed? The splitter itself shouldn't be resizable.

>+  width: 5px;
>+  -moz-margin-start: 1px;
>+  background: url("chrome://global/skin/splitter/dimple.png") transparent no-repeat center;
>+}

r=philipp. I'm not quite sure where to push the changesets for 2.6.1, so I'll wait with the release approval.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 808148 [details] [diff] [review]
Patch for c-c


>+
>+    let calSidebarMenu = document.getElementById("ltnViewCalendarPane");
>+    if (calSidebarMenu)
>+      calSidebarMenu.setAttribute("checked", !calSidebar.getAttribute("collapsed"));
>+
>+    let calSidebarAppMenu = document.getElementById("appmenu_ltnViewCalendarPane");
>+    if (calSidebarAppMenu)
>+      calSidebarAppMenu.setAttribute("checked", !calSidebar.getAttribute("collapsed"));
Brackets even for one-line if()'s. What cases are there that these elements are null? If they are always available you can use our helper setBooleanAttribute("appmenu_ltnViewCalendarPane", "checked", !calSidebar.getAttribute("collapsed")); This helper also takes care of removing the attribute instead of setting it false, in case there are css selectors that falsely use  .node[checked].


>+<!ENTITY lightning.toolbar.calendarmenu.label        "Calendar Pane">
>+<!ENTITY lightning.toolbar.calendarmenu.accesskey    "P">
>+<!ENTITY lightning.toolbar.calendarpane.label        "Show Calendar Pane">
>+<!ENTITY lightning.toolbar.calendarpane.accesskey    "o">
Would it work to use P as the accesskey again? Its in a submenu, so I would say so. 

The last guidelines about accesskeys I read say that its best to use a letter that is very prominent for the phrase, i.e its about a Pane, so "P" would be best. More guidelines not relevant to this code but FYI: If that doesn't work, then the next captial letter. Avoid narrow letters like i, and if all fails take anything thats left.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: Patch for c-c

I'm asking for review again to be sure my changed js is okay.

(In reply to Philipp Kewisch [:Fallen] from comment #11)
> Comment on attachment 808148 [details] [diff] [review]
> Patch for c-c
> 
> What cases are there that these elements are null?

ltnViewCalendarPane is always available -> removed the if().
appmenu_ltnViewCalendarPane isn't available on SM.

> If they are always available you can use our helper
> setBooleanAttribute("appmenu_ltnViewCalendarPane", "checked",
> !calSidebar.getAttribute("collapsed")); This helper also takes care of
> removing the attribute instead of setting it false, in case there are css
> selectors that falsely use  .node[checked].

I changed it to:

// Initialize the Calendar sidebar menu state
function InitViewCalendarPaneMenu() {
    let calSidebar = document.getElementById("ltnSidebar");

    setBooleanAttribute("ltnViewCalendarPane", "checked",
                        !calSidebar.getAttribute("collapsed"));

    if (document.getElementById("appmenu_ltnViewCalendarPane")) {
        setBooleanAttribute("appmenu_ltnViewCalendarPane", "checked",
                            !calSidebar.getAttribute("collapsed"));
    }
}

Is this okay like this or should I let the old "let, if" for appmenu_ltnViewCalendarPane?

> >+<!ENTITY lightning.toolbar.calendarmenu.label        "Calendar Pane">
> >+<!ENTITY lightning.toolbar.calendarmenu.accesskey    "P">
> >+<!ENTITY lightning.toolbar.calendarpane.label        "Show Calendar Pane">
> >+<!ENTITY lightning.toolbar.calendarpane.accesskey    "o">
> Would it work to use P as the accesskey again? Its in a submenu, so I would
> say so. 

Done

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Please push attachment 808146 [details] [diff] [review] to comm-aurora, comm-beta and comm-esr24 and leave open for the comm-central patch.

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 812773 [details] [diff] [review]
Patch for c-c

c-c code looks fine to me. Stefan, are you ok with the new UI here?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/comm-aurora/rev/2052311a9a16
https://hg.mozilla.org/releases/comm-beta/rev/923154a407c8
https://hg.mozilla.org/releases/comm-esr24/rev/81048f4f9b1c

Comment 16

[Stefan Sitter [:ssitter]]

Comment on attachment 812773 [details] [diff] [review]
Patch for c-c

I think this is OK for now. 

When looking at the menu structure I think there might be chance to make our menus more similar to each other. For example menu could be: 

> View - Today Pane    - Show Today Pane | Show Mini-Day | Show Mini-Month ...
>        Calendar Pane - Show Calendar Pane | Mini-Month | Calendar List ...
>        Calendar Tab  - Show Calendar Tab | Day | Week | Multi-Week ...
>        Task Tab      - Show Task Tab | Filter Tasks | ...

But this might be discussed in new bug.

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Followup bug sounds good to me. Richard or Stefan, could you file it?

https://hg.mozilla.org/comm-central/rev/d4099f72c46d

Comment 18

[Ken Shaffer]

In my download of lightning 2.6 for mac, I see no lightning-toolbar.dtd to patch.

Comment 19

[Richard Marti (:Paenglab)]

(In reply to Ken Shaffer from comment #18)
> In my download of lightning 2.6 for mac, I see no lightning-toolbar.dtd to
> patch.

lightning-toolbar.dtd is inside lightning-en-US.jar.

Comment 20

[Ken Shaffer]

awesome, now I'm fully patched.