Option Groups with display: none should not show the children option elements

The patch skips showing <option> elements in popupmenu that
are children of <optgroup> element with "display: none". The 
popupmenu is handled differently on Mac. Hence the layout 
testcase in this patch is only for Windows/Linux.There is a 
separate patch[1] that covers testing on Mac.

[1] https://codereview.chromium.org/447503002/ 

BUG=398051
TEST=fast/forms/select/popup-with-display-none-optgroup.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179790

[spartha, 2014-07-31 21:26:05]

On 2014/07/31 21:25:43, spartha wrote:

PTAL

[keishi, 2014-08-01 02:01:11]

https://codereview.chromium.org/415343003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderMenuList.cpp (right):

https://codereview.chromium.org/415343003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderMenuList.cpp:505: bool displayNone = style &&
style->display() == NONE;
This nested display:none case is already handled correctly for listboxes, so we
just need to fix the menu list popup.

So I think this change to RenderMenuList.cpp should be all you need. You
shouldn't need to change HTMLSelectElement.cpp and HTMLOptionElement.cpp.

But there is a problem on Mac. ExternalPopupMenu ignores
PopupMenuStyle::isDisplayNone. We would need to fix that too.

[spartha, 2014-08-01 09:29:16]

https://codereview.chromium.org/415343003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderMenuList.cpp (right):

https://codereview.chromium.org/415343003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderMenuList.cpp:505: bool displayNone = style &&
style->display() == NONE;
On 2014/08/01 02:01:11, keishi wrote:
> This nested display:none case is already handled correctly for listboxes, so
we
> just need to fix the menu list popup.
> 
> So I think this change to RenderMenuList.cpp should be all you need. You
> shouldn't need to change HTMLSelectElement.cpp and HTMLOptionElement.cpp.
> 
> But there is a problem on Mac. ExternalPopupMenu ignores
> PopupMenuStyle::isDisplayNone. We would need to fix that too.

Thanks for the review! Even I felt that it would be sufficient. But the
RenderMenuList change will affect only when the popup is shown. But the test
case in the patch will fail without the HTMLSelectElement.cpp and
HTMLOptionElement.cpp changes. The test case just sets the focus on the select
and uses keyup/down to select an option. 
I haven't tried the patch on Mac. Let me try it on the bots first (I don't have
access to a Mac :( )

[keishi, 2014-08-04 03:04:04]

I see.
I am trying to make the select element a standard shadow dom based
implementation so this special handling of display:none should only be used for
menulist mode.

https://codereview.chromium.org/415343003/diff/140001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/415343003/diff/140001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:295: m_style->setDisplay(NONE);
I don't think we should be changing the style of the option element.

We used to have HTMLOptionElement::isDisplayNone and
HTMLOptGroupElement::isDisplayNone before r178354.
We should revive those and use them inside HTMLSelectElement::nextValidIndex and
RenderMenuList::itemStyle.

[spartha, 2014-08-04 15:59:02]

Thanks! All the changes are now in the blink side. I will add a browser test in
the next patch. PTAL!

https://codereview.chromium.org/415343003/diff/140001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/415343003/diff/140001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:295: m_style->setDisplay(NONE);
On 2014/08/04 03:04:03, keishi wrote:
> I don't think we should be changing the style of the option element.
> 
> We used to have HTMLOptionElement::isDisplayNone and
> HTMLOptGroupElement::isDisplayNone before r178354.
> We should revive those and use them inside HTMLSelectElement::nextValidIndex
and
> RenderMenuList::itemStyle.

Done.

[keishi, 2014-08-06 03:00:21]

https://codereview.chromium.org/415343003/diff/160001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/select/popup-with-display-none-optgroup.html
(right):

https://codereview.chromium.org/415343003/diff/160001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/popup-with-display-none-optgroup.html:39:
eventSender.keyDown('downArrow', []);
nit: second argument can be removed

https://codereview.chromium.org/415343003/diff/160001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/415343003/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:404: HTMLElement* parent =
Traversal<HTMLElement>::firstAncestor(*this);
I don't think we need to traverse.
Options that are displayed in a select element will always have select or
optgroup as the parent node.

This can be
if (parent is optgroup)
  return parent.isDisplayNone

Optgroup's parent should also be only select or optgroup.
So HTMLOptGroupElement::isDisplayNone should also be implemented the same way.

https://codereview.chromium.org/415343003/diff/160001/Source/web/ExternalPopu...
File Source/web/ExternalPopupMenu.cpp (right):

https://codereview.chromium.org/415343003/diff/160001/Source/web/ExternalPopu...
Source/web/ExternalPopupMenu.cpp:227: if (itemCount == m_itemCount ||
externalPopupMenuItemIndex < 0)
I get what you're trying to do with itemCount == m_itemCount but I worry it
might make the behavior  hard to predict for web developers debugging their
apps.
This should only be called sparingly so maybe we don't need this.

[spartha, 2014-08-06 06:23:02]

https://codereview.chromium.org/415343003/diff/160001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/415343003/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:404: HTMLElement* parent =
Traversal<HTMLElement>::firstAncestor(*this);
On 2014/08/06 03:00:20, keishi wrote:
> I don't think we need to traverse.
> Options that are displayed in a select element will always have select or
> optgroup as the parent node.
> 
> This can be
> if (parent is optgroup)
>   return parent.isDisplayNone
> 
> Optgroup's parent should also be only select or optgroup.
> So HTMLOptGroupElement::isDisplayNone should also be implemented the same way.
The spec says "In HTML 4, all OPTGROUP elements must be specified directly
within a SELECT element (i.e., groups may not be nested).", hence did not find
it necessary to add a similar method for <optgroup>

[keishi, 2014-08-06 06:29:48]

On 2014/08/06 06:23:02, spartha wrote:
>
https://codereview.chromium.org/415343003/diff/160001/Source/core/html/HTMLOp...
> File Source/core/html/HTMLOptionElement.cpp (right):
> 
>
https://codereview.chromium.org/415343003/diff/160001/Source/core/html/HTMLOp...
> Source/core/html/HTMLOptionElement.cpp:404: HTMLElement* parent =
> Traversal<HTMLElement>::firstAncestor(*this);
> On 2014/08/06 03:00:20, keishi wrote:
> > I don't think we need to traverse.
> > Options that are displayed in a select element will always have select or
> > optgroup as the parent node.
> > 
> > This can be
> > if (parent is optgroup)
> >   return parent.isDisplayNone
> > 
> > Optgroup's parent should also be only select or optgroup.
> > So HTMLOptGroupElement::isDisplayNone should also be implemented the same
way.
> The spec says "In HTML 4, all OPTGROUP elements must be specified directly
> within a SELECT element (i.e., groups may not be nested).", hence did not find
> it necessary to add a similar method for <optgroup>

Nesting of optgroups shouldn't be possible but it is possible right now as
legacy behavior. I don't feel strongly about it so you don't need to support it
if you don't want to.

[spartha, 2014-08-06 08:13:31]

PTAL! Thanks

https://codereview.chromium.org/415343003/diff/160001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/select/popup-with-display-none-optgroup.html
(right):

https://codereview.chromium.org/415343003/diff/160001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/popup-with-display-none-optgroup.html:39:
eventSender.keyDown('downArrow', []);
On 2014/08/06 03:00:20, keishi wrote:
> nit: second argument can be removed

Done.

https://codereview.chromium.org/415343003/diff/160001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/415343003/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:404: HTMLElement* parent =
Traversal<HTMLElement>::firstAncestor(*this);
Avoided calling the Traversal
On 2014/08/06 03:00:20, keishi wrote:
> I don't think we need to traverse.
> Options that are displayed in a select element will always have select or
> optgroup as the parent node.
> 
> This can be
> if (parent is optgroup)
>   return parent.isDisplayNone
> 
> Optgroup's parent should also be only select or optgroup.
> So HTMLOptGroupElement::isDisplayNone should also be implemented the same way.

https://codereview.chromium.org/415343003/diff/160001/Source/web/ExternalPopu...
File Source/web/ExternalPopupMenu.cpp (right):

https://codereview.chromium.org/415343003/diff/160001/Source/web/ExternalPopu...
Source/web/ExternalPopupMenu.cpp:227: if (itemCount == m_itemCount ||
externalPopupMenuItemIndex < 0)
On 2014/08/06 03:00:20, keishi wrote:
> I get what you're trying to do with itemCount == m_itemCount but I worry it
> might make the behavior  hard to predict for web developers debugging their
> apps.
> This should only be called sparingly so maybe we don't need this.

Done.

[keishi, 2014-08-06 08:29:08]

LGTM.

Please update the issue description.

[spartha, 2014-08-06 09:36:09]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-08-06 09:36:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/415343003/200001

[spartha, 2014-08-06 10:03:53]

On 2014/08/06 09:36:44, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> 
https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/415343003/200001
@tkent need a owner lgtm! thanks

[spartha, 2014-08-06 11:58:03]

The CQ bit was unchecked by sudarshan.p@samsung.com

[tkent, 2014-08-06 12:46:08]

https://codereview.chromium.org/415343003/diff/200001/public/platform/WebVect...
File public/platform/WebVector.h (right):

https://codereview.chromium.org/415343003/diff/200001/public/platform/WebVect...
public/platform/WebVector.h:155: void shrink(size_t size)
Why is this necessary?
WebVector has no rich operations intentionally. We should not add this.

[spartha, 2014-08-06 13:31:09]

https://codereview.chromium.org/415343003/diff/200001/public/platform/WebVect...
File public/platform/WebVector.h (right):

https://codereview.chromium.org/415343003/diff/200001/public/platform/WebVect...
public/platform/WebVector.h:155: void shrink(size_t size)
On 2014/08/06 12:46:07, tkent wrote:
> Why is this necessary?
> WebVector has no rich operations intentionally. We should not add this.
The function pre-allocates the vector for efficiency. This worked fine earlier
as the size was fixed. Now with this patch, the final size can change and we
have to resize the vector for the patch to work. I wanted to avoid looping
through the list again to make a copy, hence added the method. If the goal is to
keep WebVector simple, then I can check for alternative approach. Please let me
know your opinion.

[tkent, 2014-08-07 01:13:38]

https://codereview.chromium.org/415343003/diff/200001/Source/web/ExternalPopu...
File Source/web/ExternalPopupMenu.cpp (right):

https://codereview.chromium.org/415343003/diff/200001/Source/web/ExternalPopu...
Source/web/ExternalPopupMenu.cpp:188: WebVector<WebMenuItemInfo>
items(static_cast<size_t>(itemCount));
This should be Vector<WebMenuItemInfo>.

https://codereview.chromium.org/415343003/diff/200001/Source/web/ExternalPopu...
Source/web/ExternalPopupMenu.cpp:219: info->items.swap(items);
This should be |info->items = items;| if |items| is a Vector.

[spartha, 2014-08-07 06:57:25]

PTAL, thanks!

https://codereview.chromium.org/415343003/diff/200001/Source/web/ExternalPopu...
File Source/web/ExternalPopupMenu.cpp (right):

https://codereview.chromium.org/415343003/diff/200001/Source/web/ExternalPopu...
Source/web/ExternalPopupMenu.cpp:188: WebVector<WebMenuItemInfo>
items(static_cast<size_t>(itemCount));
On 2014/08/07 01:13:38, tkent wrote:
> This should be Vector<WebMenuItemInfo>.

Done.

https://codereview.chromium.org/415343003/diff/200001/Source/web/ExternalPopu...
Source/web/ExternalPopupMenu.cpp:219: info->items.swap(items);
On 2014/08/07 01:13:38, tkent wrote:
> This should be |info->items = items;| if |items| is a Vector.

Done.

[tkent, 2014-08-07 06:58:50]

lgtm

[commit-bot: I haz the power, 2014-08-07 08:22:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-08-07 08:39:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-07 08:39:38]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[spartha, 2014-08-07 09:49:41]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-08-07 09:50:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/415343003/240001

[commit-bot: I haz the power, 2014-08-07 09:58:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[commit-bot: I haz the power, 2014-08-07 10:56:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-07 10:56:55]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[spartha, 2014-08-07 20:14:25]

On 2014/08/07 10:56:55, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_dbg on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

I have made a small change to fix dbg build assert. PTAL

[keishi, 2014-08-08 02:16:13]

On 2014/08/07 20:14:25, spartha wrote:
> On 2014/08/07 10:56:55, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_blink_dbg on tryserver.blink
> >
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
> 
> I have made a small change to fix dbg build assert. PTAL

LGTM

[spartha, 2014-08-08 04:48:01]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-08-08 04:48:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/415343003/260001

[commit-bot: I haz the power, 2014-08-08 06:14:18]

Change committed as 179790