Prefs -> Navigator radio buttons not underlining accesskeys in labels

RESOLVED FIXED in mozilla1.8beta2

Status

()

P1
normal
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: aaronlev, Assigned: aaronlev)

Tracking

({access})

Trunk
mozilla1.8beta2
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

14 years ago
Created attachment 159484 [details] [diff] [review]
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?
(Assignee)

Comment 2

14 years ago
Created attachment 159490 [details] [diff] [review]
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)
Attachment #159484 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #159490 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Updated

14 years ago
URL: ]
Priority: -- → P1
(Assignee)

Updated

14 years ago
Attachment #159490 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 3

14 years ago
Created attachment 159781 [details] [diff] [review]
Add Firefox part of patch, and fix unregistering of old accesskeys for radios and checkboxes
Attachment #159490 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #159781 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Updated

14 years ago
Blocks: 260605
(Assignee)

Comment 4

14 years ago
Created attachment 159860 [details] [diff] [review]
Switch order of accesskey attribute setting
Attachment #159781 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #159781 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 5

14 years ago
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
Attachment #159860 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 6

14 years ago
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.
Attachment #159860 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
(Assignee)

Comment 7

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

Updated

14 years ago
Attachment #159860 - Flags: review?(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.
Attachment #159860 - Flags: review?(mconnor) → review+
(Assignee)

Comment 9

14 years ago
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Updated

14 years ago
Keywords: aviary-landing
(Assignee)

Comment 10

14 years ago
Relanding relevant parts of patch following aviary branch landing.
Keywords: aviary-landing
Blocks: 282186
Created attachment 176799 [details] [diff] [review]
(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.
Attachment #176799 - Flags: superreview+
Attachment #176799 - Flags: review+
URL: ]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OS: Windows XP → All
Hardware: PC → All
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 }
Attachment #176799 - Attachment description: (Ev1) <text.xml> forgotten suggested fix → (Ev1) <text.xml> forgotten suggested fix [Checked in: Comment 12]
Attachment #176799 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8beta2

Updated

14 years ago
Attachment #176799 - Attachment is obsolete: false
You need to log in before you can comment on or make changes to this bug.