Open a color input with display:none via a label (issue 2110783002 by ramya.v@samsung.com)

ram...@samsung.com

unread,

Jun 30, 2016, 2:27:23 AM6/30/16

to tk...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org

Reviewers: tkent
CL: https://codereview.chromium.org/2110783002/

Message:
PTAL!

Thanks

Description:
Open a color input with display:none via a label

BUG=452838

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+25, -1 lines):
A third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
M third_party/WebKit/Source/core/html/forms/ColorInputType.cpp

Index: third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
diff --git a/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html b/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
new file mode 100644
index 0000000000000000000000000000000000000000..1be0b2b7b5adfcee694db70bbd1bdb54d51b27cc
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<script src="../../../resources/testharness.js"></script>
+<script src="../../../resources/testharnessreport.js"></script>
+
+<input id="colorPick" type="color" />
+<label for="colorPick" id="labelPick">Pick a color</label>
+
+<script>
+test (function() {
+ assert_true(window.eventSender !== null);
+}, "window.eventSender is required for the test to run");
+
+test (function() {
+ var colorPicker = document.getElementById("colorPick");
+ colorPicker.style.display = "none";
+ var labelPick = document.getElementById("labelPick");
+ var x = labelPick.offsetLeft + labelPick.offsetWidth/2;
+ var y = labelPick.offsetTop + labelPick.offsetHeight/2;
+ eventSender.mouseMoveTo(x, y);
+ eventSender.mouseDown();
+ eventSender.mouseUp();
+ assert_true(testRunner.isChooserShown());
+}, "Tests click on label for color picker with display none should show chooser");
+</script>
Index: third_party/WebKit/Source/core/html/forms/ColorInputType.cpp
diff --git a/third_party/WebKit/Source/core/html/forms/ColorInputType.cpp b/third_party/WebKit/Source/core/html/forms/ColorInputType.cpp
index 3dd217acfd75aa5dfe7a994dca9ae9476ac6e6a8..66b9327d052b4ebc99e34f1738484b444ec8ceeb 100644
--- a/third_party/WebKit/Source/core/html/forms/ColorInputType.cpp
+++ b/third_party/WebKit/Source/core/html/forms/ColorInputType.cpp
@@ -167,7 +167,7 @@ void ColorInputType::setValue(const String& value, bool valueChanged, TextFieldE

void ColorInputType::handleDOMActivateEvent(Event* event)
{
- if (element().isDisabledFormControl() || !element().layoutObject())
+ if (element().isDisabledFormControl())
return;

if (!UserGestureIndicator::utilizeUserGesture())

tk...@chromium.org

unread,

Jun 30, 2016, 8:30:21 PM6/30/16

to ram...@samsung.com, kei...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

I wonder whether the color picker is closed correctly on the document shutdown
if it is opened during display:none.

We call InputTypeView::closePopupView() from HTMLInputElement::detach. It's
enough because we open the color picker only if the color input has a
LayoutObject.

https://codereview.chromium.org/2110783002/

ram...@samsung.com

unread,

Jul 1, 2016, 1:17:02 AM7/1/16

to tk...@chromium.org, kei...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

On 2016/07/01 00:30:20, tkent wrote:
> I wonder whether the color picker is closed correctly on the document shutdown
> if it is opened during display:none.
>
> We call InputTypeView::closePopupView() from HTMLInputElement::detach. It's
> enough because we open the color picker only if the color input has a
> LayoutObject.

Ex: https://jsfiddle.net/pe0v07vn/ (click on label on load of the page)
After 5sec,
Firefox: only input is removed.
Chrome: With this patch, input as well as picker dialog are removed.

Updated the existing test-case with this scenario.

https://codereview.chromium.org/2110783002/

tk...@chromium.org

unread,

Jul 1, 2016, 2:54:09 AM7/1/16

to ram...@samsung.com, kei...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

> Chrome: With this patch, input as well as picker dialog are removed.

Can you tell me what code path closed the picker?

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
File
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
(right):

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html#newcode25
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html:25:
document.body.removeChild(div);
In this case, HTMLInputElement::removedFrom() is called. So the picker
is closed by InputTypeView::closePopupView().

I talked about a case of a document destruction, which won't call
HTMLInputElement::removedFrom().

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html#newcode26
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html:26:
setTimeout(function() { assert_false(testRunner.isChooserShown()); },
5000);
This doesn't work. This is a sync test.

https://codereview.chromium.org/2110783002/

ram...@samsung.com

unread,

Jul 1, 2016, 5:14:46 AM7/1/16

to tk...@chromium.org, kei...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
File
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
(right):

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html#newcode25
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html:25:
document.body.removeChild(div);

On 2016/07/01 06:54:08, tkent wrote:
> In this case, HTMLInputElement::removedFrom() is called. So the
picker is
> closed by InputTypeView::closePopupView().
>
> I talked about a case of a document destruction, which won't call
> HTMLInputElement::removedFrom().

Can you please provide a test case of document destruction? Will check
if closePopup can be called from document destruction code path as well.
Also one more doubt, is there any specific reason that file input and
color input handled differently in case of display none case?

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html#newcode26
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html:26:
setTimeout(function() { assert_false(testRunner.isChooserShown()); },
5000);

On 2016/07/01 06:54:08, tkent wrote:
> This doesn't work. This is a sync test.

Did not get you? Test case shows pass. Is this line invalid?

https://codereview.chromium.org/2110783002/

tk...@chromium.org

unread,

Jul 1, 2016, 5:23:28 AM7/1/16

to ram...@samsung.com, kei...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
File
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
(right):

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html#newcode25
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html:25:
document.body.removeChild(div);

On 2016/07/01 at 09:14:45, ramya.v wrote:
> On 2016/07/01 06:54:08, tkent wrote:
> > In this case, HTMLInputElement::removedFrom() is called. So the
picker is
> > closed by InputTypeView::closePopupView().
> >
> > I talked about a case of a document destruction, which won't call
> > HTMLInputElement::removedFrom().
>
>
> Can you please provide a test case of document destruction? Will check
if closePopup can be called from document destruction code path as well.
> Also one more doubt, is there any specific reason that file input and
color input handled differently in case of display none case?

for example,
1. click a label associated to input[type=color] with display:none
2. click a link in the same document to navigate to another page.

Picker implementations of input[type=color] and input[type=file] are
very different. So I have a concern.

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html#newcode26
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html:26:
setTimeout(function() { assert_false(testRunner.isChooserShown()); },
5000);

On 2016/07/01 at 09:14:45, ramya.v wrote:
> On 2016/07/01 06:54:08, tkent wrote:
> > This doesn't work. This is a sync test.
>
> Did not get you? Test case shows pass. Is this line invalid?

I didn't try this patch. But this assert_false() won't be executed in
run-webkit-tests command or |content_shel --run-layout-test|.

https://codereview.chromium.org/2110783002/

ram...@samsung.com

unread,

Jul 1, 2016, 6:05:51 AM7/1/16

to tk...@chromium.org, kei...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
File
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
(right):

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html#newcode25
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html:25:
document.body.removeChild(div);

On 2016/07/01 09:23:28, tkent wrote:
> On 2016/07/01 at 09:14:45, ramya.v wrote:
> > On 2016/07/01 06:54:08, tkent wrote:
> > > In this case, HTMLInputElement::removedFrom() is called. So the
picker is
> > > closed by InputTypeView::closePopupView().
> > >
> > > I talked about a case of a document destruction, which won't call
> > > HTMLInputElement::removedFrom().
> >
> >
> > Can you please provide a test case of document destruction? Will
check if
> closePopup can be called from document destruction code path as well.
> > Also one more doubt, is there any specific reason that file input
and color
> input handled differently in case of display none case?
>
> for example,
> 1. click a label associated to input[type=color] with display:none
> 2. click a link in the same document to navigate to another page.
>
> Picker implementations of input[type=color] and input[type=file] are
very
> different. So I have a concern.

Made below test case.

<!DOCTYPE html>
<input type="color" id="colorPick" />
<label for="colorPick">Pick a color</label>
<a id="link" href="http://www.google.com">Google</a>
<style>
input {
display: none;
}
</style>
<script>
setTimeout(function() {
document.getElementById("link").click();
}, 5000);
</script>

In this case, irrespective of color picker opened with input display
none or not,
Firefox: picker not dismissed.
Chrome:
Windows : picker is not dismissed.
Linux: picker is dismissed.

I've linux opensource setup. There call-stack is as below in this case.
ColorInputType::closePopupView
ContainerNode::detach()
Element::detach()
ContainerNode::detach()
Element::detach()
ContainerNode::detach()
Document::detach()
FrameLoader::prepareForCommit()

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html#newcode26
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html:26:
setTimeout(function() { assert_false(testRunner.isChooserShown()); },
5000);

On 2016/07/01 09:23:28, tkent wrote:
> On 2016/07/01 at 09:14:45, ramya.v wrote:
> > On 2016/07/01 06:54:08, tkent wrote:
> > > This doesn't work. This is a sync test.
> >
> > Did not get you? Test case shows pass. Is this line invalid?
>
> I didn't try this patch. But this assert_false() won't be executed in
> run-webkit-tests command or |content_shel --run-layout-test|.

Thanks got your point :) Will remove testing this way.

https://codereview.chromium.org/2110783002/

tk...@chromium.org

unread,

Jul 3, 2016, 9:26:00 PM7/3/16

to ram...@samsung.com, kei...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
File
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
(right):

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html#newcode25
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html:25:
document.body.removeChild(div);

On 2016/07/01 at 10:05:50, ramya.v wrote:
> In this case, irrespective of color picker opened with input display
none or not,
> Firefox: picker not dismissed.
> Chrome:
> Windows : picker is not dismissed.
> Linux: picker is dismissed.

Thank you for the investigation. So, this CL won't make the situation
worse. LGTM.
BTW, would you file a bug about this scenario please?

https://codereview.chromium.org/2110783002/

ram...@samsung.com

unread,

Jul 4, 2016, 12:47:56 AM7/4/16

to tk...@chromium.org, kei...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
File
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html
(right):

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html#newcode25
third_party/WebKit/LayoutTests/fast/forms/color/display-none-input-color-chooser-shown.html:25:
document.body.removeChild(div);

On 2016/07/04 01:25:59, tkent wrote:
> On 2016/07/01 at 10:05:50, ramya.v wrote:
> > In this case, irrespective of color picker opened with input display
none or
> not,
> > Firefox: picker not dismissed.
> > Chrome:
> > Windows : picker is not dismissed.
> > Linux: picker is dismissed.
>
> Thank you for the investigation. So, this CL won't make the situation
worse.
> LGTM.
> BTW, would you file a bug about this scenario please?

Filed a bug in crbug.com/625525.
Done.

https://codereview.chromium.org/2110783002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 4, 2016, 12:48:42 AM7/4/16

to ram...@samsung.com, tk...@chromium.org, kei...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2110783002/60001

https://codereview.chromium.org/2110783002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 4, 2016, 2:07:06 AM7/4/16

to ram...@samsung.com, tk...@chromium.org, kei...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

Committed patchset #4 (id:60001)

https://codereview.chromium.org/2110783002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 4, 2016, 2:07:08 AM7/4/16

to ram...@samsung.com, tk...@chromium.org, kei...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

CQ bit was unchecked.

https://codereview.chromium.org/2110783002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Jul 4, 2016, 2:09:02 AM7/4/16

to ram...@samsung.com, tk...@chromium.org, kei...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

Patchset 4 (id:??) landed as
https://crrev.com/9447bd245501e7f1abb9b51f1420e8ef4f73c4dc
Cr-Commit-Position: refs/heads/master@{#403638}

https://codereview.chromium.org/2110783002/

Reply all

Reply to author

Forward