135196 - F7 to toggle browse-with-caret hits a JS error

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

When I press F7 in 2002 040303, nothing happens in the browser window, and the
following appears on the JS console:

Error: uncaught exception: [Exception... "Component returned failure code:
0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult:
"0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: <unknown filename> ::
onxblkeypress :: line 4"  data: no]

Comment 1

[Aaron Leventhal]

Strange. Can you look in your all.js file and see if it has the lines:
pref("accessibility.browsewithcaret", false);
pref("accessibility.warn_on_browsewithcaret", true);

Is this a downloaded binary build or compiled from source?

Comment 2

[Boris Zbarsky [:bzbarsky]]

umm... no matter what the default all.js is, distributors can configure it. 
Code should always wrap get*Pref calls in try/catch

Comment 3

[Aaron Leventhal]

Ok, now I know. Accepting.

Comment 4

[Aaron Leventhal]

Interesting, I think we don't use try/catch in a lot of places where we get
prefs, for example tabbrowser.xml.

Comment 5

[Aaron Leventhal]

Attachment: Wraps F7 handler in try/catch.

Still not sure why Jesse was getting exceptions. Curious whether his all.js
file has the lines I mentioned.

Comment 6

[Boris Zbarsky [:bzbarsky]]

doesn't that just silently ignore F7 and not go into browse-with-caret mode
(since all the code is in thee try)?

I'd think that a better approach would be:

var foo = true;
try {
  foo = mPrefs.getBoolPref("accessibility.warn_on_browsewithcaret");
} catch (ex) {}
if (foo && !browseWithCaret) {
  // go on 
}

And similarly for other pref accesses.

Comment 7

[Aaron Leventhal]

Attachment: Better patch -uses multiple try/catch's

Comment 8

[Jesse Ruderman]

The 2002 040303 I tested is a nightly talkbalk zip build for Windows.  I 
installed it in a new directory, like I usually do when I get a new build.

I checked all.js and only saw
pref("accessibility.browsewithcaret", false);

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 77535 [details] [diff] [review]
Better patch -uses multiple try/catch's

Wouldn't it be better to have browseWithCaretOn default to "false"?  Do that
and r=bzbarsky

Comment 10

[Aaron Leventhal]

Done.

The reason the exception happened in the first place, is that I forgot to check
in my all.js changes! Duh.

Anyway, the try/catch is an improvement as well.

Comment 11

[Joe Hewitt (gone)]

Comment on attachment 77535 [details] [diff] [review]
Better patch -uses multiple try/catch's

yeah, default that to false

sr=hewitt

Comment 12

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for approval for checkin.

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 77535 [details] [diff] [review]
Better patch -uses multiple try/catch's

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 14

[Aaron Leventhal]

checked in

Comment 15

[sairuh (rarely reading bugmail)]

i don't see a js error when using F7. vrfy'd using 2002040510 linux comm bits.