View buttons should be type=radio

RESOLVED FIXED in 0.7

Status

--
minor
RESOLVED FIXED
16 years ago
11 years ago

People

(Reporter: lists, Assigned: sipaq)

Tracking

(Depends on: 1 bug)

unspecified
Dependency tree / graph

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

16 years ago
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

16 years ago
-> Me
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Reporter)

Comment 2

16 years ago
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

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

Comment 4

16 years ago
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.
Status: ASSIGNED → NEW

Comment 5

16 years ago
Created attachment 126507 [details]
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).
(Reporter)

Comment 6

16 years ago
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

15 years ago
New contact from mikep@oeone.com to mostafah@oeone.com
Filter on string OttawaMBA to get rid of these messages. 
Sorry for the spam.
Assignee: mikep → mostafah

Comment 8

15 years ago
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!)

Updated

15 years ago
Status: NEW → ASSIGNED

Updated

15 years ago
Assignee: mostafah → robert.moz
Status: ASSIGNED → NEW

Comment 9

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

Comment 10

15 years ago
-> me. (Sorry, meant to comment this as I changed it, not after, to prevent
bugspam.)
Status: NEW → ASSIGNED

Comment 11

15 years ago
(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

15 years ago
Any progress on this patch? I'm working on updating classic and this fix would
be nice to see.
(Reporter)

Comment 13

15 years ago
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 <><
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

14 years ago
Created attachment 156002 [details]
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

14 years ago
Created attachment 156277 [details] [diff] [review]
Patch based on zip file

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

Comment 17

14 years ago
Created attachment 156279 [details] [diff] [review]
Patch2 based on zip file

Previous patch didn't include changes for calendar.xul
Attachment #156277 - Attachment is obsolete: true

Comment 18

14 years ago
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

14 years ago
(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

14 years ago
(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

14 years ago
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

14 years ago
(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

14 years ago
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

14 years ago
(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.
Assignee: robert.moz → mostafah
Status: ASSIGNED → NEW
(Assignee)

Updated

13 years ago
QA Contact: gurganbl → sunbird
(Assignee)

Comment 25

13 years ago
Created attachment 207219 [details] [diff] [review]
Updated patch from Mostafah

Patch with bitrot removed.
Thanks to Silver and jminta for the help and inspiration
Attachment #156279 - Attachment is obsolete: true
Attachment #207219 - Flags: first-review?(jminta)
(Assignee)

Comment 26

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

Comment 27

13 years ago
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.
Attachment #207219 - Flags: first-review?(jminta) → first-review-

Comment 28

13 years ago
(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

13 years ago
(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.


(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)

(Assignee)

Comment 31

13 years ago
Created attachment 207267 [details] [diff] [review]
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.
Assignee: mostafah → bugzilla
Attachment #207219 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #207267 - Flags: first-review?(jminta)
(Assignee)

Comment 32

13 years ago
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

13 years ago
(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?
(Assignee)

Comment 34

13 years ago
Created attachment 209235 [details] [diff] [review]
Patch v2 (unbitrotted)

Same patch with bitrot removed
Attachment #207267 - Attachment is obsolete: true
Attachment #209235 - Flags: first-review?(jminta)
Attachment #207267 - Flags: first-review?(jminta)

Comment 35

13 years ago
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.
Attachment #209235 - Flags: first-review?(jminta) → first-review-

Comment 36

13 years ago
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.
Depends on: 172090
(Assignee)

Comment 37

13 years ago
Created attachment 210050 [details] [diff] [review]
Patch v3

Patch with review comments addressed.
Attachment #126507 - Attachment is obsolete: true
Attachment #156002 - Attachment is obsolete: true
Attachment #209235 - Attachment is obsolete: true
Attachment #210050 - Flags: first-review?(jminta)

Comment 38

13 years ago
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

13 years ago
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
(Assignee)

Comment 40

13 years ago
(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

13 years ago
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.
Attachment #210050 - Flags: first-review?(jminta) → first-review+

Comment 42

13 years ago
"Patch v3" checked in.  Leaving bug open until the dependencies get sorted out and we can finish the cleanup here.
(Assignee)

Comment 43

13 years ago
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?
(Assignee)

Comment 44

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

Updated

11 years ago
Component: Sunbird Only → Calendar Views
OS: Windows 2000 → All
QA Contact: sunbird → views
Hardware: PC → All
(Assignee)

Updated

11 years ago
Depends on: 376086
(Assignee)

Comment 45

11 years ago
Fixed by the checkin for bug 376086.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7
You need to log in before you can comment on or make changes to this bug.