148010 - JS exceptions in mailnews autocomplete widget on trunk

Status: RESOLVED FIXED

(SeaMonkey :: Autocomplete, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

This seems to have been caused by the fix to bug 133365; backing out that patch
causes the exceptions to go away.  Seth has suggested adding
"disableKeyNavigation" to the addressingWidgetOverlay, which sounds reasonable.
 But is this actually something that should be disabled on _all_ widgets of type
autocomplete -- ie should we instead make an analogous fix in autocomplete.xml?

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

FWIW, the exceptions are happening around line 684 in listbox.xml.  I added a
dump statement and it shows the following:

i= 0; start= 1; c= -1
i= 1; start= 1; c= -1
i= 2; start= 1; c= -1
i= 3; start= 1; c= -1
i= 0; start= 0; c= -1

before throwing the exception.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

JavaScript error: 
 line 0: uncaught exception: [Exception... "Component returned failure code:
0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIListBoxObject.getItemAtIndex]" 
nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
chrome://global/content/bindings/listbox.xml#listbox.getItemAtIndex() ::
getItemAtIndex :: line 1"  data: no]

is the exception in question.  I see multiple of these just by typing in the
addressing widget, both linux and windows.

Comment 3

[Kyle Yuan]

can reproduce, taking...

Comment 4

[Kyle Yuan]

Attachment: prevent JS exception and disable key-navigation feature in autocomplete listbox

1) Check (i + start + c) >= 0 is always true to prevent JS exception
2) Prevent navigation when <ctrl>/<alt>/<shift>/<meta> was pressed
3) Disable key-navigation feature in autocomplete listbox. There are about 30
xul files that use <listbox>, I think only the autocomplete widget need to
disable this feature.
4) Fix the same potential error (JS exception) in tree.xml

Comment 5

[Kyle Yuan]

call Jan to review my patch

Comment 6

[:deanis74]

Alernatively, could you not check that k > 0 after

    var k = (i + start + c) % rowCount;

?

As for the question in comment 0, I think we should disable this for all
auto-complete widgets and not this specific one.  I can't think of why you'd
want both auto-complete and keyboard navigation enabled.

Comment 7

[Kyle Yuan]

I can guarantee (i + start + c) >= 0 before entering the loop.
Checking (k > 0) in loop will consume more time.

Yes, we should disable this for all the autocomplete widgets.
Dean, can you figure them out?

Comment 8

[:deanis74]

Yeah, I realized that as soon as I had pressed commit.

Can I figure out autocompletes?  No.  But maybe you can set the attribute in the
constructor.  Just a stab in the dark.

http://lxr.mozilla.org/seamonkey/source/xpfe/components/autocomplete/resources/content/autocomplete.xml#41

Comment 9

[Kyle Yuan]

Yeah! Pretty good idea. I'll do that soon.

Comment 10

[Kyle Yuan]

Attachment: revised (follow Dean's suggestion)

turned on disableKeyNavigation in autocomplete.xml rather than in
addressingWidgetOverlay.xul.

Comment 11

[Jan Varga [:janv]]

Comment on attachment 85549 [details] [diff] [review]
revised (follow Dean's suggestion)

r=varga

Comment 12

[Joe Hewitt (gone)]

Comment on attachment 85549 [details] [diff] [review]
revised (follow Dean's suggestion)

sr=hewitt

Comment 13

[Kyle Yuan]

checked in!