FormFill: Taskbar pops up to middle of the screen with no-associated protocol error

VERIFIED FIXED

Status

VERIFIED FIXED
9 years ago
5 years ago

People

(Reporter: aakashd, Assigned: vingtetun)

Tracking

Fennec 1.1
Bug Flags:
in-testsuite ?

Details

(Whiteboard: formfill)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Build Id:
Mozilla/5.0 (Windows; U; Window3sCE 5.2; en-US; rv:1.9.2b5pre) Gecko/20091203 Namoroka/3.6b5pre Fennec/1.0a4pre

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091203 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091203 Firefox/3.7a1pre Fennec/1.0b5

Steps to Reproduce:
1. Go to http://feeds.feedburner.com/SlickdealsnetFP
2. Click on "show all Subscribe options
3. Click on the selection drop down to open up Fennec's selection box UI
4. Selection an option and click "Done"

Actual Results:
The formfill taskbar pops up to the middle of the screen behind the "Firefox doesn't know how to open this address because the protocol (feed) isn't associated with any program" error.

Screenshot: http://www.flickr.com/photos/42893104@N04/4155924696/

Expected Results:
The taskbar should not pop up.
More generally this *could* happened is a onchange handler open an alert.
I've started a wip for it.
Assignee: nobody → 21
Created attachment 415977 [details] [diff] [review]
Patch

This patch dismiss the UI once the alert has been dismissed
Attachment #415977 - Flags: review?(mark.finkle)
Comment on attachment 415977 [details] [diff] [review]
Patch

>diff -r 1d1b9c70d423 chrome/content/browser.js

>-  // we need to insert before select-container if we want it to show correctly
>-  let selectContainer = document.getElementById("select-container");
>-  let parent = selectContainer.parentNode;
>+  // we need to insert before alerts-container if we want it to show correctly
>+  //XXXvn why?
>+  let alertsContainer = document.getElementById("alerts-container");
>+  let parent = alertsContainer.parentNode;

Does this break the select prompt?
(In reply to comment #3)
> (From update of attachment 415977 [details] [diff] [review])
> >diff -r 1d1b9c70d423 chrome/content/browser.js
> 
> >-  // we need to insert before select-container if we want it to show correctly
> >-  let selectContainer = document.getElementById("select-container");
> >-  let parent = selectContainer.parentNode;
> >+  // we need to insert before alerts-container if we want it to show correctly
> >+  //XXXvn why?
> >+  let alertsContainer = document.getElementById("alerts-container");
> >+  let parent = alertsContainer.parentNode;
> 
> Does this break the select prompt?

Instead of being in top of it, it was inside it and pushed the select prompt offscreen or to the bottom of the screen.  I think the initial intention was to insert the prompt somewhere in the stack, not "in" a box where select-container can live.
Comment on attachment 415977 [details] [diff] [review]
Patch

Fabrice, I want to add your opinion on the change in the importDialog method of browser.js.
Attachment #415977 - Flags: review?(fabrice.desre)

Comment 6

9 years ago
The initial intent was to be sure that the selectHelper would show on top of a dialog, to allow the use of <menulist/> in dialogs. The prompt service needs this for prompt.select() for instance.
Comment on attachment 415977 [details] [diff] [review]
Patch

Thinking about that i'm not sure of what to do.
For example it can co-exist two select in the same times if during an onchange event someone open a dialog with a select list.
Attachment #415977 - Flags: review?(mark.finkle)
Attachment #415977 - Flags: review?(fabrice.desre)
Do we have any new ideas for this?
(In reply to comment #8)
> Do we have any new ideas for this?

I will retry something tomorrow and cross my fingers hoping there is an easy solution I've missed
Created attachment 467390 [details] [diff] [review]
Patch

This patch hides the form assistant when a dialog is opened which resolve this bug, and to handle the prompt.select() case I've change the look of the menulist and introduce a new helper which resolve this case and the case where the form assistant is opened in the content but someone want to open a menulist in the preferences panel (which was impossible before)
The patch does not alter the support for menulist in content which continue to work the old way.

The patch also includes:
 * a fix for form.helper.enabled when it comes to handle list from webpage
 * a new style for the preferences menulist (Matt and Madhava has discussed this once on IRC iirc)
Attachment #415977 - Attachment is obsolete: true
Attachment #467390 - Flags: review?(mark.finkle)
Adding Matt and Madhava because of the style change for the menulist in preferences
Comment on attachment 467390 [details] [diff] [review]
Patch

Just started looking at this, but I saw this:

+
+    // Listen for modal dialog to show/hide the UI
+    window.addEventListener("DOMWillOpenModalDialog", this, true);
+    window.addEventListener("DOMModalDialogClosed", this, true);


Shouldn't we be listening for messages, not events?
(In reply to comment #12)
> Comment on attachment 467390 [details] [diff] [review]
> Patch
> 
> Just started looking at this, but I saw this:
> 
> +
> +    // Listen for modal dialog to show/hide the UI
> +    window.addEventListener("DOMWillOpenModalDialog", this, true);
> +    window.addEventListener("DOMModalDialogClosed", this, true);
> 
> 
> Shouldn't we be listening for messages, not events?

That's a good question. I wonder why it works this way. I will look closer.
Created attachment 468279 [details] [diff] [review]
Patch v0.2

Finally my previous response could be: because it _was_ not working!
Attachment #467390 - Attachment is obsolete: true
Attachment #468279 - Flags: review?(mark.finkle)
Attachment #467390 - Flags: review?(mark.finkle)
Attachment #467390 - Flags: review?(fabrice.desre)
Comment on attachment 468279 [details] [diff] [review]
Patch v0.2

>-        if (enabled) {
>-          this.show(json.current, json.hasPrevious, json.hasNext);
>-        }
>-        else {
>-          SelectHelperUI.show(json.current.list);
>-        }
>+        enabled ? this.show(json.current, json.hasPrevious, json.hasNext)
>+                : SelectHelperUI.show(json.current.choices);

old code used "json.current.list"
new code uses "json.current.choices"

Is this intended?


>+var MenuListHelperUI = {

Let's think about any code duplication. At some point we might want a base class.


>diff -r 2a87eb6f6cd3 chrome/content/browser.js

>+  // we need to insert before menulist-container if we want it to show correctly
>+  // for prompt.select for instancee

"instancee" ? should that be "instance"?


> FormAssistant.prototype = {
>+  _selectWrapper: null,

Do we need to hold this as a data member?

>diff -r 2a87eb6f6cd3 themes/core/browser.css

> /* context popup ----------------------------------------------------------- */

Update the comment

The CSS is another place to watch for duplication

r+ with nits addressed
Attachment #468279 - Flags: review?(mark.finkle) → review+
Whiteboard: formfill → formfill [fennec-checkin-posta1]
http://hg.mozilla.org/mobile-browser/rev/5d4dcba3841d

Filed bug 590071 for the deduplication but I have to admit that I'm more concerned about the css (which evolved quickly) that the js where I don't think we will add a new helper for an input element before a long time.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Created attachment 468691 [details] [diff] [review]
browser_preferences_text.js bustage fix

My patch from this morning has cause a failure into browser_preferences_text.js. This patch fix that.
Attachment #468691 - Flags: review?(mark.finkle)
Whiteboard: formfill [fennec-checkin-posta1] → formfill
Attachment #468691 - Flags: review?(mark.finkle) → review+
Duplicate of this bug: 571999
(Reporter)

Comment 20

8 years ago
verified FIXED On builds:
Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b5pre) Gecko/20100825 Namoroka/4.0b5pre Fennec/2.0a1pre

and

Mozilla/5.0 (Android; Linux armv71; Nokia N900; en-US; rv:2.0b5pre) Gecko/20100825 Namoroka/4.0b5pre Fennec/2.0a1pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.