Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2372913002:
fixed <select> element isn't updated after its single option removing. (Closed)
Descriptionfixed <select> element isn't updated after its single option removing.
This problem is occurred because, in resetToDefaultSelection, there was no
handling when firstEnabledOption and lastSelectedOption are both nullptr.
So added this case.
BUG=650093
Committed: https://crrev.com/238fff24c2c0dbcc3cec19a6a981b74d11928092
Cr-Commit-Position: refs/heads/master@{#421733}
Patch Set 1 #Patch Set 2 : add handling case when first and last element is both null in resetToDefaultSelection #Patch Set 3 : add layout test #
Total comments: 8
Patch Set 4 : refactored html fild and changed test to reference test #
Total comments: 6
Patch Set 5 : minor code cleaning #Patch Set 6 : changed script activation point #
Total comments: 7
Patch Set 7 : LayoutTest Code updated #
Total comments: 6
Patch Set 8 : remove anchor element and adjunst some indents #
Messages
Total messages: 37 (18 generated)
jhwon0415@gmail.com changed reviewers: + jinho.bang@samsung.com
fixed bug
The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
PTAL
The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== fixed <select> element isn't updated after its single option removing. Check size of selec element before leave optionRemoved fucntion. If size is 0, set value to empty string BUG=650093 ========== to ========== fixed <select> element isn't updated after its single option removing. This problem is occurred because, in resetToDefaultSelection, there was no handling when firstEnabledOption and lastSelectedOption are both nullptr. So added this case. BUG=650093 ==========
jhwon0415@gmail.com changed reviewers: + skobes@chromium.org, tkent@chromium.org
Hello, I'm newbie developer who wants to get in this chromium project. I'll be happy if you review my code! Thank you BTW, I have question about unit test. I want to make some unit test, but when I checked this problem on not patched version in development console, there is no object option elements in selection element, and element is not updated. Then, how can I make unit test?
Thank you for woking on this! C++ code change looks ok. On 2016/09/27 at 16:24:03, jhwon0415 wrote: > I want to make some unit test, but when I checked this problem on not patched version in development console, there is no object option elements in selection element, and element is not updated. Then, how can I make unit test? It seems HTMLSelectElement::optionToBeShown() returns a wrong option, but HTMLSelectElement::selectedOption() returns null correctly. So, we can't make a test using web-exposed API of HTMLSelectElement. If you'd like to make C++ unit test, Add new TEST_F() to HTMLSelectElementTest.cpp, and test if optionToBeShown() returns a correct value. If you'd like to make a LayoutTest, we need to add a reference test to third_party/WebKit/LayoutTests/fast/forms/select/. "Reference test" is a pair of HTML files which should have identical rendering. You find many pairs of HTML files named foo.html and foo-expected.html in third_party/WebKit/LayoutTests/.
I added layout test this time. I couldn't get proper result by using unit test because DOM option object is already deleted. So I made layout test. On Linux and windows, without min-width property, this issue was not occurred. However on Mac, issue reproduced without min-width property. Because I don't have mac device, I added min-width property in layout test. Thank you.
I'm not owner but I left some comments for layout test. https://codereview.chromium.org/2372913002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.txt (right): https://codereview.chromium.org/2372913002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.txt:1: Remove option Please make expected html file which have identical rendering instead of this expected txt. https://codereview.chromium.org/2372913002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html (right): https://codereview.chromium.org/2372913002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:2: <html> According to layout test style guide line[1], <html>, <head>, <body> tags are normally omitted. [1] https://www.chromium.org/blink/coding-style/layout-test-style-guidelines https://codereview.chromium.org/2372913002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:5: <script> Please move this script block below <a></a>. https://codereview.chromium.org/2372913002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:7: function load() { You can simplify this script as follows: var removeLink = document.querySelector('a'); removeLink.addEventListener('click', () => { document.querySelector('option').remove(); }); removeLink.click(); https://codereview.chromium.org/2372913002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:29: <form> This looks unnecessary. https://codereview.chromium.org/2372913002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:33: <a id="a" herf="#" onload="test()">Remove option</a> Let's remove 'onload' attribute as well. https://codereview.chromium.org/2372913002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:35: <script> This looks unnecessary. https://codereview.chromium.org/2372913002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:43: if (window.testRunner) You could do ref test instead of this pixel test. That's enough if you just write a expected html file after copying this.
As zino mentioned, I refactored code and changed test to reference test. It goes well. Thank you
I left some minor comments. Thanks. https://codereview.chromium.org/2372913002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.html (right): https://codereview.chromium.org/2372913002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.html:9: <select id="s"></select> id attribute is unnecessary. https://codereview.chromium.org/2372913002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.html:11: <a id="a" herf="#">Remove option</a> ditto https://codereview.chromium.org/2372913002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html (right): https://codereview.chromium.org/2372913002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:1: <style type="text/css"> Need <!DOCTYPE html> https://codereview.chromium.org/2372913002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:9: <select id="s"> We don't need id attributes here if they doesn't be used. https://codereview.chromium.org/2372913002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:16: function load() { This is unnecessary. https://codereview.chromium.org/2372913002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:24: window.onload = load; This is unnecessary.
Cleaned code as referred. Thanks!
Changed remove script's activation point to specific point. Now remove is called when window calls 'load' Thank you
On 2016/09/28 17:17:43, jhwon0415 wrote: > Cleaned code as referred. > Thanks! looks good. Please wait for tkent@'s review. Thank you!
https://codereview.chromium.org/2372913002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.html (right): https://codereview.chromium.org/2372913002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.html:1: <title>Test for HTMLSelectElement.remove() on an Options object</title> Please add <!DOCTYPE html> https://codereview.chromium.org/2372913002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.html:12: This line is unnecessary. https://codereview.chromium.org/2372913002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html (right): https://codereview.chromium.org/2372913002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:1: <title>Test for HTMLSelectElement.remove() on an Options object</title> Please add <!DOCTYPE html> https://codereview.chromium.org/2372913002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:4: select { nit: Do not indent the whole content of <style>. i.e. <style> select { min-width: 100px; } </style> https://codereview.chromium.org/2372913002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:10: <option selected >hello</option> A space after "selected" is unnecessary. https://codereview.chromium.org/2372913002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:21: removeLink.click(); I don't think we need to click the <a>. Why don't you just call document.querySelector('option').remove()? window.addEventListener('load', function() { document.querySelector('option').remove(); }); https://codereview.chromium.org/2372913002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:24: This line is unnecessary.
Thanks for reviewing. I changed code as mentioned. Then, this time, anchor elements looks unnecessary to me. Is it okay to remove anchor element from layout test?
https://codereview.chromium.org/2372913002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.html (right): https://codereview.chromium.org/2372913002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.html:1: <title>Test for HTMLSelectElement.remove() on an Options object</title> Please add <!DOCTYPE html> https://codereview.chromium.org/2372913002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.html:5: min-width: 100px; Please indent the content of a ruleset. https://codereview.chromium.org/2372913002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single-expected.html:11: <a herf="#">Remove option</a> Please remove it. https://codereview.chromium.org/2372913002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html (right): https://codereview.chromium.org/2372913002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:6: min-width: 100px; Please indent the content of a ruleset. https://codereview.chromium.org/2372913002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:14: <a herf="#">Remove option</a> Please remove it. https://codereview.chromium.org/2372913002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/select/select-remove-option-single.html:18: document.querySelector('option').remove(); Please indent the content of a function.
Sorry for doctype in expceted.html, i forgot to change it. removed anchor and made some indents.
The CQ bit was checked by tkent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tkent@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== fixed <select> element isn't updated after its single option removing. This problem is occurred because, in resetToDefaultSelection, there was no handling when firstEnabledOption and lastSelectedOption are both nullptr. So added this case. BUG=650093 ========== to ========== fixed <select> element isn't updated after its single option removing. This problem is occurred because, in resetToDefaultSelection, there was no handling when firstEnabledOption and lastSelectedOption are both nullptr. So added this case. BUG=650093 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== fixed <select> element isn't updated after its single option removing. This problem is occurred because, in resetToDefaultSelection, there was no handling when firstEnabledOption and lastSelectedOption are both nullptr. So added this case. BUG=650093 ========== to ========== fixed <select> element isn't updated after its single option removing. This problem is occurred because, in resetToDefaultSelection, there was no handling when firstEnabledOption and lastSelectedOption are both nullptr. So added this case. BUG=650093 Committed: https://crrev.com/238fff24c2c0dbcc3cec19a6a981b74d11928092 Cr-Commit-Position: refs/heads/master@{#421733} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/238fff24c2c0dbcc3cec19a6a981b74d11928092 Cr-Commit-Position: refs/heads/master@{#421733} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
