Bug 23696 - CHROMIUM: select element doesn't show new value when focus is switched in onchange event
Summary: CHROMIUM: select element doesn't show new value when focus is switched in onc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Fisher (:fishd, Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-02 13:57 PST by Darin Fisher (:fishd, Google)
Modified: 2009-02-02 16:45 PST (History)
0 users

See Also:


Attachments
test case (733 bytes, text/html)
2009-02-02 13:58 PST, Darin Fisher (:fishd, Google)
no flags Details
v1 patch (3.52 KB, patch)
2009-02-02 15:28 PST, Darin Fisher (:fishd, Google)
eric: review-
Details | Formatted Diff | Diff
v2 patch (3.58 KB, patch)
2009-02-02 16:24 PST, Darin Fisher (:fishd, Google)
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2009-02-02 13:57:45 PST
CHROMIUM: select element doesn't show new value when focus is switched in onchange event

Original bug report:
http://code.google.com/p/chromium/issues/detail?id=3514

It looks like we need to hide the PopupMenu before calling valueChanged.  Patch coming up.
Comment 1 Darin Fisher (:fishd, Google) 2009-02-02 13:58:09 PST
Created attachment 27258 [details]
test case
Comment 2 Darin Fisher (:fishd, Google) 2009-02-02 15:28:21 PST
Created attachment 27262 [details]
v1 patch

This already has R=ojan here:
http://codereview.chromium.org/19765
Comment 3 Eric Seidel (no email) 2009-02-02 15:32:35 PST
Comment on attachment 27262 [details]
v1 patch

     if (!m_listBox->parent()) {
 366         // Must get called after we have a client.
 367         addChild(m_listBox.get());
 368     }

Why?


Also:
 PopupMenu::~PopupMenu()
 {
     hide();
+    p.popup = 0;
 }

is p.popup a smartpointer?  If so, we should consider using .clear() instead.  If it's not, I expect we're leaking the PopupContainer.
Comment 4 Darin Fisher (:fishd, Google) 2009-02-02 16:01:31 PST
(In reply to comment #3)
> (From update of attachment 27262 [details] [review])
>      if (!m_listBox->parent()) {
>  366         // Must get called after we have a client.
>  367         addChild(m_listBox.get());
>  368     }

Whoa, that is actually a very stale comment.  I removed it.


> Also:
>  PopupMenu::~PopupMenu()
>  {
>      hide();
> +    p.popup = 0;
>  }
> 
> is p.popup a smartpointer?  If so, we should consider using .clear() instead. 
> If it's not, I expect we're leaking the PopupContainer.

Good catch.  Nulling out this RefPtr is definitely extraneous.
Comment 5 Darin Fisher (:fishd, Google) 2009-02-02 16:24:02 PST
Created attachment 27264 [details]
v2 patch
Comment 6 Eric Seidel (no email) 2009-02-02 16:35:46 PST
Comment on attachment 27264 [details]
v2 patch

LGTM.
Comment 7 Darin Fisher (:fishd, Google) 2009-02-02 16:45:29 PST
http://trac.webkit.org/changeset/40504