Preferences panel access keys don't work when category tree or if OK/Cancel/Help buttons have focus

VERIFIED FIXED

Status

()

Core
Keyboard: Navigation
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: Jesse Ruderman, Assigned: Gilbert Fang)

Tracking

({access})

Trunk
x86
All
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

16 years ago
Steps to reproduce:
1. Open prefs.
2. Leave focus in the category tree, or focus one of the ok/cancel/help buttons.
3. Alt+A to focus the location field.

Result: Mozilla beeps.
Expected: Focus should move to the location field.

See also bug 64606, "page accesskeys don't work when location bar has focus".
(Reporter)

Updated

16 years ago
Keywords: access
(Reporter)

Updated

16 years ago
Blocks: 18575

Comment 1

16 years ago
030808 trunk build for MacOS9 seems WFM.

Comment 2

16 years ago
I believe, you meant Alt+L to focus the location field, not Alt+A (L is
underlined). I also see that other mnemonic Alt+R to activate 'Restore Default'
button - doesn't work as well.
i see this on linux as well [2002.03.13.08 comm bits].

olga, jesse is correct --in the Navigator preferences panel, the mnemonic for
Location is the (underlined) 'a'.

workaround: hit tab to move focus from the category to the pref panel, then the
accesskeys work. Aaron, this shouldn't be necessary, should it? if so, perhaps
this should be nominated?
OS: Windows 98 → All
Hardware: PC → All
clarifying summary a bit --when focus is in either the category item or the main
OK / Cancel / Help window buttons, the dialog accesskeys won't work.
Summary: pref panel accesskeys don't work when category tree or dialog button has focus → pref panel accesskeys don't work when category tree or if OK/Cancel/Help buttons have focus
*** Bug 136206 has been marked as a duplicate of this bug. ***

Comment 6

16 years ago
Changing summary to catch dupes.
Summary: pref panel accesskeys don't work when category tree or if OK/Cancel/Help buttons have focus → Preferences panel accelerator keys don't work when category tree or if OK/Cancel/Help buttons have focus

Updated

16 years ago
Blocks: 78261

Comment 7

15 years ago
Assigning to Gilbert Fang - this is just like bug 130644.

Keystrokes should be made available to all iframe's in a dialog, if they're not
first handled by the focused iframe.

CC'ing Bryner and Saari for comment on the proposed fix.
Assignee: aaronl → gilbert.fang
(Assignee)

Comment 8

15 years ago
okay, aaronl. that is my pleasure.
(Assignee)

Comment 9

15 years ago
the underlying reason is 
(from http://www.mozilla.org/projects/ui/accessibility/mozkeyintro.html)
# Accesskey: an attribute in HTML or XUL for defining an *accelator*. ...  
and
# Accelerator: a key who's behavior is defined locally by the widget the *focus*
is currently in.

But from the user's view, that is not convenient. I think we should change the
accesskey's definition to :
an attribute in HTML or XUL for defining an specail *accelator* which effective
on the focus *window*. ....

For some scenario, i suggest that
1. menuitem accesskey, as current, only effecive when the menus pop up.
2. xul-widget accesskey, only effective when they are not hidden. I think it is
almost the current status. For eg. in pref dialog, only the iframe showing
should  has effective accesskey.
3. if there are conflicts, i suggest the priorities should be , from highest to
lowest, focused widget, focused widget's sibling, focused widget's
container/parent 's siblings, etc.. , focused window. 

Note:  rule 2 and rule3  should satisfy the bug 64606.

Hi, how about others' suggestion? 
 
(Assignee)

Comment 10

15 years ago
For the implementation of my suggestion, one of the ways is
to make all the accesskey registered to the top-level window. 
But i do not think it is a good idea, because then the top-level window should
care too much things, initially finding all the accesskey from all the sub
domxul node including their the bindings, listening the dom change event because
maybe some codes dynamically appending accesskey attributes to a domsul  node,
anyway , I am not so sure about this. 

The other way is we do some event bubble-down with most carefulness. I suggest
adding a new event handling state--NS_EVENT_FLAG_EXPANDING

It will cause a little performance issuse, but i think it will not cost too much
if we only expanding key events with the IsAlt== true. mouse event and other
events do not need expanding. 

         
(Assignee)

Comment 11

15 years ago
oh, pls forgetting what i said in Comment #10.
Now, I think it should be handled in the ESM::PreHandleEvent.
After directly the local ESM processing the accesskey, it is a good place to
check all his sub docShell to see whether the key press  is an accesskey. 
 
(Assignee)

Comment 12

15 years ago
Created attachment 95820 [details] [diff] [review]
checking all sub docshell's accesskey

Comment 13

15 years ago
Gilbert, does this handle when there are more than 2 levels deep of docshells?

For example,
      docshell
     /        \
  docshell   docshell
 /   \          /   \
ds    ds       ds    ds
   
The event can occur on any one of those, right?
(Assignee)

Comment 14

15 years ago
yes, aaronl. that is like a recursive process. every ESM checks all its
docshell's sub docshells. so it can traversing into any level of doc shell. 
(Assignee)

Comment 15

15 years ago
oh, sorry, aaronl. if you mean the event is in sub docshell 2 levels deeper.
then it may be not.  

Comment 16

15 years ago
Maybe I'm wrong, but I think the event can occur in any of the docshells.

      docshell
     /        \
  docshell   docshell
  /   \          /   \
*ds    ds       ds    ds*  <-- accesskey needs to be processed here

^
|
accesskey
event
occurs
here
(Assignee)

Comment 17

15 years ago
i am not sure.
what i think is the presShell will handle the event firstly, and make the pass
the key press event to the top-level docshell's ESM . 
for example, open a new brower window, ctrl-T to create at least 2 tabs. then in
one tab, go to resource:///res/samples/test9.html,  make the focus in the frame1.
Then alt-T still  works, so i think there is already some mechanism to pass the
event to the top-level docshell's ESM. 

(Assignee)

Comment 18

15 years ago
Comment on attachment 95820 [details] [diff] [review]
checking all sub docshell's accesskey

>+            nsCOMPtr<nsIDocShellTreeItem> subShellItem;
>+            docShell->GetChildAt(counter, getter_AddRefs(subShellItem));
>+            nsCOMPtr<nsIDocShell> subDS = do_QueryInterface(subShellItem);
>+            if (subDS) {
>+              nsCOMPtr<nsIPresShell> subPS;
>+              subDS->GetPresShell(getter_AddRefs(subPS));
>+
>+              nsCOMPtr<nsIPresContext> subPC;

It is better to change 
 if (subDS) {
to 
  if (subDS && IsShellVisible(subDS)) {
Attachment #95820 - Flags: needs-work+
(Assignee)

Comment 19

15 years ago
hi, joki and saari
would u please review the patch?
(Assignee)

Comment 20

15 years ago
Created attachment 95962 [details] [diff] [review]
new patch after my commnent 18

need r=?
(Assignee)

Updated

15 years ago
Attachment #95820 - Attachment is obsolete: true
(Assignee)

Comment 21

15 years ago
The following is from bryner on IRC today
<bryner> this patch is pretty scary
<bryner> i've got a couple of concerns here:
<bryner> 1. i want to make sure this doesn't slow down typing
<bryner> 2. i'd prefer if the subdocument ESM was only asked to do the accesskey
handling, as opposed to all the stuff PreHandleEvent does.  functionally, they
should be the same, but i'd prefer if the accesskey handling was factored out of
PreHandleEvent into its own method, and you just call that method on the subdoc ESM.
<bryner> that way we don't risk extra stuff happening as a result of calling
PreHandleEvent
(Assignee)

Comment 22

15 years ago
But I donot think it is really scary. 
1#. my codes is processed only when accesskey happens --- keypress event with
accesskey modifier is true. And in my w2k box, mozilla has no apparently 
affected in speed at all. And in most case, docShell is less than 4 levels or
even less. 
2# That is really a good solution. The PreHandleEvent is handling too much things.
If we factor out the accesskey handling by its own method, then we have to
change the *idl*. Would that be Okay, bryner?
 
Well, actually nsIEventStateManager is not an idl interface.  But you could also
just put it as a protected method on nsEventStateMangaer, since you're doing
this from inside the same class, and just cast the subdocument's ESM to
nsEventStateManager*.
(Assignee)

Comment 24

15 years ago
Created attachment 96669 [details] [diff] [review]
factor out the HandleAccessKey from nsEventStateManager

need r=?
Attachment #95962 - Attachment is obsolete: true
Attachment #96669 - Flags: review+
Comment on attachment 96669 [details] [diff] [review]
factor out the HandleAccessKey from nsEventStateManager

>+void
>+nsEventStateManager::HandleAccessKey(nsIPresContext* aPresContext, nsKeyEvent *aEvent, nsEventStatus* aStatus)
>+{
>+  //This method is experimental, so here is no strict parameter checking.
>+  //and it is supposed that this method is only called in PreHandleEvent after checking the access
>+  //modifier key is down.
>+

What exactly do you mean that this is "experimental"? That part of the comment
is confusing.  Also, the prevailing style for // comments is to have a space
between '/' and the beginning of the comment.

>+
>+        nsIEventStateManager * iesm = subESM;
>+        nsEventStateManager * esm = NS_STATIC_CAST(nsEventStateManager *, iesm);

I think you can do without the temporary here:

nsEventStateManager* esm = NS_STATIC_CAST(nsEventStateManager*, subESM.get());

I can't remember offhand if that actually works though, so you should test it.

>+        if (esm) {
>+          esm->HandleAccessKey(subPC, aEvent, aStatus);
>+        }
>+
>+        if (nsEventStatus_eConsumeNoDefault == *aStatus) {
>+          break;
>+        }

Please be consistent with whether single-line |if| bodies are made into blocks.

r=bryner with those fixes.
(Assignee)

Comment 26

15 years ago
Created attachment 96970 [details] [diff] [review]
new patch after bryner's review
Attachment #96669 - Attachment is obsolete: true
(Assignee)

Comment 27

15 years ago
Comment on attachment 96970 [details] [diff] [review]
new patch after bryner's review 

Carrying bryner's review
Attachment #96970 - Flags: review+
+  //Alt or other accesskey modifier is down, we may need to do an accesskey
+    //Someone registered an accesskey.  Find and activate it.
+          //B) Click on it if the users prefs indicate to do so.

I know you just copied those, but please add spaces after the "//"

+  // after the local  accesskey handling

Extra space after "local"

+    //checking all sub docshell;

Space after "//"?  "sub docshells", remove the ';'

+    nsCOMPtr<nsIDocShellTreeNode> docShell(do_QueryInterface(pcContainer));

Assert of |docShell| is null after this?

+      nsCOMPtr<nsIPresShell> subPS;

You never use this anywhere...

Looking at this code, you're checking the children of the docshell.  So if you have:

     A
   /   \
  1     2
 / \   / \
a   b c   d

And an event happens in "2", the code will check for accesskeys in "2", "c", and
"d" but not in "A", "1", "a", or "b", correct?  That does not match comment 9's
list...

Or is PreHandleEvent called on the ESM of "A" as well as on the ESM of "2"?
(Assignee)

Comment 29

15 years ago
Created attachment 97491 [details]
a testcase for examing comments 28's scenario

This testcase is for 
    A
   /   \
  1	2
 / \   / \
a   b c   d
(Assignee)

Comment 30

15 years ago
Created attachment 97492 [details] [diff] [review]
this patch will bubble up the proecessing and traversing the docShell tree

I had thought that the PreHandleEvent was  called on the ESM of "A" as well as
on the ESM of "2". see comment 11. 
But That is not true, and it seems menu accesskey handling is in a different
part. 
Anyway, This testcase 
http://bugzilla.mozilla.org/attachment.cgi?id=97491&action=view can exam
comment 28 and what i said in comment 9.
Attachment #96970 - Attachment is obsolete: true
(Assignee)

Comment 31

15 years ago
Comment on attachment 97492 [details] [diff] [review]
this patch will bubble up the proecessing and traversing the docShell tree


>+  // Alt or other accesskey modifier is down, we may need to do an accesskey
>+    // Someone registered an accesskey.  Find and activate it.
>+          // B) Click on it if the users prefs indicate to do so.

+  // after the local accesskey handling

+    // checking all sub docshells

+    nsCOMPtr<nsIDocShellTreeNode> docShell(do_QueryInterface(pcContainer));
+    NS_ASSERTION(docShell, "no docShellTreeNode for presContext");
Yes, I had change that.

>+      nsCOMPtr<nsIPresShell> subPS;

>You never use this anywhere...
No, It is used at 
+	 subDS->GetPresShell(getter_AddRefs(subPS));
+
+	 subPS->GetPresContext(getter_AddRefs(subPC));
+
+	 subPC->GetEventStateManager(getter_AddRefs(subESM));
Comment on attachment 97492 [details] [diff] [review]
this patch will bubble up the proecessing and traversing the docShell tree

sr=bzbarsky.  Looks good!
Attachment #97492 - Flags: superreview+
(Assignee)

Comment 33

15 years ago
Comment on attachment 97492 [details] [diff] [review]
this patch will bubble up the proecessing and traversing the docShell tree

check in
(Assignee)

Comment 34

15 years ago
patch had been checked in and mozilla works well with my own sandbox on windows
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Reporter)

Comment 35

15 years ago
*** Bug 64606 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 36

15 years ago
*** Bug 147692 has been marked as a duplicate of this bug. ***
This caused regression bug 167038
verification of this is blocked by bug 169767.
Summary: Preferences panel accelerator keys don't work when category tree or if OK/Cancel/Help buttons have focus → Preferences panel access keys don't work when category tree or if OK/Cancel/Help buttons have focus

Comment 39

15 years ago
Bug 169767 only blocks verification of this when the pref is set to text field
only, right?  At least, I can tab into prefs panels if I set my
accessibility.tabfocus to 7.
vrfy'd fixed with 2002.10.15.08 comm trunk builds on win2k and linux rh7.2
(access keys not supported on mac os).
Status: RESOLVED → VERIFIED
Hardware: All → PC
You need to log in before you can comment on or make changes to this bug.