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

VERIFIED FIXED in mozilla1.0

Status

()

Core
Keyboard: Navigation
P1
critical
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Jesse Ruderman, Assigned: Aaron Leventhal)

Tracking

({embed})

Trunk
mozilla1.0
x86
Windows 98
embed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

16 years ago
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]
(Assignee)

Comment 1

16 years ago
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

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

Comment 3

16 years ago
Ok, now I know. Accepting.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 4

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

Comment 5

16 years ago
Created attachment 77518 [details] [diff] [review]
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

16 years ago
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.
(Assignee)

Comment 7

16 years ago
Created attachment 77535 [details] [diff] [review]
Better patch -uses multiple try/catch's
Attachment #77518 - Attachment is obsolete: true
(Reporter)

Comment 8

16 years ago
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

16 years ago
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
Attachment #77535 - Flags: review+
(Assignee)

Comment 10

16 years ago
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.

Updated

16 years ago
Attachment #77535 - Flags: superreview+

Comment 11

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

yeah, default that to false

sr=hewitt
(Assignee)

Updated

16 years ago
Keywords: nsbeta1

Updated

16 years ago
Keywords: nsbeta1 → embed, nsbeta1+
Whiteboard: [adt2]
(Assignee)

Updated

16 years ago
Keywords: adt1.0.0

Comment 12

16 years ago
adt1.0.0+ (on ADT's behalf) for approval for checkin.
Keywords: adt1.0.0 → adt1.0.0+

Comment 13

16 years ago
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
Attachment #77535 - Flags: approval+
(Assignee)

Comment 14

16 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
i don't see a js error when using F7. vrfy'd using 2002040510 linux comm bits.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.