189416 - View buttons should be type=radio

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Joshua Paine]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030116
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030116

[entering details because bugzilla wants them--see other text]

Reproducible: Always

Steps to Reproduce:
1. Select a view (Day/Week/Month) with view menu, toolbar button, or hotkey.
Actual Results:  
Selected view button is shown disabled, other two buttons are enabled.

Expected Results:  
Selected view button should be depressed, indicating not only that you can't
press it, but that it's active.

The way to accomplish the desired effect is to set the view buttons
[type="radio" group="calendarViews"]. Mozilla takes care of the proper
button-depressing behavior if the user clicks a button, but if a view is
activated some other way (including the extremely common case of Calendar
startup), the active button is only disabled and not depressed. So we need to
add to the code that sets/removes the disabled attributes on the buttons.

I don't currently have the ability to make patches, but below is exactly what
needs to be done to fix the bug:

calendar.xul
-----------------
Lines that read:
<toolbarbutton class="toolbarbutton-1" id="calendar-day-view-button"      
label="&calendar.dayview.button.label;"        
tooltiptext="&calendar.dayview.button.tooltip;"      observes="day_view_command"/>
<toolbarbutton class="toolbarbutton-1" id="calendar-week-view-button"     
label="&calendar.weekview.button.label;"       
tooltiptext="&calendar.weekview.button.tooltip;"     observes="week_view_command"/>
<toolbarbutton class="toolbarbutton-1" id="calendar-month-view-button"    
label="&calendar.monthview.button.label;"      
tooltiptext="&calendar.monthview.button.tooltip;"    observes="month_view_command"/>

Should be replaced with:
<toolbarbutton type="radio" group="calendarViews" class="toolbarbutton-1"
id="calendar-day-view-button"       label="&calendar.dayview.button.label;"    
    tooltiptext="&calendar.dayview.button.tooltip;"     
observes="day_view_command"/>
<toolbarbutton type="radio" group="calendarViews" class="toolbarbutton-1"
id="calendar-week-view-button"      label="&calendar.weekview.button.label;"   
    tooltiptext="&calendar.weekview.button.tooltip;"    
observes="week_view_command"/>
<toolbarbutton type="radio" group="calendarViews" class="toolbarbutton-1"
id="calendar-month-view-button"     label="&calendar.monthview.button.label;"  
    tooltiptext="&calendar.monthview.button.tooltip;"   
observes="month_view_command"/>


dayView.js
------------------
Below line:
   dayViewButton.setAttribute( "disabled", "true" );

Add:
   monthViewButton.removeAttribute( "checked" );
   weekViewButton.removeAttribute( "checked" );
   dayViewButton.setAttribute( "checked", "true" );


weekView.js
----------------
Below line:
   dayViewButton.removeAttribute( "disabled" );

Add lines:
   monthViewButton.removeAttribute( "checked" );
   weekViewButton.setAttribute( "checked", "true" );
   dayViewButton.removeAttribute( "checked" );


monthView.js
----------------
Below line:
   dayViewButton.removeAttribute( "disabled" );

Add lines:
   monthViewButton.setAttribute( "checked", "true" );
   weekViewButton.removeAttribute( "checked" );
   dayViewButton.removeAttribute( "checked" );



And then all will be well.

Comment 1

[Mike Potter]

-> Me

Comment 2

[Joshua Paine]

Just realized that the six lines I suggested in the form
something.removeAttribute( "checked" );
are redundant since Mozilla takes care of unsetting checked on the other items
in the radio group. So cut those 6 lines and everything will still work well.

Comment 3

[Mike Potter]

This doesn't work for me on startup.  I can't seem to choose any of the views,
they are all disabled.

Comment 4

[Mike Potter]

OK, I got it working OK if I took out disabled="true" on the commands to change
the week view.
However, the UI for the buttons looks very bad (in my opinion) if that's done,
so I'm going to mark this bug as new again, until we have a perfect working
solution.

Comment 5

[Robert Morris]

Attachment: Appearance of toolbar buttons with this code

Made an attachment with the appearance of my buttons after I make these code
changes (adjusted to accomodate the newly added mutliweek view).

Is this what you mean when you say it looks "bad"? If so, I'd say. :-) Must we
work on a new graphic to get this to display pleasantly?

While we're at it, I'll at least work on the context menu items for switching
views and see what I can do there in turning them into radio buttons, since
both should behave as radio buttons if that's the way we want it (which I think
is a good idea).

Comment 6

[Joshua Paine]

I hadn't even looked at Modern--in Classic the buttons jump around and don't
maintain a consistent width. (I only suggested it in the first place b/c in my
modified Crystal-Classic theme the radio buttons look great without modification
and I didn't check the others.) Modern is even worse, but it would be nice to
have this right. I think the normal effect in context menus is to have the
selected one with a check mark next to it.

-Joshua <><

Comment 7

[Mike Potter]

New contact from [e-mail] to [e-mail]
Filter on string OttawaMBA to get rid of these messages. 
Sorry for the spam.

Comment 8

[Robert Morris]

Joshua, I think it's a good idea, too. I'd love to work on it, but my knowledge
of XUL is--well, bad. Although it's probably improved in the past two months, so
maybe I could start looking at this again and figuring it out. (I would assign
this bug to me, but I'm not sure I can do it ... so I'll leave it this way for now!)

Comment 9

[Robert Morris]

-> me. (Sorry, meant to comment this as I changed it, not after, to prevent
bugspam.)

Comment 10

[Robert Morris]

-> me. (Sorry, meant to comment this as I changed it, not after, to prevent
bugspam.)

Comment 11

[Robert Morris]

(In reply to comment #6)
> I hadn't even looked at Modern--in Classic the buttons jump around and don't
> maintain a consistent width. ...

I believe I just figured out why you see this happen: it's due to Mozilla's
Classic theme. See toolbarbutton.css (it's under global). In the section for
toolbarbutton[checked="true"], it changes the padding for some reason. Why, I
don't know. But it does look fine when I comment out the padding lines. Anybody
know what those are doing there--and why they set it twice? Mine has two lines,
the first being [no quotes]  "padding: 3px 1px 1px 3px;" and the second being
"padding: 2px !important;". Wouldn't the second cancel the first, making that
one useless? And is it necessary to change it in the first place? Mozilla
doesn't seem to use radio/check buttons on primary toolbars, just on the
Mail/News compose formatting mini-toolbar. And those buttons look fine with or
without this padding.

So, basically, that's a problem of the Mozilla Classic theme problem, not
Calendar itself.

BTW, if you're using Windows XP and have it using a Windows XP visual style (as
opposed to Windows classic), depressed radio buttons don't even look depressed.
That's also, apparently, an issue with the Classic theme, though it may be
harder to overcome. This happens even on the mail/news compose formatting
toolbar with the B, I, U, and alignment buttons.

Maybe there are bugs covering this already; I should look when I'm done here. 

As for Modern, I'll look at that, but I think it might be simpler; including an
additional "pressed" image may be all that's needed. Hopefully the (Mozilla)
Modern theme itself won't have to be changed. But no guarantees--I haven't
looked yet. =)

(Oh, and I am working on a patch; I could post a preliminary version soon if
anyone's interested in checking out what happens in these various circumstances.)

Comment 12

[Chris Cook]

Any progress on this patch? I'm working on updating classic and this fix would
be nice to see.

Comment 13

[Joshua Paine]

Chris,

Seems the only problems with this are theme problems. If you're updating
classic, I'd say just go ahead and fix it. It seems reasonable to me to let the
modern appearance drop if we have to, as it only exists at all when calendar is
attached to the suite. But I'm not in charge.

-Joshua <><

Comment 14

[Justin Wood (:Callek)]

We shouldnt degrade any currently established theme to make an eddition, we have
modern for suite, and letting it degrade without suite modern actually degrading
is a BAD idea, if its a shipped theme, we should have at least *something* which
works (and flows in context to its environment), in FF we can get away with a
new theme, hosted under *stripe, but in suite, we can't.  Suite is meant to flow
all apps `installed` together.

just my two cents.

Comment 15

[Chris Cook]

Attachment: Updated files for Mozilla Suite to implement view radio buttons

I've updated both modern and classic to support the button change. It required
no graphic changes to do so. I simply modified the Mozilla Suite extension jar
to test the changes.

I would be happy to make a patch but I don't really understand the cvs
structure of calendar so I'm not sure what files to diff against. Therefore,
here's a zip of the modified files. The calendar build they are based on is
2004080916.

There is one remaining bug which I have not been able to trace. Using the view
menu options or the F1-4 keys to change views does not update the associated
view button.

Comment 16

[Mostafa Hosseini]

Attachment: Patch based on zip file

I've created a patch for others to try, based on the previous attachment.

Comment 17

[Mostafa Hosseini]

Attachment: Patch2 based on zip file

Previous patch didn't include changes for calendar.xul

Comment 18

[Mostafa Hosseini]

Aside from the bug mentioned in comment #15, the active state looks barely
different than the others since it is just the hover icon.
Ccing gekacheka to see if he can help with this, or whether he has an opinion.
Also the code should be tested for compatibilty with the customizable-toolbar
feature.

Comment 19

[Chris Cook]

(In reply to comment #18)
Are you using WinXP with the Luna theme? Buttons of type radio don't display as
"checked" when using this OS theme. That is one argument against using radio
type buttons. However, Firefox does use this type of button (at least the
history and bookmarks buttons have a "checked" state) regardless of this bug.

Comment 20

[Mostafa Hosseini]

(In reply to comment #19)
> (In reply to comment #18)
> Are you using WinXP with the Luna theme? Buttons of type radio don't display as
> "checked" when using this OS theme
No. I'm on Linux

Comment 21

[Chris Cook]

It sounds like the same native widget bug though. Wish I could refer you to it
but I haven't been able to locate a bug for it. I have considered looking into
fixing it myself because it really annoys me but I haven't gotten around to it.

Comment 22

[Chris Cook]

(In reply to comment #18)
Mark and I have been working on updated view icons for the "active" state
because even without this patch, a grey "disabled" icon is not appropriate for
the button state. With the new graphics in place, this concern should be reduced.

Comment 23

[Robert Morris]

Someone feel free to assign this bug to yourself if you want--I haven't had the
time lately (other projects), and classes are about to start, so I definitely
won't any time soon. :-)

Comment 24

[Joey Minta]

(In reply to comment #23)
> Someone feel free to assign this bug to yourself if you want--I haven't had the
> time lately (other projects), and classes are about to start, so I definitely
> won't any time soon. :-)

Reassigning to default to get this back on the NEW list.

Comment 25

[Simon Paquet [:sipaq]]

Attachment: Updated patch from Mostafah

Patch with bitrot removed.
Thanks to Silver and jminta for the help and inspiration

Comment 26

[Simon Paquet [:sipaq]]

*** Bug 321138 has been marked as a duplicate of this bug. ***

Comment 27

[Joey Minta]

Comment on attachment 207219 [details] [diff] [review]
Updated patch from Mostafah

Unfortunately, I don't think it's quite this simple.

-  <command id="day_view_command"  oncommand="gCalendarWindow.switchToDayView()" disabled="true"/>
+  <command id="day_view_command"  oncommand="gCalendarWindow.switchToDayView()"/>

The context menu ( http://lxr.mozilla.org/mozilla/source/calendar/resources/content/calendar.xul#300 ) observes this command as well.  That menu-item needs to be disabled and it's still observing this.  You'll need to be a bit more creative in finding a solution to the fact that we now have both a radio-button and a menuitem that observes this command.

Index: calendar/sunbird/base/content/calendar.xul
===================================================================
RCS file: /cvsroot/mozilla/calendar/sunbird/base/content/calendar.xul,v

Don't forget that all changes made to sunbird's calendar.xul need to be mirrored in resources/content/calendar.xul, if we're to continue trying to semi-support calendar.xpi

 CalendarWindow.prototype.switchToDayView = function calWin_switchToDayView( )
 {
-    document.getElementById("day_view_command").setAttribute("disabled", true);
-    document.getElementById("week_view_command").removeAttribute("disabled");
-    document.getElementById("multiweek_view_command").removeAttribute("disabled");
-    document.getElementById("month_view_command").removeAttribute("disabled");
+    document.getElementById( "day_view_command" ).setAttribute( "checked", "true" );
     this.switchToView('day-view');
 }

These functions are so generic now that I'd like to eventually get rid of them in favor of a single switchToView(type) call.  That's one of the key steps towards allowing easy plugging of additional views by extensions.  That may be outside the scope of this bug, though.  It's up to you.

Comment 28

[James Ross]

(In reply to comment #27)
> You'll need to be a bit more creative in finding a
> solution to the fact that we now have both a radio-button and a menuitem that
> observes this command.

That's bollocks. The menu items should also be type=radio (if they're not already), and so should just get the blob next to them to mark the current one. Disabling it is not a standard UI practise that I've seen, and more to the point, it should be left to the Toolkit to decide if the currently selected menuitem with type=radio should appear disabled or not.

Comment 29

[Joey Minta]

(In reply to comment #28)
> The menu items should also be type=radio (if they're not
> already), 
They're not currently radio.  Switching them to also be radio does indeed seem to be a good solution.  I'd also be fine with removing them entirely from the context menu (that menu is way too complex), but I'd want mvl's opinion on that approach first.

Comment 30

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #29)

> I'd also be fine with removing them entirely from the
> context menu (that menu is way too complex)

I have no objections against removing the 'switch to...' options from the context menu. (But don't remove them from the main view menu)

Comment 31

[Simon Paquet [:sipaq]]

Attachment: Patch v2

>Unfortunately, I don't think it's quite this simple.

>-  <command id="day_view_command" oncommand="gCalendarWindow.switchToDayView()" disabled="true"/>
>+  <command id="day_view_command" oncommand="gCalendarWindow.switchToDayView()"/>
>
>The context menu 
>http://lxr.mozilla.org/mozilla/source/calendar/resources/content/calendar.xul#300
>observes this command as well.  That menu-item needs to be disabled and it's 
>still observing this.  You'll need to be a bit more creative in finding a 
>solution to the fact that we now have both a radio-button and a menuitem that 
>observes this command.

As proposed by Silver, I set the menuitems also to type="radio".

>Index: calendar/sunbird/base/content/calendar.xul
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/sunbird/base/content/calendar.xul,v
>
>Don't forget that all changes made to sunbird's calendar.xul need to be
>mirrored in resources/content/calendar.xul, if we're to continue trying to
>semi-support calendar.xpi

Done together with the corresponding changes in menuOverlay.xul.

> CalendarWindow.prototype.switchToDayView = function calWin_switchToDayView( )
> {
>-    document.getElementById("day_view_command").setAttribute("disabled", true);
>-    document.getElementById("week_view_command").removeAttribute("disabled");
>-   
document.getElementById("multiweek_view_command").removeAttribute("disabled");
>-    document.getElementById("month_view_command").removeAttribute("disabled");
>+    document.getElementById( "day_view_command" ).setAttribute( "checked",
"true" );
>     this.switchToView('day-view');
> }
>
>These functions are so generic now that I'd like to eventually get rid of them
>in favor of a single switchToView(type) call.  That's one of the key steps
>towards allowing easy plugging of additional views by extensions.  That may be
>outside the scope of this bug, though.  It's up to you.

Unfortunately while this works code works with the toolbarbuttons, it does not 
correctly work with the menuitems, so I had to bring it back.

Comment 32

[Simon Paquet [:sipaq]]

Comment on attachment 207267 [details] [diff] [review]
Patch v2

I forgot one thing. As proposed in comment 29, I removed the contextmenu-items for the different views from Sunbird and the Calendar extension.

Comment 33

[Joey Minta]

(In reply to comment #31)
> Unfortunately while this works code works with the toolbarbuttons, it does not 
> correctly work with the menuitems, so I had to bring it back.
> 
Why? Is this by design in xul or can you cite a bug somewhere that's filed on it?  I'm somewhat reminded of bug 287393, where we introduced radioGroupSelectItem http://lxr.mozilla.org/mozilla/source/calendar/resources/content/applicationUtil.js#368
Can something like this be used here too or is there a different problem?

Comment 34

[Simon Paquet [:sipaq]]

Attachment: Patch v2 (unbitrotted)

Same patch with bitrot removed

Comment 35

[Joey Minta]

Comment on attachment 209235 [details] [diff] [review]
Patch v2 (unbitrotted)

OK, I spent quite awhile fighting this, since it does seem like we should be able to lose the extra removeAttribute() code.  Unfortunately, the best I could come up with was something like    
    document.getElementById("calendar-view-menu-day").setAttribute("checked", "true");
    document.getElementById("calendar-day-view-button").checked = true;
    this.switchToView('day-view');
This worked nicely until I realized that it has bug 172090.  So it does seem there is no way to get rid of all the 
+    document.getElementById("month_view_command").removeAttribute("checked");
+    document.getElementById("multiweek_view_command").removeAttribute("checked");
+    document.getElementById("week_view_command").removeAttribute("checked");
+    document.getElementById("day_view_command").setAttribute("checked", true);
     this.switchToView('day-view');

I looked around the mozilla code-base, and found something similar in mail code for setting labels on messages.  They seem to have the same basic issue, and have to check/uncheck each menuitem. http://landfill.mozilla.org/mxr-test/mozilla/source/mailnews/base/resources/content/mailWindowOverlay.js#551

So with that, it seems that there are 2 issues here.  The first is what type of menu/toolbar button behavior best matches the semantics of the view-switching code.  The general agreement seems to be that checked radio buttons are better than the current disabled option, so in that respect, this patch helps.  The second issue, which this patch doesn't help with, is that code readability would be improved if we could get the full benefit of radio buttons, in that we should only need to set 1 as checked, and the rest should do their thing.

Actual review issues to be addressed:
+                   type="radio"
+                   group="calendarViews"
According to xul planet, you should use the 'name' attribute, not the 'group' attribute on menuitems http://xulplanet.com/references/elemref/ref_menuitem.html#attr_name  Using 'group' also seems to work, but I'd prefer to at least follow what documentation does exist.  So, please don't use 'group' here and don't change to 'group' in calendar-menubar.inc
-                     name="viewGroup"
+                     group="calendarViews"

Additionally, "calendarViews" is already the name of the toolbarbutton group.  I don't think you want to have two groups with the same name.  Please give them distinct names.

Comment 36

[Joey Minta]

Bug 172090 is probably what Simon ran into too, and I can't imagine a solution to this problem that wouldn't be relying on the problem in that bug not existing.  Adding dependency.

Comment 37

[Simon Paquet [:sipaq]]

Attachment: Patch v3

Patch with review comments addressed.

Comment 38

[Joey Minta]

Comment on attachment 210050 [details] [diff] [review]
Patch v3

     <toolbarbutton class="toolbarbutton-1"
                    id="calendar-day-view-button"
+                   type="radio"
+                   name="calendarViews"
                    label="&calendar.dayview.button.label;"

According to http://xulplanet.com/references/elemref/ref_toolbarbutton.html#attr_group toolbarbuttons use 'group', unlike menuitems which use 'name'.  This is incredibly confusing to me, so maybe Silver wants to jump in, but otherwise I think we're going to need to follow what they say: 'group' for toolbarbuttons, 'name' for menuitems. sigh...

Comment 39

[James Ross]

You are correct: buttons (inc. toolbarbuttons) use the "group" attribute, and menuitems use "name".

References:
 http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/button.xml#52
 http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuFrame.cpp#1346

Comment 40

[Simon Paquet [:sipaq]]

(In reply to comment #38)

> (From update of attachment 210050 [details] [diff] [review] [edit])
>      <toolbarbutton class="toolbarbutton-1"
>                     id="calendar-day-view-button"
> +                   type="radio"
> +                   name="calendarViews"
>                     label="&calendar.dayview.button.label;"
> According to
> http://xulplanet.com/references/elemref/ref_toolbarbutton.html#attr_group
> toolbarbuttons use 'group', unlike menuitems which use 'name'.  This is
> incredibly confusing to me, so maybe Silver wants to jump in, but otherwise I
> think we're going to need to follow what they say: 'group' for 
> toolbarbuttons, 'name' for menuitems. sigh...

Well, it seemed to work with "name" all over the place, but as Silver said, this is not correct and should be corrected.

Joey, do you need a new patch for this or can you change that on checkin for calendar/resources/content/calendar.xul and calendar/sunbird/base/content/calendar.xul?

Comment 41

[Joey Minta]

Comment on attachment 210050 [details] [diff] [review]
Patch v3

r=jminta with the group/name change, which I'll fix prior to checkin.  Thanks for all the time you put in on this Simon.

Comment 42

[Joey Minta]

"Patch v3" checked in.  Leaving bug open until the dependencies get sorted out and we can finish the cleanup here.

Comment 43

[Simon Paquet [:sipaq]]

Okay, I need some help from the XUL gurus once again. I tried to fiddle around in Thunderbird's DOM Inspector with Lightning today, trying the same approach that I took for Sunbird for the Lightning buttons and menuitems.

First the good news:
Inserting an attribute "type" with value "radio" and an attribute "group" with value "calendarViews" for all three calendar view buttons worked like a charm. The currently enabled view button shows up in a pressed state.

Now the bad news:
Doing nearly the same for the three calendar-view-menuitems (using the attribute "name" instead of "group" and the attribute value "calendarMenuViews" instead of "calendarViews") doesn't work. There are two bugs:

1. The button state is not updated when I change the view via the menuitems, 
   e.g. if I'm in the week view and go to the day view, the week view button is 
   still in a pressed state. As far as I can see Sunbird has observers in 
   place, which observe the the current view status. Lightning does not, so 
   introducing these observers should fix that.
2. The menuitems do not react to the type="radio" attribute. There is no small 
   dot on the right of the active view. Does anyone have an idea, why that is 
   the case?

Comment 44

[Simon Paquet [:sipaq]]

The patch in bug 376086 (attachment 271539 [details] [diff] [review]) contains a fix for this bug.

Comment 45

[Simon Paquet [:sipaq]]

Fixed by the checkin for bug 376086.