573447 - FormHelper doesn't work with iframes in designmode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chwiej Pawel]

User-Agent:       Opera/9.80 (Windows NT 5.1; U; pl) Presto/2.5.24 Version/10.53
Build Identifier: Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.3a6pre) Gecko/20100621 Namoroka/3.7a6pre Fennec/2.0a1pre

Maemo5
Fennec trunk build Gecko/20100621


Reproducible: Always

Steps to Reproduce:
1. Open attached TC
2. Focus first input field
3. Tap 'Next' button from formhelper

Actual Results:  
Last input is focused, iframe in the middle is omitted


Expected Results:  
iframe in the middle should be focused


Note that such iframes are used in rich text editors, this affects usability of formhelper on gmail, mail.aol.com, mail.yahoo etc..

Comment 1

[Chwiej Pawel]

Attachment: TC

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

copied test page to http://people.mozilla.com/~nhirata/html_tp/bug573447.htm

confirmed:
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101116 Firefox/4.0b8pre Fennec/4.0b3pre

Comment 3

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

fix is in bug 609729

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

This was not fixed by bug 609729.

Form viewer does not appear for the iframe, VKB does appear.

Comment 5

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

Attachment: Patch

The patch add code to handle "designMode" and factorize the code added in the bug for editable frame to not spread code to check those cases everywhere

Comment 6

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

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

>diff --git a/chrome/content/Util.js b/chrome/content/Util.js

>+  /* Like dump but print the module name in red (on Linux) */
>+  log: function(aModule, aMessage) {
>+    dump("\033[0;31m" + aModule + ":: " + "\033[0;37m");
>+    dump(aMessage + "\n");

I don't know that we should have this in the code. Let's not add this.

>diff --git a/chrome/content/forms.js b/chrome/content/forms.js

>   handleEvent: function formHelperHandleEvent(aEvent) {

>         // If a body element is editable and the body is the child of an
>         // iframe we can assume this is an advanced HTML editor, so let's 
>         // redirect the form helper selection to the iframe element
>-        if (focusedElement) {
>+        if (focusedElement && this._isEditable(focusedElement)) {
>           let editableElement = this._getTopLevelEditable(focusedElement);
>-          if (editableElement.isContentEditable && this._isValidElement(editableElement)) {
>+          if (this._isValidElement(editableElement)) {
>             let self = this;
>+            this.focusSync = false;
>             let timer = new Util.Timeout(function() {
>               self.open(editableElement);
>+              this.focusSync = true;
>             });
>             timer.once(0);

Having the | this.focusSync = false; | outside the timer function worries me. Can we move the | self.focusSync = false; | _inside_ the timer function?

Also, you need to use | self.focusSync = true; | not | this.focusSync = true; | inside the timer function (notice "this" -> "self")

r- for the this -> self part, but also waiting for the answer to moving the focusSync part into the timer function.

Comment 7

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

Attachment: Patch v0.2 - addressed comments

In this patch:
 * Removal the Util.log function 
 * Removal of the [this/self].focusSync call

Comment 9

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/c2dd2c1c428b

Comment 10

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101210 Namoroka/4.0b8pre Fennec/4.0b3pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101210 Namoroka/4.0b8pre Fennec/4.0b3pre

Comment 11

[Dão Gottwald [:dao]]

bugspam