622577 - Most content is not rendered after these steps on bed4less.nl

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

Tested, using:
Mozilla/5.0 (Maemo; Linux armv7l; rv:2.0b9pre) Gecko/20110102 Firefox/4.0b9pre
Fennec/4.0b4pre ID:20110102010340

Steps to reproduce:
- Go to http://www.bed4less.nl/boxsprings.html
- Go to the select at the bottom of the page between "Toon" and "per pagina".
- Tap on that select and choose a different option than the current selected one.
- After tapping, the page will reload.

Expected result:
- The reloaded page should be shown

Actual reslut:
- Almost no content is shown of the reloaded page until you tap on the page, then the content is suddenly shown.

Comment 1

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

Attachment: Patch

A large patch of the patch include the code from bug 621637, because this bug has revealed me that if we disabled the formhelper pref, the Select Helper does not disappear sometimes and we need a better focus handling for resolve this case.

This bugs is because of a bad handling of a Resize message we ignore now if the FormHelperUI is already dismisseD.

Comment 2

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

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

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>   receiveMessage: function formHelperReceiveMessage(aMessage) {
>+    if (!this._open && aMessage.name != "FormAssist:Show" && aMessage.name != "FormAssist:Hide")
>+      return;

Doesn't this mean we'll only handle FormAssist:Show and FormAssist:Hide messages?

>       case "FormAssist:Show":

>         this.enabled ? this.show(json.current, json.hasPrevious, json.hasNext)
>                      : SelectHelperUI.show(json.current.choices);
>         break;
> 
>+      case "FormAssist:Hide":
>+        this.enabled ? this.hide() :
>+                       SelectHelperUI.hide();

One line for both of these "? :" statements? Or keep the ":" on the same line to be consistent

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

>   addEventListener("keyup", this, false);
>   addEventListener("focus", this, true);
>-  addEventListener("DOMWindowCreated", this, false);
>   addEventListener("pageshow", this, false);
>+  addEventListener("pagehide", this, true);

pageshow uses | false |, pagehide uses | true | - is this on purpose?

r- for now

Comment 3

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

(In reply to comment #2)
> Comment on attachment 500870 [details] [diff] [review]
> Patch
> 
> >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
> 
> >   receiveMessage: function formHelperReceiveMessage(aMessage) {
> >+    if (!this._open && aMessage.name != "FormAssist:Show" && aMessage.name != "FormAssist:Hide")
> >+      return;
> 
> Doesn't this mean we'll only handle FormAssist:Show and FormAssist:Hide
> messages?

No, it means we handle all events when the FormHelper is opened and only Show/Hide when it is closed.

> >diff --git a/chrome/content/forms.js b/chrome/content/forms.js
> 
> >   addEventListener("keyup", this, false);
> >   addEventListener("focus", this, true);
> >-  addEventListener("DOMWindowCreated", this, false);
> >   addEventListener("pageshow", this, false);
> >+  addEventListener("pagehide", this, true);
> 
> pageshow uses | false |, pagehide uses | true | - is this on purpose?

Not on purpose.

I've updated the patch and added a few tests for some focus changes.

Comment 4

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

Attachment: Patch v0.2

Comment 5

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

Attachment: Patch v0.2

Sorry, I've attached the wrong patch.

Comment 6

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

Comment on attachment 501001 [details] [diff] [review]
Patch v0.2

Can you rename _executeSoon -> _executeDelayed, since we already have an executeSoon and it dopes not use timers.

Comment 7

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

(In reply to comment #6)
> Comment on attachment 501001 [details] [diff] [review]
> Patch v0.2
> 
> Can you rename _executeSoon -> _executeDelayed, since we already have an
> executeSoon and it dopes not use timers.

Done

http://hg.mozilla.org/mobile-browser/rev/7fa54c36121f

Comment 8

[Aakash Desai [:aakashd]]

verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20100104 Namoroka/4.0b9pre Fennec/4.0b4pre

Comment 9

[Dão Gottwald [::dao]]

bugspam