Bug 113220

Summary: Should close select popup when the element loses focus
Product: WebKit Reporter: Gustavo Noronha (kov) <gns>
Component: FormsAssignee: Gustavo Noronha (kov) <gns>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, esprehn+autocc, gtk-ews, mifenton, ojan.autocc, rniwa, syoichi, tkent, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 140506, 113931    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Patch
none
Archive of layout-test-results from webkit-ews-01 for mac-future
none
Archive of layout-test-results from webkit-ews-05 for mac-future
none
Patch
none
Archive of layout-test-results from webkit-ews-01 for mac-future
none
Patch none

Description Gustavo Noronha (kov) 2013-03-25 10:41:37 PDT
Doing that would make us match Firefox'es behaviour.
Comment 1 Gustavo Noronha (kov) 2013-03-25 10:48:44 PDT
Created attachment 194885 [details]
Patch
Comment 2 Build Bot 2013-03-25 11:23:02 PDT
Comment on attachment 194885 [details]
Patch

Attachment 194885 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17330037

New failing tests:
fast/forms/select-popup-closes-on-blur.html
Comment 3 Build Bot 2013-03-25 11:23:03 PDT
Created attachment 194892 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 4 Gustavo Noronha (kov) 2013-03-25 13:32:03 PDT
Created attachment 194913 [details]
Patch
Comment 5 Kent Tamura 2013-03-25 17:45:11 PDT
Comment on attachment 194913 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194913&action=review

> Source/WebCore/html/HTMLSelectElement.cpp:1231
> +        if (RenderMenuList* menuList = toRenderMenuList(renderer())) {

We need to check renderer()->isMenuList(). A blur event handler in capturing phase can modify renderer state before the blur event reaches to the target element.

> Source/WebCore/testing/Internals.cpp:2068
> +    HTMLSelectElement* select = toHTMLSelectElement(node);

Possible bad cast.  Please check element type even in testing code not to disturb fuzzers.

> Source/WebCore/testing/Internals.cpp:2069
> +    RenderMenuList* menuList = toRenderMenuList(select->renderer());

ditto.

> LayoutTests/fast/forms/select-popup-closes-on-blur-expected.txt:3
> +PASS internals.isSelectPopupVisible(popup) is true
> +PASS internals.isSelectPopupVisible(popup) is false
> +Test for http://bugs.webkit.org/show_bug.cgi?id=113220 Ensures select popup closes when focus changes.

Test description after test results is not good.  Please use description() to show the description.
Comment 6 Kent Tamura 2013-03-25 17:48:19 PDT
Comment on attachment 194913 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194913&action=review

>> Source/WebCore/html/HTMLSelectElement.cpp:1231
>> +        if (RenderMenuList* menuList = toRenderMenuList(renderer())) {
> 
> We need to check renderer()->isMenuList(). A blur event handler in capturing phase can modify renderer state before the blur event reaches to the target element.

Ah, my comment was wrong. We don't need to check it. because usesMenuList in defaultEventHandler ensures it.
Comment 7 Kent Tamura 2013-03-25 19:39:21 PDT
Comment on attachment 194913 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194913&action=review

> LayoutTests/ChangeLog:9
> +        * fast/forms/select-popup-closes-on-blur-expected.txt: Added.
> +        * fast/forms/select-popup-closes-on-blur.html: Added.

Please put the test into fast/forms/select/.
Comment 8 Gustavo Noronha (kov) 2013-03-26 09:09:29 PDT
Created attachment 195097 [details]
Patch
Comment 9 Build Bot 2013-03-26 10:18:34 PDT
Comment on attachment 195097 [details]
Patch

Attachment 195097 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17311349

New failing tests:
fast/forms/select/popup-closes-on-blur.html
fast/repaint/japanese-rl-selection-repaint-in-regions.html
Comment 10 Build Bot 2013-03-26 10:18:35 PDT
Created attachment 195107 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 11 Build Bot 2013-03-26 10:29:21 PDT
Comment on attachment 195097 [details]
Patch

Attachment 195097 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17335028
Comment 12 Gustavo Noronha (kov) 2013-03-26 11:11:40 PDT
Created attachment 195120 [details]
Patch
Comment 13 kov's GTK+ EWS bot 2013-03-26 11:59:07 PDT
Comment on attachment 195120 [details]
Patch

Attachment 195120 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/17172707
Comment 14 Gustavo Noronha (kov) 2013-03-26 12:49:17 PDT
GTK+ EWS failure caused by github being down (it's where it downloads the test fonts from), we should probably make the jhbuild build step a bit more robust.
Comment 15 Build Bot 2013-03-26 12:53:18 PDT
Comment on attachment 195120 [details]
Patch

Attachment 195120 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17147885
Comment 16 Kent Tamura 2013-03-26 17:05:05 PDT
Comment on attachment 195120 [details]
Patch

ok
Comment 17 Build Bot 2013-03-27 05:25:23 PDT
Comment on attachment 195120 [details]
Patch

Attachment 195120 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17295408

New failing tests:
fast/forms/select/popup-closes-on-blur.html
Comment 18 Build Bot 2013-03-27 05:25:26 PDT
Created attachment 195288 [details]
Archive of layout-test-results from webkit-ews-01 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-future  Platform: Mac OS X 10.8.2
Comment 19 Build Bot 2013-03-27 06:02:43 PDT
Comment on attachment 195120 [details]
Patch

Attachment 195120 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17231648

New failing tests:
fast/forms/select/popup-closes-on-blur.html
Comment 20 Build Bot 2013-03-27 06:02:47 PDT
Created attachment 195294 [details]
Archive of layout-test-results from webkit-ews-05 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-future  Platform: Mac OS X 10.8.2
Comment 21 Gustavo Noronha (kov) 2013-03-27 07:29:01 PDT
Created attachment 195316 [details]
Patch

Patch for landing, final run through the EWS
Comment 22 Build Bot 2013-03-27 13:11:28 PDT
Comment on attachment 195316 [details]
Patch

Attachment 195316 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17314430
Comment 23 Build Bot 2013-03-28 02:30:26 PDT
Comment on attachment 195316 [details]
Patch

Attachment 195316 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17251709

New failing tests:
fast/forms/select/popup-closes-on-blur.html
Comment 24 Build Bot 2013-03-28 02:30:29 PDT
Created attachment 195507 [details]
Archive of layout-test-results from webkit-ews-01 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-future  Platform: Mac OS X 10.8.2
Comment 25 Gustavo Noronha (kov) 2013-04-02 14:19:27 PDT
Created attachment 196223 [details]
Patch
Comment 26 Gustavo Noronha (kov) 2013-04-03 06:19:11 PDT
Comment on attachment 196223 [details]
Patch

Clearing flags on attachment: 196223

Committed r147548: <http://trac.webkit.org/changeset/147548>
Comment 27 Gustavo Noronha (kov) 2013-04-03 06:19:19 PDT
All reviewed patches have been landed.  Closing bug.