Closed Bug 622577 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
All
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: vingtetun)

References

()

Details

Attachments

(1 file, 2 obsolete files)

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.
tracking-fennec: --- → ?
OS: Windows 7 → All
Hardware: x86 → ARM
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #500870 - Flags: review?(mark.finkle)
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
Attachment #500870 - Flags: review?(mark.finkle) → review-
(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.
Attached patch Patch v0.2 (obsolete) — Splinter Review
Attachment #500870 - Attachment is obsolete: true
Attachment #500978 - Flags: review?(mark.finkle)
Attached patch Patch v0.2Splinter Review
Sorry, I've attached the wrong patch.
Attachment #500978 - Attachment is obsolete: true
Attachment #501001 - Flags: review?(mark.finkle)
Attachment #500978 - Flags: review?(mark.finkle)
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.
Attachment #501001 - Flags: review?(mark.finkle) → review+
(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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20100104 Namoroka/4.0b9pre Fennec/4.0b4pre
Status: RESOLVED → VERIFIED
bugspam
Assignee: nobody → 21
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.