Closed Bug 918239 Opened 8 years ago Closed 8 years ago

Calendar list can be collapsed making it impossible to restore

Categories

(Calendar :: Calendar Frontend, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Paenglab)

References

Details

Attachments

(4 files, 2 obsolete files)

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?
Duplicate of this bug: 736064
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?
Attached patch WIP (obsolete) β€” β€” Splinter Review
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?
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?
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.
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.
Assignee: nobody → richard.marti
Attachment #807169 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #808146 - Flags: review?(philipp)
Attachment #808146 - Flags: approval-calendar-release?(philipp)
Attachment #808146 - Flags: approval-calendar-beta?(philipp)
Attachment #808146 - Flags: approval-calendar-aurora?(philipp)
Attached patch Patch for c-c (obsolete) β€” β€” Splinter Review
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.
Attachment #808148 - Flags: review?
Attached image Patch for c-c in action β€”
Blocks: ltn261
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.
Attachment #808146 - Flags: review?(philipp)
Attachment #808146 - Flags: review+
Attachment #808146 - Flags: approval-calendar-beta?(philipp)
Attachment #808146 - Flags: approval-calendar-beta+
Attachment #808146 - Flags: approval-calendar-aurora?(philipp)
Attachment #808146 - Flags: approval-calendar-aurora+
Attachment #808148 - Flags: review? → review?(philipp)
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.
Attachment #808148 - Flags: review?(philipp) → review+
Attached patch Patch for c-c β€” β€” Splinter Review
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
Attachment #808148 - Attachment is obsolete: true
Attachment #812773 - Flags: review?(philipp)
Attachment #808146 - Attachment description: Patch for 2.6.1 to Aurora → Patch for 2.6.1, Beta and Aurora
Attachment #808146 - Flags: approval-calendar-release?(philipp) → approval-calendar-release+
Please push attachment 808146 [details] [diff] [review] to comm-aurora, comm-beta and comm-esr24 and leave open for the comm-central patch.
Keywords: checkin-needed
Whiteboard: [wanted-2.6.x] → [wanted-2.6.x][leave open]
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?
Attachment #812773 - Flags: ui-review?(ssitter)
Attachment #812773 - Flags: review?(philipp)
Attachment #812773 - Flags: review+
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.
Attachment #812773 - Flags: ui-review?(ssitter) → ui-review+
Followup bug sounds good to me. Richard or Stefan, could you file it?

https://hg.mozilla.org/comm-central/rev/d4099f72c46d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → 2.6.1
In my download of lightning 2.6 for mac, I see no lightning-toolbar.dtd to patch.
(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.
awesome, now I'm fully patched.
You need to log in before you can comment on or make changes to this bug.