260527 - Prefs -> Navigator radio buttons not underlining accesskeys in labels

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[Aaron Leventhal]

The navigator pref window radio buttons (Home page, Blank page, Last page) are
actually in a deck which changes depending on the combo box choice. The
accesskey is applied late to the radio button.

There are several problems with the code:
1. The prefwindow sets accesskey dynamically via attribute instead of js property
2. Setting accesskey dynamically doesn't update the appearance of <radio
label="foo">

Comment 1

[Aaron Leventhal]

Attachment: Still has one problem

Unfortunately, the accesskeys disappear when you down arrow and then up arrow
back to "Navigator" in the category pane.

Neil, any idea why?

Comment 2

[Aaron Leventhal]

Attachment: Four-fold fix

1) Change h->m for Ho_m_e page which conflicts with _H_elp 
2) Use accesskey property instead of attribute for dynamic changes
3) Prefer accesskey attribute be stored on control, not label which is often an
anonymous part of the control
4) Call setPageAccessKeys() each time panel is shown, not just first time
(otherwise accesskeys disappear when you go away and back to that panel)

Comment 3

[Aaron Leventhal]

Attachment: Add Firefox part of patch, and fix unregistering of old accesskeys for radios and checkboxes

Comment 4

[Aaron Leventhal]

Attachment: Switch order of accesskey attribute setting

Comment 5

[Aaron Leventhal]

Comment on attachment 159860 [details] [diff] [review]
Switch order of accesskey attribute setting

All I did was switch the order of some lines of code in general.xml

Comment 6

[[e-mail]]

Comment on attachment 159860 [details] [diff] [review]
Switch order of accesskey attribute setting

>@@ -247,12 +247,10 @@ function init()
>   } catch(e) {
>   }
> 
>   gData.navigatorData = navigatorData;
> 
>-  setPageAccessKeys(document.getElementById("behaviourDeck").firstChild);
>-
>   prefWindow.registerOKCallbackFunc(doOnOk);
> }
> 
> function Startup()
> {
>@@ -265,10 +263,12 @@ function Startup()
>     setHomePageValue(homePages[0]);
>   else
>     setHomePageValue(navigatorData.groupIsSet);
> 
>   updateHomePageButtons();
>+
>+  setPageAccessKeys(document.getElementById("behaviourDeck").firstChild);
> }
Not sure why you moved this as Startup() calls init().

>+            var accessKey;
I think you should initialize it rather than leaving it undefined if there's no
labelled control element.

Comment 7

[Aaron Leventhal]

I moved setPageAccessKeys to Startup() because Init() checks to see if
initialization has already happened. If we don't move it, then down arrow and up
arrow again and the accesskeys dissappear.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 159860 [details] [diff] [review]
Switch order of accesskey attribute setting

Ok.  With neil's comments about not leaving accessKey as undefined, r=me.

Comment 9

[Aaron Leventhal]

Checking in toolkit/content/widgets/text.xml;
/cvsroot/mozilla/toolkit/content/widgets/text.xml,v  <--  text.xml
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/content/widgets/general.xml;
/cvsroot/mozilla/toolkit/content/widgets/general.xml,v  <--  general.xml
new revision: 1.6; previous revision: 1.5
done
Checking in xpfe/components/prefwindow/resources/content/pref-navigator.js;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-navigator.js,v
 <--  pref-navigator.js
new revision: 1.16; previous revision: 1.15
done
Checking in xpfe/components/prefwindow/resources/locale/en-US/pref-navigator.dtd;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/locale/en-US/pref-navigator.dtd,v
 <--  pref-navigator.dtd
new revision: 1.25; previous revision: 1.24
done
Checking in xpfe/global/resources/content/bindings/general.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/general.xml,v  <-- 
general.xml
new revision: 1.32; previous revision: 1.31
done
Checking in xpfe/global/resources/content/bindings/text.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/text.xml,v  <--  text.xml
new revision: 1.13; previous revision: 1.12
done
Checking in content/xul/content/src/nsXULElement.cpp;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v  <--  nsXULElement.cpp
new revision: 1.546; previous revision: 1.545
done

Comment 10

[Aaron Leventhal]

Relanding relevant parts of patch following aviary branch landing.

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) <text.xml> forgotten suggested fix [Checked in: Comment 12]

Keeping
{{
neil.parkwaycc.co.uk: superreview+
mconnor: review+ 
}}
from
{{
(In reply to comment #6)
> (From update of attachment 159860 [details] [diff] [review] [edit])
> >+		var accessKey;
> I think you should initialize it rather than leaving it undefined if there's
no
> labelled control element.

(In reply to comment #8)
> (From update of attachment 159860 [details] [diff] [review] [edit])
> Ok.  With neil's comments about not leaving accessKey as undefined, r=me.
}}

Aaron (or Neil), Can you check it in on my behalf ? Thanks.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 176799 [details] [diff] [review]
(Ev1) <text.xml> forgotten suggested fix
[Checked in: Comment 12]


Check in: { 2005-03-10 06:29	aaronleventhal%moonset.net	mozilla/
toolkit/ content/ widgets/ text.xml    1.12 }