Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
258933006:
Fix selection of label having control element. (Closed)
|
Created:
6 years, 7 months ago by deepak.sa Modified:
6 years, 7 months ago CC:
blink-reviews, dglazkov+blink, adamk+blink_chromium.org, ojan Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionFix selection of label having control element.
Any click event on label goes to its control element. So, if
we tried to select the text inside label by dragging the mouse
or by double clicking on the text, it went to its control
element. This patch checks whether there is selection or not,
after the click is happened. If there is selection then do
nothing else continue.
New Layout test added:
fast/forms/label/label-selection.html
BUG=148912
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173310
Patch Set 1 #
Total comments: 8
Patch Set 2 : Layout test updated #
Total comments: 11
Patch Set 3 : Addressing comments #
Total comments: 2
Patch Set 4 : addressing comments #Patch Set 5 : Fixed Layout test for windows #
Messages
Total messages: 25 (0 generated)
Please have a look. Thanks!
> fast/selectors/label-selection.html It's a wrong path. fast/selectors/ is for CSS selectors. The test should be in editing/selection/ or fast/forms/label/. https://codereview.chromium.org/258933006/diff/1/LayoutTests/fast/selectors/l... File LayoutTests/fast/selectors/label-selection.html (right): https://codereview.chromium.org/258933006/diff/1/LayoutTests/fast/selectors/l... LayoutTests/fast/selectors/label-selection.html:5: function log(msg) Please remove this function, include ../../resource/js-test.js and use debug(). https://codereview.chromium.org/258933006/diff/1/LayoutTests/fast/selectors/l... LayoutTests/fast/selectors/label-selection.html:22: log("PASS selected text is :\"" + window.getSelection().toString() + "\""); Please use shouldBeEqualToString() defined in js-test.js. https://codereview.chromium.org/258933006/diff/1/Source/core/html/HTMLLabelEl... File Source/core/html/HTMLLabelElement.cpp (right): https://codereview.chromium.org/258933006/diff/1/Source/core/html/HTMLLabelEl... Source/core/html/HTMLLabelElement.cpp:141: // If text of label element is selected, do not pass Is this behavior compatible with other browsers?
https://codereview.chromium.org/258933006/diff/1/LayoutTests/fast/selectors/l... File LayoutTests/fast/selectors/label-selection.html (right): https://codereview.chromium.org/258933006/diff/1/LayoutTests/fast/selectors/l... LayoutTests/fast/selectors/label-selection.html:44: <input id="inputText" type='text'></input> nit: input should not have an end tag.
Layout test updated. Please have another look. Thanks! https://codereview.chromium.org/258933006/diff/1/LayoutTests/fast/selectors/l... File LayoutTests/fast/selectors/label-selection.html (right): https://codereview.chromium.org/258933006/diff/1/LayoutTests/fast/selectors/l... LayoutTests/fast/selectors/label-selection.html:5: function log(msg) On 2014/04/29 23:36:09, tkent (ooo until May 6) wrote: > Please remove this function, include ../../resource/js-test.js and use debug(). Done. https://codereview.chromium.org/258933006/diff/1/LayoutTests/fast/selectors/l... LayoutTests/fast/selectors/label-selection.html:22: log("PASS selected text is :\"" + window.getSelection().toString() + "\""); On 2014/04/29 23:36:09, tkent (ooo until May 6) wrote: > Please use shouldBeEqualToString() defined in js-test.js. > Done. https://codereview.chromium.org/258933006/diff/1/LayoutTests/fast/selectors/l... LayoutTests/fast/selectors/label-selection.html:44: <input id="inputText" type='text'></input> On 2014/04/30 01:13:14, keishi1 wrote: > nit: input should not have an end tag. Done. https://codereview.chromium.org/258933006/diff/1/Source/core/html/HTMLLabelEl... File Source/core/html/HTMLLabelElement.cpp (right): https://codereview.chromium.org/258933006/diff/1/Source/core/html/HTMLLabelEl... Source/core/html/HTMLLabelElement.cpp:141: // If text of label element is selected, do not pass On 2014/04/29 23:36:09, tkent (ooo until May 6) wrote: > Is this behavior compatible with other browsers? Firefox allows the text of label associated with input element to be selected either by double clicking or by dragging the mouse over text. In I.E. and Safari, label text is not getting selected.
https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... File LayoutTests/fast/forms/label/label-selection.html (right): https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:3: <script src="../../../resources/js-test.js"></script> I think form tests generally don't indent after <html>, <body>, <script> https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:5: <label id="labelWithInput" for="inputText">Some Text associated with input</label> Ditto. https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:16: shouldBeEqualToString('window.getSelection().toString()', "Some"); We shouldn't mix single and double quotes. We try to use single quotes in JavaScript https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:19: } Indentation is messed up. https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:22: { I think form tests generally don't break before curly braces
Updated HTML file. https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... File LayoutTests/fast/forms/label/label-selection.html (right): https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:3: <script src="../../../resources/js-test.js"></script> On 2014/05/02 07:52:03, keishi1 wrote: > I think form tests generally don't indent after <html>, <body>, <script> Done. https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:5: <label id="labelWithInput" for="inputText">Some Text associated with input</label> On 2014/05/02 07:52:03, keishi1 wrote: > Ditto. Done. https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:16: shouldBeEqualToString('window.getSelection().toString()', "Some"); On 2014/05/02 07:52:03, keishi1 wrote: > We shouldn't mix single and double quotes. We try to use single quotes in > JavaScript Done. https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:19: } On 2014/05/02 07:52:03, keishi1 wrote: > Indentation is messed up. Done. https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:22: { On 2014/05/02 07:52:03, keishi1 wrote: > I think form tests generally don't break before curly braces Sorry, but i am not able to follow the above comment. Can you elaborate? Thanks!
https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... File LayoutTests/fast/forms/label/label-selection.html (right): https://codereview.chromium.org/258933006/diff/20001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:22: { On 2014/05/02 08:28:27, deepak.sa wrote: > On 2014/05/02 07:52:03, keishi1 wrote: > > I think form tests generally don't break before curly braces > > Sorry, but i am not able to follow the above comment. Can you elaborate? > Thanks! I was talking about the line break between line 21 and 22. https://codereview.chromium.org/258933006/diff/40001/LayoutTests/fast/forms/l... File LayoutTests/fast/forms/label/label-selection.html (right): https://codereview.chromium.org/258933006/diff/40001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:14: testRunner.dumpAsText(); Don't need this. This is done automatically in js-test.js
Layout test updated. https://codereview.chromium.org/258933006/diff/40001/LayoutTests/fast/forms/l... File LayoutTests/fast/forms/label/label-selection.html (right): https://codereview.chromium.org/258933006/diff/40001/LayoutTests/fast/forms/l... LayoutTests/fast/forms/label/label-selection.html:14: testRunner.dumpAsText(); On 2014/05/02 08:48:35, keishi1 wrote: > Don't need this. This is done automatically in js-test.js Done.
lgtm
The CQ bit was checked by deepak.sa@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/258933006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink
The CQ bit was checked by deepak.sa@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/258933006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink
The CQ bit was checked by deepak.sa@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/258933006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink
The CQ bit was checked by deepak.sa@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.sa@samsung.com/258933006/80001
Message was sent while issue was closed.
Change committed as 173310
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/290153002/ by wangxianzhu@chromium.org. The reason for reverting is: Causes bug 370859 BUG=148912,370859. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
