XUL select1 does not display items

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
8 years ago
a year ago

People

(Reporter: imphil, Assigned: Sergey Reymerov)

Tracking

({regression})

1.9.2 Branch
regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 421506 [details]
testcase

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

Comment 1

8 years ago
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?

Updated

8 years ago
Blocks: 539184

Updated

8 years ago
Blocks: 531297
Keywords: regression
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?
(Reporter)

Comment 3

8 years ago
(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

8 years ago
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

8 years ago
(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

8 years ago
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.
(Reporter)

Comment 7

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

Comment 8

8 years ago
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.
(Assignee)

Comment 9

8 years ago
Created attachment 422301 [details] [diff] [review]
patch

I use setTimeout to right order of creating elements.
Attachment #422301 - Flags: review?(surkov.alexander)

Comment 10

8 years ago
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
Attachment #422301 - Flags: review?(Olli.Pettay)

Updated

8 years ago
Assignee: nobody → sergeyreym
Status: NEW → ASSIGNED

Comment 11

8 years ago
Comment on attachment 422301 [details] [diff] [review]
patch

forgot to set r=me
Attachment #422301 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 12

8 years ago
Created attachment 422323 [details] [diff] [review]
patch 2

Move comment.
Attachment #422301 - Attachment is obsolete: true
Attachment #422323 - Flags: review?(surkov.alexander)
Attachment #422301 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

8 years ago
Attachment #422323 - Flags: review?(Olli.Pettay)

Comment 13

8 years ago
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
Attachment #422323 - Flags: review?(surkov.alexander)
(Assignee)

Comment 14

8 years ago
Created attachment 422336 [details] [diff] [review]
patch 3
Attachment #422323 - Attachment is obsolete: true
Attachment #422336 - Flags: review?(surkov.alexander)
Attachment #422323 - Flags: review?(Olli.Pettay)

Comment 15

8 years ago
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.
(Assignee)

Comment 16

8 years ago
Created attachment 422475 [details] [diff] [review]
patch 4
Attachment #422336 - Attachment is obsolete: true
Attachment #422475 - Flags: review?(surkov.alexander)
Attachment #422336 - Flags: review?(surkov.alexander)

Updated

8 years ago
Attachment #422475 - Flags: review?(surkov.alexander) → review+

Comment 17

8 years ago
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.
Attachment #422475 - Flags: review+

Updated

8 years ago
Attachment #422301 - Flags: review+
(Assignee)

Comment 18

8 years ago
Created attachment 422510 [details] [diff] [review]
patch 5
Attachment #422475 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #422510 - Flags: review?(surkov.alexander)
(Assignee)

Updated

8 years ago
Attachment #422510 - Flags: review?(Olli.Pettay)

Comment 19

8 years ago
Try to use @deferredrefresh like it's done for xformswidget-range-numeric binding.
(Assignee)

Comment 20

8 years ago
I've done it but it's not successfull.

Comment 21

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

put the patch please
(Assignee)

Comment 22

8 years ago
Created attachment 425194 [details] [diff] [review]
patch with deferredrefresh

Comment 23

8 years ago
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.
(Assignee)

Comment 24

8 years ago
Created attachment 425359 [details] [diff] [review]
patch 6

It works fine.
Attachment #422510 - Attachment is obsolete: true
Attachment #425194 - Attachment is obsolete: true
Attachment #425359 - Flags: review?(surkov.alexander)
Attachment #422510 - Flags: review?(surkov.alexander)
Attachment #422510 - Flags: review?(Olli.Pettay)

Comment 25

8 years ago
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
Attachment #425359 - Flags: review?(surkov.alexander)
(Assignee)

Comment 26

8 years ago
Created attachment 425424 [details] [diff] [review]
patch 7
Attachment #425359 - Attachment is obsolete: true
Attachment #425424 - Flags: review?(surkov.alexander)

Comment 27

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

please address all my comments.
(Assignee)

Comment 28

8 years ago
Created attachment 425439 [details] [diff] [review]
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.
Attachment #425424 - Attachment is obsolete: true
Attachment #425439 - Flags: review?(surkov.alexander)
Attachment #425424 - Flags: review?(surkov.alexander)
(Assignee)

Updated

8 years ago
Attachment #425439 - Flags: review?(Olli.Pettay)

Comment 29

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

fine with me, thanks.
Attachment #425439 - Flags: review?(surkov.alexander) → review+
Attachment #425439 - Flags: review?(Olli.Pettay) → review+

Comment 30

8 years ago
http://hg.mozilla.org/xforms/rev/8f5a9b43d98b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
OS: Linux → All
Hardware: x86 → All
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.