Open a color input with display:none via a label

Open a color input with display:none via a label

BUG=452838
Committed: https://crrev.com/9447bd245501e7f1abb9b51f1420e8ef4f73c4dc
Cr-Commit-Position: refs/heads/master@{#403638}

[ramya.v, 2016-06-30 06:27:22]

ramya.v@samsung.com changed reviewers:
+ tkent@chromium.org

[ramya.v, 2016-06-30 06:27:22]

PTAL!

Thanks

[tkent, 2016-07-01 00:22:58]

tkent@chromium.org changed reviewers:
+ keishi@chromium.org

[tkent, 2016-07-01 00:30:20]

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.

[ramya.v, 2016-07-01 05:17:02]

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.

[tkent, 2016-07-01 06:54:08]

> 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/Layo...
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/Layo...
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/Layo...
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.

[ramya.v, 2016-07-01 09:14:46]

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/Layo...
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/Layo...
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/Layo...
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?

[tkent, 2016-07-01 09:23:28]

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/Layo...
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/Layo...
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/Layo...
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|.

[ramya.v, 2016-07-01 10:05:50]

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/Layo...
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/Layo...
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/Layo...
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.

[tkent, 2016-07-04 01:25:59]

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/Layo...
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/Layo...
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?

[ramya.v, 2016-07-04 04:47:56]

https://codereview.chromium.org/2110783002/diff/40001/third_party/WebKit/Layo...
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/Layo...
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.

[ramya.v, 2016-07-04 04:48:37]

The CQ bit was checked by ramya.v@samsung.com

[ramya.v, 2016-07-04 04:48:37]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2110783002/#ps60001
(title: "Updated test case as per review comments")

[commit-bot: I haz the power, 2016-07-04 04:48:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-04 06:07:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-04 06:07:07]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-04 06:09:01]

Description was changed from

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

BUG=452838
==========

to

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

BUG=452838

Committed: https://crrev.com/9447bd245501e7f1abb9b51f1420e8ef4f73c4dc
Cr-Commit-Position: refs/heads/master@{#403638}
==========

[commit-bot: I haz the power, 2016-07-04 06:09:02]

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