No pref observer for accessibility.tabfocus pref

VERIFIED FIXED

Status

()

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

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

(Depends on: 1 bug, {access})

Trunk
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

15 years ago
While working to expose tab navigation prefs in bug 169489, I noticed that the
pref can't be changed dynamically. You have to restart the browser, which is
inconsistent with our general prefs behavior.

The code is in nsEventStateManager.cpp

      // Tab focus mode is constant across all windows.
      // It would be nicer if ESM had a prefs callback,
      // so we could store this and change behavior when it changes.
      // But until the pref is exposed, that doesn't matter.
Make sure you do this as a pref observer, not a callback (callbacks are thru
nsIPref, which is deprecated, observers are the cool way to do it).

nsEventStateManager already implements nsIObserver, so all you need to do is
register the observer (you need to QI mPrefBranch to nsIPrefBranchInternal
first), and then handle NS_PREFBRANCH_PREFCHANGE_TOPIC_ID in Observe().
Summary: No prefs callback for accessibility.tabfocus pref → No pref observer for accessibility.tabfocus pref
(Assignee)

Comment 2

15 years ago
Taking.
Assignee: akkana → aaronl
(Assignee)

Comment 3

15 years ago
Created attachment 110548 [details] [diff] [review]
Make pref dynamically changeable, make textfields always tabbable

Some notes that I included in the code:

  // XXX todo: On the mac, only links tabbing should be configurable in Mozilla

  //	       Whether other form controls are tabbable is a system setting
  //	       that we should adhere to via look and feel.
  // Something like this:
  // sTabFocusModel &= eTabFocus_linksMask;
  // nsCOMPtr<nsILookAndFeel> lookNFeel(do_GetService(kLookAndFeelCID));
  // PRInt32 tabToNonTextControls = 0;
  // lookNFeel->GetMetric(nsILookAndFeel::eMetric_TabToNonTextControls,
  //			  tabToNonTextControls);
  // if (tabToNonTextControls)
  //   sTabFocusModel |= eTabFocus_formElementsMask;
  // else
  //   sTabFocusModel = eTabFocus_textControlsMask;  // Only tab to text fields

  // This assumes that when the setting says "only text controls"
  // that links shouldn't be tabbable.
  // Also, we currently ignore the pref for XUL dialogs
  // but we should pay attention to the mac setting there

We should file a separate bug on adhering to the mac system setting.
(Assignee)

Updated

15 years ago
Attachment #110548 - Flags: superreview?(bryner)
Attachment #110548 - Flags: review?(akkana)

Comment 4

15 years ago
Comment on attachment 110548 [details] [diff] [review]
Make pref dynamically changeable, make textfields always tabbable

Thanks -- using the pref observer is definitely better.

If we're not going to be using eTabFocus_textControlsMask. let's just remove it
entirely rather than keeping it in with a comment.

Regarding mac and look-n-feel: maybe we should move all these prefs to
look-n-feel.  It wouldn't be surprising if other platforms added global
settings for this, and it's much better to be consistent between platforms. 

Is there any possibility of performance problems, if we're calling this once
per esm?  If we only have one window at startup, it should be the same number
of calls, so no performance hit, right? 

r=akkana but I'd prefer that you remove textControlsMask entirely.
Attachment #110548 - Flags: review?(akkana) → review+
Comment on attachment 110548 [details] [diff] [review]
Make pref dynamically changeable, make textfields always tabbable

>Index: nsEventStateManager.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/events/src/nsEventStateManager.h,v
>retrieving revision 1.89
>diff -u -r1.89 nsEventStateManager.h
>--- nsEventStateManager.h	24 Dec 2002 20:21:40 -0000	1.89
>+++ nsEventStateManager.h	3 Jan 2003 00:25:41 -0000
>@@ -74,10 +74,11 @@
>   // Tab focus model bit field:
>   enum nsTabFocusModel {
>     eTabFocus_unset = 0,                  // unset, check preferences
>-    eTabFocus_textControlsMask = (1<<0),  // text elements
>+    eTabFocus_textControlsMask = (1<<0),  // text elements, always focusable
>     eTabFocus_formElementsMask = (1<<1),  // non-text form elements
>-    eTabFocus_linksMask = (1<<2)          // links
>-  };
>+    eTabFocus_linksMask = (1<<2),         // links
>+    eTabFocus_any = 1 + (1<<1) + (1<<2),  // everything that can be focused
>+    };

I think a logical OR would be easier to follow than addition of the masks.

>@@ -3440,7 +3472,7 @@
>         }
>         else if (nsHTMLAtoms::textarea==tag) {
>           // it's a textarea
>-          disabled = !(sTabFocusModel & eTabFocus_textControlsMask);
>+          disabled = PR_FALSE;
>           if (!disabled) {
>             nsCOMPtr<nsIDOMHTMLTextAreaElement> nextTextArea(do_QueryInterface(child));
>             if (nextTextArea) {

You can remove the |if (!disabled)| since you're always setting it to false.
sr=bryner with those changes.
Attachment #110548 - Flags: superreview?(bryner) → superreview+
(Assignee)

Comment 6

15 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

15 years ago
Argh, this fix caused a large spike in Txul (window opoen time).

Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

15 years ago
This stinks, I have only 1 static global that I want to update in
nsEventStateManager, based on a prefcallback. So do I have to create a global
observer object too... a single one?

I don't want to have it callback for every esm when the pref changes, and if I
attach it to only 1 of the esm's, it could get destroyed and leave me without a
pref callback.

Bnesse, thoughts?

Comment 9

15 years ago
generally the idea is to try to reuse an existing nsIObserver implementation -
is there an existing service that's related to events that could update the
global variable? The other option is just to cache the nsIPrefBranch and just
call GetIntPref every time - its a relatively cheap call, but I don't know how
often you need to call it.
You might want to define a special observer class.  Call it TabFocusPrefObserver
or something.  have it only inherit nsIObserver.  From within nsESM::Init(),
attempt to create it only when mInstanceCount is 1.  Just add the observer, as a
strong observer and you don't need to worry about memory managing it, it will
destroy itself on shutdown (the prefs code handles this).
Alecf, nsEventStateManager is an existing nsIObserver impl, and it does cache
mPrefBranch.  So yeah, the other option is to just get the pref each time, but
if the pref rarely changes and we need to know this information quite
frequently, creating a specialized observer for it might be better.

Comment 12

15 years ago
sure - just do everything you can to avoid creating a special nsIObserver
implementation. Remember that classes with virtual methods have vtables, which
means they take up at least 4 bytes per instance, and they occupy a slot on the
heap which then usually has another 8 or more bytes of overhead... not to
mention the static code footprint of the vtable, the Observe() implementation,
and so forth. 

For this reason, I honestly think that static C callbacks may be better than
nsIObserver - brian and I had talked about bringing them over and hiding them in
nsIPrefBranchInternal or something..
(Assignee)

Comment 13

15 years ago
Created attachment 111230 [details] [diff] [review]
Uses GetIntPref() whenever it needs the pref, via alecf's suggestion. Only uses mPrefBranch->GetIntPref() once per tab, and mPrefBranch is cached.

Since this only happens once per tab, I like alecf's suggestion of caching pref
branch, and using GetIntPref().
I think this is better than writing a bunch of code and creating a singleton
object. I think that would be overcomplicating things for nothing.
Attachment #110548 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #111230 - Flags: superreview?(bryner)
Attachment #111230 - Flags: review?(akkana)
(Assignee)

Comment 14

15 years ago
Alecf and caillon, should we be wondering why there was such a huge spike in
txul, when all I did was add a pref observer once per window? I think that
adding the pref observer must be an extremely slow operation, or am I missing
something?

On Windows, it went from 344->359
That's reasonable

On Linux, 477->736 and 1173->1488
That's crazy!

Comment 15

15 years ago
I like this approach as well.. 
I agree, something is very messed up if that changed pageload times that much.
(though even 344->359 is not reasonable - that's over 4%! 
If that really turns out to be the only culprit, I'll investigate - but lets
wait until you've landed this updated version of the patch. Maybe the place you
added the AddObserver() is being called way more than we thought (in which case,
maybe we have another performance win waiting for us?)

Comment 16

15 years ago
Comment on attachment 111230 [details] [diff] [review]
Uses GetIntPref() whenever it needs the pref, via alecf's suggestion. Only uses mPrefBranch->GetIntPref() once per tab, and mPrefBranch is cached.

I'm a little uncomfortable with this -- are prefs really fast enough that it's
okay to call GetIntPref for every tab?

I'm marking it r=akkana, and the code looks fine, so it's just the performance
concern (and at least it happens at tab time, not at startup) ... if bryner
and/or alecf think this is the right solution, it's okay with me.
Attachment #111230 - Flags: review?(akkana) → review+

Comment 17

15 years ago
prefs are more or less as fast as you can get, once you've got the prefbranch.
The construction of the pref call is mostly:
- vtable call into prefs
- hashing of the pref name (standard, fast hash algorithm)
- lookup in a table
- copying of the integer value to the out parameter in GetIntPref()

that's it. The only reason to use cache values and use callbacks is if you're in
a performance-sensitive environment, such as being in a loop. Otherwise, its
overkill - it results in much more static code (setup/teardown of the observer)
and heap use (keeping one more observer in the observer list, possibly creating
a specialized observer, etc)
Attachment #111230 - Flags: superreview?(bryner) → superreview+
the Txul numbers on the win32 and linux tboxen look fine after this recent
checkin. marking resolved. (reopen if there are other issues pertaining to this,
though.)
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
rs vrfy
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.