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

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

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".

Comment 1

[hirata masakazu]

030808 trunk build for MacOS9 seems WFM.

Comment 2

[Olga]

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.

Comment 3

[sairuh (rarely reading bugmail)]

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?

Comment 4

[sairuh (rarely reading bugmail)]

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.

Comment 5

[sairuh (rarely reading bugmail)]

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

Comment 6

[André Dahlqvist]

Changing summary to catch dupes.

Comment 7

[Aaron Leventhal]

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.

Comment 8

[Gilbert Fang]

okay, aaronl. that is my pleasure.

Comment 9

[Gilbert Fang]

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? 
 

Comment 10

[Gilbert Fang]

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. 

         

Comment 11

[Gilbert Fang]

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. 
 

Comment 12

[Gilbert Fang]

Attachment: checking all sub docshell's accesskey

Comment 13

[Aaron Leventhal]

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?

Comment 14

[Gilbert Fang]

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. 

Comment 15

[Gilbert Fang]

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

Comment 16

[Aaron Leventhal]

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

Comment 17

[Gilbert Fang]

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. 

Comment 18

[Gilbert Fang]

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

Comment 19

[Gilbert Fang]

hi, joki and saari
would u please review the patch?

Comment 20

[Gilbert Fang]

Attachment: new patch after my commnent 18

need r=?

Comment 21

[Gilbert Fang]

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

Comment 22

[Gilbert Fang]

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?
 

Comment 23

[Brian Ryner (not reading)]

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*.

Comment 24

[Gilbert Fang]

Attachment: factor out the HandleAccessKey from nsEventStateManager

need r=?

Comment 25

[Brian Ryner (not reading)]

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.

Comment 26

[Gilbert Fang]

Attachment: new patch after bryner's review

Comment 27

[Gilbert Fang]

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

Carrying bryner's review

Comment 28

[Boris Zbarsky [:bzbarsky]]

+  //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"?

Comment 29

[Gilbert Fang]

Attachment: a testcase for examing comments 28's scenario

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

Comment 30

[Gilbert Fang]

Attachment: 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.

Comment 31

[Gilbert Fang]

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 32

[Boris Zbarsky [:bzbarsky]]

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

sr=bzbarsky.  Looks good!

Comment 33

[Gilbert Fang]

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

check in

Comment 34

[Gilbert Fang]

patch had been checked in and mozilla works well with my own sandbox on windows

Comment 35

[Jesse Ruderman]

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

Comment 36

[Jesse Ruderman]

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

Comment 37

[Boris Zbarsky [:bzbarsky]]

This caused regression bug 167038

Comment 38

[sairuh (rarely reading bugmail)]

verification of this is blocked by bug 169767.

Comment 39

[Akkana Peck]

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.

Comment 40

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2002.10.15.08 comm trunk builds on win2k and linux rh7.2
(access keys not supported on mac os).