539533 - XUL select1 does not display items

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Philipp Wagner [:imphil]]

Attachment: testcase

UL select1 don't work in 1.9.2 any more (were fine in 1.9.1). No items are displayed, only empty entries.

Comment 1

[Philipp Wagner [:imphil]]

This is a regression caused by the fix for bug 531297. Reverting that patch restores the correct behavior.
Sergey, could you look possibly look into it?

Comment 2

[juan becerra [:juanb]]

I'm not sure how to use the test case to verify this bug on the RC2. Philipp, you could help us verify this bug?

Comment 3

[Philipp Wagner [:imphil]]

(In reply to comment #2)
> I'm not sure how to use the test case to verify this bug on the RC2. Philipp,
> you could help us verify this bug?

it's an XForms bug, not toolkit or Firefox. To verify, build the xforms extension with Gecko 1.9.2 and open the testcase.

Comment 4

[Alexey Gaidukov]

On my Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091031 MRA 5.5 (build 02842) Minefield/3.7a1pre (.NET CLR 3.5.30729) the testcase works without problems. All Sergey's patches are applied.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> On my Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre)
> Gecko/20091031 MRA 5.5 (build 02842) Minefield/3.7a1pre (.NET CLR 3.5.30729)
> the testcase works without problems. All Sergey's patches are applied.

What kind of patches? Have you applied any additional patches to the trunk code?

Comment 6

[Alexey Gaidukov]

I didn't apply patches from bug 537881 and I'm not sure that I have the latest version of patches for bug 525730.

I'm sure that the patch from bug 531297 is applied.

Comment 7

[Philipp Wagner [:imphil]]

You shouldn't need any additional patches to reproduce this, it happens (here) with plain hg trunk, without any additional patches.

Comment 8

[Sergey Reymerov]

I've look into this bug. Problem is in xforms-xul.xml:202
set value(val) {
  if (val != null) {
    this._implicitContent.hidden = false;
    this._explicitContent.hidden = true;
    this._implicitContent.textContent = val;
  } else {
    this._implicitContent.hidden = true;
    this._explicitContent.hidden = false;
    this._implicitContent.textContent = '';
  }
},

get textValue() {
  if (!this._implicitContent.hidden)             
    return this._implicitContent.textContent;
  return this.textContent;
},

At first constructor of xf:select1 calls get textValue() and then constructor of labels call set value(val). It must be back to front.

Comment 9

[Sergey Reymerov]

Attachment: patch

I use setTimeout to right order of creating elements.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 422301 [details] [diff] [review]
patch


>     <!-- private -->
>+      <!-- Timeout uses for right order of creating selects, items and labels. 
>+        Additional info in bug 539533.
>+      -->

move the comment into constructor, don't refer to bug number, put all necessary information right here instead.

>       <constructor>
>-        this.handleInsertion(this, null);
>+        obj = this;

var obj

>+        setTimeout(function() {obj.handleInsertion(obj, null);}, 0);

I'm not happy with setTimeout usage but I can't see better way

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 422301 [details] [diff] [review]
patch

forgot to set r=me

Comment 12

[Sergey Reymerov]

Attachment: patch 2

Move comment.

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 422323 [details] [diff] [review]
patch 2


>     <!-- private -->
>       <constructor>
>-        this.handleInsertion(this, null);
>+        <!-- Timeout uses for right order of creating selects, items and labels. 
>+          Additional info in bug 539533.
>+        -->      

use JS comment style, remove spaces from the end and remove a reference to the bug, it's better to put valuable comment instead

>+        obj = this;

var obj = this.

waiting for new patch

Comment 14

[Sergey Reymerov]

Attachment: patch 3

Comment 15

[alexander :surkov (:asurkov)]

That's better could you change the wording? Something like

We use timeout to fix the initialization order of selects and item labels bindings. Originally the select/select1 bindings are constructed before label bindings and this break us.

Comment 16

[Sergey Reymerov]

Attachment: patch 4

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 422475 [details] [diff] [review]
patch 4

The patch doesn't make the testcase working. The instance node value doesn't appear selected in select1. That's probably timing issue like the first one.

clearing r+ unit this is addressed.

Comment 18

[Sergey Reymerov]

Attachment: patch 5

Comment 19

[alexander :surkov (:asurkov)]

Try to use @deferredrefresh like it's done for xformswidget-range-numeric binding.

Comment 20

[Sergey Reymerov]

I've done it but it's not successfull.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> I've done it but it's not successfull.

put the patch please

Comment 22

[Sergey Reymerov]

Attachment: patch with deferredrefresh

Comment 23

[alexander :surkov (:asurkov)]

I'm not sure it makes sense to add deferredrefresh attribute for every binding we have. And it's not enough. This attribute prevents the call of widgetAttached method in constructor. So if you put this attribute on the element then you should call widgetAttached yourself when the widget is ready or in timeout like we do xformswidget-range-numeric.

Comment 24

[Sergey Reymerov]

Attachment: patch 6

It works fine.

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 425359 [details] [diff] [review]
patch 6


>diff -r 576e24224968 resources/content/selects.xml
>--- a/resources/content/selects.xml	Tue Jan 19 11:48:31 2010 +0800
>+++ b/resources/content/selects.xml	Fri Feb 05 09:14:00 2010 +0800

Please put a comment that any inherited binding with the content must have mozType:deferredrefresh="true" attribute.

>@@ -625,7 +625,14 @@
>       </method>
> 
>       <constructor>
>-        this.freeEntryAllowed = this.allowFreeEntry(this.selection == "open");
>+        // Call widgetAttached() and handleInsertion() after timeout to let items to be
>+        // loaded elsewhere select loads before items.      

remove spaces at the end of the comment

this binding hans't handleInsertion() method.

>     <!-- private -->
>       <constructor>
>-        this.handleInsertion(this, null);
>+        // Call constructor and widgetAttached() after timeout to let items to be
>+        // loaded elsewhere select loads before items.      
>+        this.ownerDocument.defaultView.setTimeout(
>+          function(aElement)
>+          {
>+            aElement.delegate.handleInsertion(aElement, null);
>+            aElement.delegate.widgetAttached();

it doesn't make sense to call widgetAttached() twice

Comment 26

[Sergey Reymerov]

Attachment: patch 7

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #26)
> Created an attachment (id=425424) [details]
> patch 7

please address all my comments.

Comment 28

[Sergey Reymerov]

Attachment: patch 8

I've removed selects.xml changes and its relatives because constructor in non-widget selects don't need timeout (constructor makes simple assignment statement). Also I wrote more detailed comment in header of selectsnw.xml.

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 425439 [details] [diff] [review]
patch 8

fine with me, thanks.

Comment 30

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/xforms/rev/8f5a9b43d98b