Closed Bug 187301 Opened 22 years ago Closed 22 years ago

No pref observer for accessibility.tabfocus pref

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

(Depends on 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

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
Taking.
Assignee: akkana → aaronl
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.
Attachment #110548 - Flags: superreview?(bryner)
Attachment #110548 - Flags: review?(akkana)
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+
checked in
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Argh, this fix caused a large spike in Txul (window opoen time).

Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
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.
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..
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
Attachment #111230 - Flags: superreview?(bryner)
Attachment #111230 - Flags: review?(akkana)
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!
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 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+
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
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
rs vrfy
Status: RESOLVED → VERIFIED
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: