563559 - uncaught exception: Empty rectangles do not have centers when going Next/Previous to display: none form elements with form helper

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

Make sure you have the javascript.options.showInConsole pref set to true and the browser.console.showInPanel set to true in about:config.

To reproduce, in the testcase, focus the first multiple select. Both selects will disappear on focus (still, the form helper comes up (this might be a bug in itself)).
Now, just click on the Next and Previous buttons a few times.

You will get these errors in the error console:
uncaught exception: Empty rectangles do not have centers

The form helper could probably be a bit smarter of when to show up (like don't show up when a form element disappears on focusing). It would defeat the testcase, that I attached, but there are probably other cases where this javascript error will occur.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

FormHelper._getRectForElement is returning an empty rectangle since the element has been hidden:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#1807

Do we want to rescan for valid elements everytime we jump next or prev? Could be a speed regression. We could just do a check for visible and remove (or skip) the element. I suggest skipping because there might be other conditions where the element is not hidden. A radio button selection could change visible widgets for example.

Comment 2

[Martijn Wargers (dead)]

It would already help, if the form helper first switches focus, before updating the form elements list, see bug 562223.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This patch prevent the error to show and update the button but did not change the actual behavior of fennec when dealing with a field that hide itself when clicking on it (which I don't think is a common behavior on the web).

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 446497 [details] [diff] [review]
Patch

I think this is a good fix for dynamic widgets.

The current testcase hides a widget if it is focused, but I don't think that is realistic. I think hiding other widgets based on a change to the current widget is more realistic.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/c970dd3cc3ee

pushed to m-1.1:
http://hg.mozilla.org/releases/mobile-1.1/rev/1a1d03cfc678

Comment 6

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.5pre) Gecko/20100521 Namoroka/3.6.5pre Fennec/1.1b2pre

and

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:1.9.3a5pre) Gecko/20100521 Namoroka/3.7a5pre Fennec/2.0a1pre