Closed Bug 408940 Opened 12 years ago Closed 12 years ago

Setting the value property of a menulist to null should remove the old value

Categories

(Toolkit :: XUL Widgets, defect)

Product:
Toolkit
Component:
XUL Widgets
Type:
defect
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(2 files, 2 obsolete files)

patch v2 (same with a Mochitest added)
4.31 KB, patch
mconnor
: review+
mconnor
: approval1.9+
Details | Diff | Splinter Review
patch v4
5.12 KB, patch
mano
: review+
mtschrep
: approval1.9+
Details | Diff | Splinter Review
Attached patch patch v1 (obsolete) — — Splinter Review 
No description provided.
Attachment #293797 - Flags: review?(mano)
Assignee: nobody → florian
Whiteboard: [has patch, needs review mano]
Comment on attachment 293797 [details] [diff] [review]
patch v1

r=mano, needs a test.
Attachment #293797 - Flags: review?(mano) → review+
Attachment #293797 - Attachment is obsolete: true
Attachment #296262 - Flags: review?(mano)
Comment on attachment 296262 [details] [diff] [review]
patch v2 (same with a Mochitest added)

r=me on the test, a=me for landing
Attachment #296262 - Flags: review?(mano)
Attachment #296262 - Flags: review+
Attachment #296262 - Flags: approval1.9+
Checking in toolkit/content/widgets/menulist.xml;
/cvsroot/mozilla/toolkit/content/widgets/menulist.xml,v  <--  menulist.xml
new revision: 1.35; previous revision: 1.34
done
Checking in toolkit/content/tests/widgets/Makefile.in;
/cvsroot/mozilla/toolkit/content/tests/widgets/Makefile.in,v  <--  Makefile.in
new revision: 1.47; previous revision: 1.46
done
RCS file: /cvsroot/mozilla/toolkit/content/tests/widgets/test_menulist_null_value.xul,v
done
Checking in toolkit/content/tests/widgets/test_menulist_null_value.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_menulist_null_value.xul,v  <--  test_menulist_null_value.xul
initial revision: 1.1
done
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch, needs review mano]
Depends on: 412541
Your code needs to work with values of 0, "" and false (I don't know about false but we definitely have code that expects to set the value to 0 or "").
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch v3 (obsolete) — — Splinter Review 
Address the issue mentioned in comment 5.
This will fix the regressions in bug 412497, 412534 and 412541.

Neil, thanks for finding the exact issue.
Attachment #297445 - Flags: review?(mano)
Attached patch patch v4 — — Splinter Review 
Handle the undefined case too.
Attachment #297445 - Attachment is obsolete: true
Attachment #297461 - Flags: review?(mano)
Attachment #297445 - Flags: review?(mano)
Comment on attachment 297461 [details] [diff] [review]
patch v4

looks good, r=mano.
Attachment #297461 - Flags: review?(mano) → review+
Comment on attachment 297461 [details] [diff] [review]
patch v4

Fix the regressions.
Attachment #297461 - Flags: approval1.9?
(In reply to comment #9)
> (From update of attachment 297461 [details] [diff] [review])
> Fix the regressions.
> 

I tried this patch and it fixes bug 412534
Attachment #297461 - Flags: approval1.9? → approval1.9+
Attachment 297461 [details] [diff] checked in.

Checking in toolkit/content/widgets/menulist.xml;
/cvsroot/mozilla/toolkit/content/widgets/menulist.xml,v  <--  menulist.xml
new revision: 1.36; previous revision: 1.35
done
Checking in toolkit/content/tests/widgets/test_menulist_null_value.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_menulist_null_value.xul,v  <--  test_menulist_null_value.xul
new revision: 1.2; previous revision: 1.1
done
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 412534
You need to log in before you can comment on or make changes to this bug.