Implement select listbox using shadow DOM

Implement select listbox using shadow DOM

User visible behavior changes.
- Old listbox could only scroll in itemHeight increments but now we have smooth scroll.
- Mouse events need to be dispatched on the option element, not select element.
- Select element height is determined based on the height of the first item, not the font size.
- Spatial navigation now draws a black dashed outline around the active selection instead of a focus ring.

We can no longer let RenderListBox draw the focus ring for spatial navigation mode because the items are drawn on another RenderLayer.
I added a user agent stylesheet only :-internal-spatial-navigation-focus pseudo type so a style can be applied to the spatial navigation active element.
If we add the default focus ring on the option element, it gets clipped to the select element so I changed it to a black dashed outline.

Option and optgroups were represented as AXListBoxOptions but now only options are AXListBoxOptions and optgroups are represented by the optgroup-label AXNodeObject. We let AXRenderObject handle hit testing so we can remove the custom hit testing.

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

[keishi, 2014-06-22 11:59:56]

I added comments to Patch set 6 but I fixed a couple of places so please review
Patch set 7.

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1961: crbug.com/234871
fast/forms/select-max-length.html [ Slow ]
We have the 10,000 option limit so we need to make this fast. I will try to fix
this.

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1962: 
fast/spatial-navigation/snav-multiple-select-focusring.html should be the only
test failing.

Without the custom renderer, it is hard to add styles just for spatial
navigation mode. If we were to implement this, I think we need a
-internal-spatial-navigation-focus pseudo class.

Could we do this later?

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/events...
File LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-listbox.html
(right):

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-listbox.html:61:
shouldBe('box.scrollTop', '2*itemHeight+10');
Old listbox could only scroll in itemHeight increments but now we have smooth
scroll.

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/listbox-hit-test-zoomed.html (left):

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/listbox-hit-test-zoomed.html:39: sl.dispatchEvent(event);
Mouse event target needs to be an option element now.

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/listbox-onchange.html (right):

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/listbox-onchange.html:136:
sl.options[index].dispatchEvent(event);
Event target needs to be an option element now.

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/resources/select-live-pseudo-selectors.js (left):

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/resources/select-live-pseudo-selectors.js:119:
shouldBe('backgroundOf("multiple-empty")', 'transparentColor');
Background color of this option is -internal-active-listbox-selection-color

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/select/listbox-overlay-scrollbar-expected.html
(right):

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/listbox-overlay-scrollbar-expected.html:35: <div
class="scrollbar vertical" style="left: 4px; top: 4px;"></div>
rtl should be an exact mirror of ltr so this is probably the correct result.

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/text/i...
File LayoutTests/fast/text/international/listbox-width-rtl.html (right):

https://codereview.chromium.org/347773002/diff/100001/LayoutTests/fast/text/i...
LayoutTests/fast/text/international/listbox-width-rtl.html:42: var
optionPaddingEnd = 2;
emptyWidth does not contain padding anymore, only border width and scrollbar
width.

https://codereview.chromium.org/347773002/diff/100001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/347773002/diff/100001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:897: return false;
select.innerText becomes "\n" unless we do this, it is "" right now.

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

https://codereview.chromium.org/347773002/diff/100001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:357: return insertionPoint->inDocument()
? InsertionShouldCallDidNotifySubtreeInsertions : InsertionDone;
We can't call ensureUserAgentShadow inside insertedInto because of
NoEventDispatchAssertion.

https://codereview.chromium.org/347773002/diff/100001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:368: if (select &&
!select->usesMenuList())
Only create shadow root when shown in a listbox.

https://codereview.chromium.org/347773002/diff/100001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/347773002/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:716: if (!isFinishedParsingChildren())
scrollToSelection is very slow compared to the current implementation because we
use element.scrollIntoViewIfNecessary which does layout.
menulist-appearance-basic.html timesout unless we do this.

https://codereview.chromium.org/347773002/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:897: void
HTMLSelectElement::optionRemoved(HTMLOptionElement* option)
listbox-clear-restore.html tests that the scroll position will not be preserved
when removing all options and adding them back.
Right now RenderListBox::layout is resetting the scroll position to zero when
all options are removed.
That is a hack. What we should be doing is adjust the active selection when
options are removed.

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

https://codereview.chromium.org/347773002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderListBox.cpp:107: LayoutUnit
RenderListBox::itemHeight() const
Firefox takes the height of the first list item and multiplies that. This tries
to do the same.

[keishi, 2014-06-23 20:42:01]

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:901: int listIndex =
optionToListIndex(option->index());
fast/forms/select-max-length.html was slow because this line causes
recalcListItems to be called for every option removed. Adding the if statement
before this made the test pass but it will still be slow for other websites.

[esprehn, 2014-06-23 21:59:57]

You should not be using ConfusingAndOftenMisusedDisallowShadowContent, you just
broke all <select>'s in Shadow DOM. Also I'm not sure why you're manually
calling detach() or ensureUserAgentShadowRoot() (and ignoring the return value)
so much, you shouldn't need to do that.

https://codereview.chromium.org/347773002/diff/160001/Source/core/css/html.css
File Source/core/css/html.css (right):

https://codereview.chromium.org/347773002/diff/160001/Source/core/css/html.cs...
Source/core/css/html.css:868:
select:-webkit-any([size]:not([size="0"]):not([size="1"]), [multiple]) {
This is not okay. We can't have selectors like this in the UA sheet.

https://codereview.chromium.org/347773002/diff/160001/Source/core/css/html.cs...
Source/core/css/html.css:909:
select:-webkit-any([size]:not([size="0"]):not([size="1"]),
[multiple]):not(:focus):not(:disabled) option:checked:not(:disabled) {
I don't think we should be checking in crazy selectors like this. What are you
trying to do?

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

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:42: class GroupLabelElement : public
HTMLDivElement {
Put this in it's own file.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:46: void setText(const String& text)
Not inline.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:51: m_textNode =
Text::create(document(), labelText);
You should just setTextContent, don't rewrite it yourself. Always call it.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:54:
m_textNode->replaceWholeText(labelText);
Use setTextContent.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:77: RefPtr<RenderStyle> originalStyle =
document().styleResolver()->styleForElement(this);
originalStyleForRenderer()

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:90: }
Why do you need this custom styling hook, I'm not sure we should be adding more
of these.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:139: return select &&
!select->usesMenuList() && !isDisplayNone();
This is not correct, you should be calling the super class version to deal with
display none stuff.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:215: return
labelRenderer->absoluteBoundingBoxRect().height();
This is wrong, you can't call int this when layout is dirty. This shouldn't be
in the DOM.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:234:
groupLabelElement->setText(String());
I don't think yo need a custom GroupLabelElement at all, just use a <div> here
and do setTextContent. You can set an id on it to reference it later if you need
to update it.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:236: m_groupLabelElement =
groupLabelElement.get();
Remove.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:251: return !style || style->display()
== NONE;
We should just remove this method....

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:257: return
insertionPoint->inDocument() ? InsertionShouldCallDidNotifySubtreeInsertions :
InsertionDone;
You shouldn't be using InsertionShouldCallDidNotifySubtreeInsertions here...

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:269: ensureUserAgentShadowRoot();
You always have a UA ShadowRoot, you call it in ::create(). Remove this.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:270: else if (userAgentShadowRoot() &&
document().lifecycle().stateAllowsDetach())
Using stateAllowsDetach() is wrong, don't do that. That's only for assertions
and to keep you from doing something bad, checking it like this means you're
doing something bad.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:271: userAgentShadowRoot()->detach();
You should not be calling detach like this.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptGroupElement.h (right):

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.h:71: RawPtrWillBeMember<HTMLDivElement>
m_groupLabelElement;
Why not just use getElementById in the Shadow DOM like other form controls?

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

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:291: return select &&
!select->usesMenuList() && !isDisplayNone();
ditto

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:370: else if (userAgentShadowRoot() &&
document().lifecycle().stateAllowsDetach())
ditto

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:431:
root->appendChild(Text::create(document(), textIndentedToRespectGroupLabel()),
ASSERT_NO_EXCEPTION);
ditto.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:346: // ensureShadowDOM needs to be run
even when !inDocument().
ensureUserAgentShadowRoot should be run by ::create()

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1099: else if (userAgentShadowRoot() &&
document().lifecycle().stateAllowsDetach())
This is wrong, you should not be manually interacting with detach() or
ensureUserAgentShadowRoot like this.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1114: lazyReattachIfAttached();
If you're going to reattach anyway, then there's no reason for the detach above
anyway.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1397: HitTestRequest
request(HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::Move
| HitTestRequest::ConfusingAndOftenMisusedDisallowShadowContent);
Why are you using HitTestRequest::ConfusingAndOftenMisusedDisallowShadowContent?
This is going to be broken if you put a <select> inside a SHadowRoot like
polymer does.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1399:
document().renderView()->hitTest(request, result);
What makes sure the layout/style is updated before you call this?

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1414: int
HTMLSelectElement::listIndexForNode(const Node& targetNode)
Can you scope this to a more specific type?

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1417: for (const Node* node =
&targetNode; node; node = node->parentOrShadowHostNode()) {
This walks all the way up to root of the document, why is that correct?

https://codereview.chromium.org/347773002/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderListBox.cpp (right):

https://codereview.chromium.org/347773002/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderListBox.cpp:120: return
optionRenderer->absoluteBoundingBoxRect().height();
I don't think this is the correct way to figure out height in the rendering
code.

https://codereview.chromium.org/347773002/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderListBox.cpp:123: return
toHTMLOptGroupElement(firstItem)->groupLabelHeight();
You shouldn't need to ask the DOM for this, you're in the rendering code, talk
to it's renderer.

[keishi, 2014-07-01 04:53:33]

> Also I'm not sure why you're manually
> calling detach() or ensureUserAgentShadowRoot() (and ignoring the return
value)
> so much, you shouldn't need to do that.

My idea was to only create the shadow root if in listbox mode and detach it in
menulist mode. I've changed it so we always create a UA shadow root in this new
patch.

https://codereview.chromium.org/347773002/diff/160001/Source/core/css/html.css
File Source/core/css/html.css (right):

https://codereview.chromium.org/347773002/diff/160001/Source/core/css/html.cs...
Source/core/css/html.css:868:
select:-webkit-any([size]:not([size="0"]):not([size="1"]), [multiple]) {
On 2014/06/23 21:59:55, esprehn wrote:
> This is not okay. We can't have selectors like this in the UA sheet.

Done.

https://codereview.chromium.org/347773002/diff/160001/Source/core/css/html.cs...
Source/core/css/html.css:909:
select:-webkit-any([size]:not([size="0"]):not([size="1"]),
[multiple]):not(:focus):not(:disabled) option:checked:not(:disabled) {
On 2014/06/23 21:59:55, esprehn wrote:
> I don't think we should be checking in crazy selectors like this. What are you
> trying to do?

Done.

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

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:42: class GroupLabelElement : public
HTMLDivElement {
On 2014/06/23 21:59:56, esprehn wrote:
> Put this in it's own file.

Removed this class.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:90: }
On 2014/06/23 21:59:56, esprehn wrote:
> Why do you need this custom styling hook, I'm not sure we should be adding
more
> of these.

Removed. The current implementation has a 1 pixel space between two lines so I
was trying to replicate that. We can just have 1px bottom padding on all rows.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:139: return select &&
!select->usesMenuList() && !isDisplayNone();
On 2014/06/23 21:59:55, esprehn wrote:
> This is not correct, you should be calling the super class version to deal
with
> display none stuff.

Done.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:234:
groupLabelElement->setText(String());
On 2014/06/23 21:59:55, esprehn wrote:
> I don't think yo need a custom GroupLabelElement at all, just use a <div> here
> and do setTextContent. You can set an id on it to reference it later if you
need
> to update it.

Done.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:236: m_groupLabelElement =
groupLabelElement.get();
On 2014/06/23 21:59:55, esprehn wrote:
> Remove.

Done.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:251: return !style || style->display()
== NONE;
On 2014/06/23 21:59:56, esprehn wrote:
> We should just remove this method....

Done.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:257: return
insertionPoint->inDocument() ? InsertionShouldCallDidNotifySubtreeInsertions :
InsertionDone;
On 2014/06/23 21:59:55, esprehn wrote:
> You shouldn't be using InsertionShouldCallDidNotifySubtreeInsertions here...

Done.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:269: ensureUserAgentShadowRoot();
On 2014/06/23 21:59:55, esprehn wrote:
> You always have a UA ShadowRoot, you call it in ::create(). Remove this.

Done.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptGroupElement.h (right):

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.h:71: RawPtrWillBeMember<HTMLDivElement>
m_groupLabelElement;
On 2014/06/23 21:59:56, esprehn wrote:
> Why not just use getElementById in the Shadow DOM like other form controls?

Done.

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

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:291: return select &&
!select->usesMenuList() && !isDisplayNone();
On 2014/06/23 21:59:56, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:370: else if (userAgentShadowRoot() &&
document().lifecycle().stateAllowsDetach())
On 2014/06/23 21:59:56, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:431:
root->appendChild(Text::create(document(), textIndentedToRespectGroupLabel()),
ASSERT_NO_EXCEPTION);
On 2014/06/23 21:59:56, esprehn wrote:
> ditto.

Done.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1397: HitTestRequest
request(HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::Move
| HitTestRequest::ConfusingAndOftenMisusedDisallowShadowContent);
On 2014/06/23 21:59:56, esprehn wrote:
> Why are you using
HitTestRequest::ConfusingAndOftenMisusedDisallowShadowContent?
> This is going to be broken if you put a <select> inside a SHadowRoot like
> polymer does.

I was able to remove this hit test.

https://codereview.chromium.org/347773002/diff/160001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1414: int
HTMLSelectElement::listIndexForNode(const Node& targetNode)
On 2014/06/23 21:59:56, esprehn wrote:
> Can you scope this to a more specific type?

Done.

https://codereview.chromium.org/347773002/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderListBox.cpp (right):

https://codereview.chromium.org/347773002/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderListBox.cpp:120: return
optionRenderer->absoluteBoundingBoxRect().height();
On 2014/06/23 21:59:57, esprehn wrote:
> I don't think this is the correct way to figure out height in the rendering
> code.

Tried RenderObject::height

https://codereview.chromium.org/347773002/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderListBox.cpp:123: return
toHTMLOptGroupElement(firstItem)->groupLabelHeight();
On 2014/06/23 21:59:57, esprehn wrote:
> You shouldn't need to ask the DOM for this, you're in the rendering code, talk
> to it's renderer.

I try to stay in the render tree in this next patch set.

[keishi, 2014-07-01 05:06:35]

Adding some explanation.

https://codereview.chromium.org/347773002/diff/180001/LayoutTests/fast/css/re...
File LayoutTests/fast/css/recalc-optgroup-inherit.html (left):

https://codereview.chromium.org/347773002/diff/180001/LayoutTests/fast/css/re...
LayoutTests/fast/css/recalc-optgroup-inherit.html:9: description("Check that
OPTION inherits the modified OPTGROUP color.");
We specify option font color in html.css so we no longer inherit font color from
the parent optgroup.

https://codereview.chromium.org/347773002/diff/180001/Source/core/css/resolve...
File Source/core/css/resolver/SharedStyleFinder.cpp (right):

https://codereview.chromium.org/347773002/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/SharedStyleFinder.cpp:244: if
(isHTMLOptionElement(candidate) && isHTMLOptionElement(element())
This makes fast/forms/select-initial-position.html pass.
If we don't have this the style for the selected option can be shared with the
previous sibling not selected option.

https://codereview.chromium.org/347773002/diff/180001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/347773002/diff/180001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:540: if (!usesMenuList() &&
!option.renderer())
I was able to get rid of isDisplayNone

[tkent, 2014-07-01 06:45:41]

Quick review.
I'll review more later.

https://codereview.chromium.org/347773002/diff/220001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/347773002/diff/220001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1115: crbug.com/234871
fast/forms/select/listbox-appearance-basic.html [ NeedsRebaseline ]
Would you add a PNG result for reference?

https://codereview.chromium.org/347773002/diff/220001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptGroupElement.cpp (right):

https://codereview.chromium.org/347773002/diff/220001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:112:
label->setShadowPseudoId(AtomicString("-webkit-optgroup-label",
AtomicString::ConstructFromLiteral));
We can't add non-standard pseudo ID.  Can we make this available only in UA
stylesheets?

[keishi, 2014-07-02 04:27:22]

https://codereview.chromium.org/347773002/diff/220001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/347773002/diff/220001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1115: crbug.com/234871
fast/forms/select/listbox-appearance-basic.html [ NeedsRebaseline ]
On 2014/07/01 06:45:41, tkent wrote:
> Would you add a PNG result for reference?

Done. Added test results for reference.

https://codereview.chromium.org/347773002/diff/220001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptGroupElement.cpp (right):

https://codereview.chromium.org/347773002/diff/220001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:112:
label->setShadowPseudoId(AtomicString("-webkit-optgroup-label",
AtomicString::ConstructFromLiteral));
On 2014/07/01 06:45:41, tkent wrote:
> We can't add non-standard pseudo ID.  Can we make this available only in UA
> stylesheets?
> 

I think it is hard to add pseudo ids that are only available for UA stylsheets
because UASheetMode is part of CSSParserContext and is only accessible while
parsing, not when we match selectors. I've used an inline style attribute to fix
this.

[keishi, 2014-07-03 09:42:44]

On 2014/07/02 04:27:22, keishi wrote:
>
https://codereview.chromium.org/347773002/diff/220001/LayoutTests/TestExpecta...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/347773002/diff/220001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:1115: crbug.com/234871
> fast/forms/select/listbox-appearance-basic.html [ NeedsRebaseline ]
> On 2014/07/01 06:45:41, tkent wrote:
> > Would you add a PNG result for reference?
> 
> Done. Added test results for reference.
> 
>
https://codereview.chromium.org/347773002/diff/220001/Source/core/html/HTMLOp...
> File Source/core/html/HTMLOptGroupElement.cpp (right):
> 
>
https://codereview.chromium.org/347773002/diff/220001/Source/core/html/HTMLOp...
> Source/core/html/HTMLOptGroupElement.cpp:112:
> label->setShadowPseudoId(AtomicString("-webkit-optgroup-label",
> AtomicString::ConstructFromLiteral));
> On 2014/07/01 06:45:41, tkent wrote:
> > We can't add non-standard pseudo ID.  Can we make this available only in UA
> > stylesheets?
> > 
> 
> I think it is hard to add pseudo ids that are only available for UA stylsheets
> because UASheetMode is part of CSSParserContext and is only accessible while
> parsing, not when we match selectors. I've used an inline style attribute to
fix
> this.

I added spatial navigation support using :-internal-spatial-navigation-focus
pseudo class.
Select element has overflow:scroll now so if I add a focus ring to option
elements, they get clipped to the select element rect and that looks bad.
I changed the style of the spatial navigation focused option to a black dashed
outline.
https://drive.google.com/file/d/0BwRi59l_ri74VHZCTGVHQll0aUU/edit?usp=sharing

[tkent, 2014-07-04 02:14:12]

https://codereview.chromium.org/347773002/diff/290001/Source/core/css/themeMa...
File Source/core/css/themeMac.css (right):

https://codereview.chromium.org/347773002/diff/290001/Source/core/css/themeMa...
Source/core/css/themeMac.css:14: background-color: white;
This is white, but background in the expectation PNGs is light gray?

[keishi, 2014-07-04 04:09:05]

https://codereview.chromium.org/347773002/diff/290001/Source/core/css/themeMa...
File Source/core/css/themeMac.css (right):

https://codereview.chromium.org/347773002/diff/290001/Source/core/css/themeMa...
Source/core/css/themeMac.css:14: background-color: white;
On 2014/07/04 02:14:12, tkent wrote:
> This is white, but background in the expectation PNGs is light gray?

PNGs weren't updated. Updated them now.

[tkent, 2014-07-04 04:39:32]

On 2014/07/04 04:09:05, keishi wrote:
>
https://codereview.chromium.org/347773002/diff/290001/Source/core/css/themeMa...
> File Source/core/css/themeMac.css (right):
> 
>
https://codereview.chromium.org/347773002/diff/290001/Source/core/css/themeMa...
> Source/core/css/themeMac.css:14: background-color: white;
> On 2014/07/04 02:14:12, tkent wrote:
> > This is white, but background in the expectation PNGs is light gray?
> 
> PNGs weren't updated. Updated them now.

Looks like -webkit-appearance:listbox is not used by default.  But I think it is
acceptable and we may remove -webkit-appearance:listbox.


I'll take a look at the C++ code change.

[keishi, 2014-07-04 04:44:42]

On 2014/07/04 04:39:32, tkent wrote:
> On 2014/07/04 04:09:05, keishi wrote:
> >
>
https://codereview.chromium.org/347773002/diff/290001/Source/core/css/themeMa...
> > File Source/core/css/themeMac.css (right):
> > 
> >
>
https://codereview.chromium.org/347773002/diff/290001/Source/core/css/themeMa...
> > Source/core/css/themeMac.css:14: background-color: white;
> > On 2014/07/04 02:14:12, tkent wrote:
> > > This is white, but background in the expectation PNGs is light gray?
> > 
> > PNGs weren't updated. Updated them now.
> 
> Looks like -webkit-appearance:listbox is not used by default.  But I think it
is
> acceptable and we may remove -webkit-appearance:listbox.
> 
> 
> I'll take a look at the C++ code change.

I was going to remove it but I found it useful to trigger the small scroll bar
on Mac.
RenderTheme::scrollbarControlSizeForPart

[tkent, 2014-07-08 02:21:00]

first round.

You may remove -expected.* from the CL.

https://codereview.chromium.org/347773002/diff/310001/Source/core/css/Selecto...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/css/Selecto...
Source/core/css/SelectorChecker.cpp:1132: if (!isHTMLOptionElement(element))
nit: you can write:
return isHTMLOptionElement(element) &&
toHTMLOptionElement(element).spatialNavigationFocused() &&
isFrameFocused(element);

https://codereview.chromium.org/347773002/diff/310001/Source/core/css/html.css
File Source/core/css/html.css (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/css/html.cs...
Source/core/css/html.css:645: outline: black dashed 1px;
four space indentation for consistency.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptGroupElement.cpp (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:43:
PassRefPtrWillBeRawPtr<HTMLOptGroupElement>
HTMLOptGroupElement::create(Document& document)
nit: We had better put the factory function definition after the constructor
definition to make inlining easier for compilers.

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

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:347: HTMLSelectElement* select =
Traversal<HTMLSelectElement>::firstAncestorOrSelf(*insertionPoint);
nit: You can write this as:
if (HTMLSelectElement* select =
Traversal<HTMLSelectElement>::firstAncestorOrSelf(*insertionPoint))
    select->optionRemoved(this);

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:349: return select->optionRemoved(this);
This is weird for void function.
Also, should we call HTMLElement::removedFrom()?

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:383: ShadowRoot* root =
userAgentShadowRoot();
nit: You can write this as:
if (ShadowRoot* root = userAgentShadowRoot())
    root->setTextContent(...);

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:380: RenderObject*
HTMLSelectElement::createRenderer(RenderStyle* style)
This change looks unnecessary.

https://codereview.chromium.org/347773002/diff/310001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (left):

https://codereview.chromium.org/347773002/diff/310001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:780: 
This change looks unnecessary.

[tkent, 2014-07-08 03:15:29]

On 2014/07/08 02:21:00, tkent wrote:
> You may remove -expected.* from the CL.

for pixel tests.

[tkent, 2014-07-08 03:22:43]

> Implement select listbox using shadow DOM 
>
> BUG=234871

Please write details to the CL description.  e.g.
- Any user-visible behavior changes
- This CL adds :-internal-spatial-navigation-focus, and the reason
- Why we can reduce AXListBox code

Remember WebKit-style ChangeLog.  It is helpful to review a huge change.

[tkent, 2014-07-08 04:43:46]

https://codereview.chromium.org/347773002/diff/310001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:905: if (isHTMLOptionElement(&node) ||
isHTMLOptGroupElement(&node))
&s are unnecessary.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptGroupElement.cpp (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:129: HTMLDivElement*
HTMLOptGroupElement::optGroupLabelElement() const
The return value can't be null.  We can make it HTMLDivElement&.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (left):

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:399: m_lastOnChangeSelection.clear();
Why do you remove this?

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1402: } else if (event->type() ==
EventTypeNames::mouseup && event->isMouseEvent() &&
toMouseEvent(event)->button() == LeftButton && renderer() &&
!toRenderBox(renderer())->autoscrollInProgress()) {
Is it safe to remove autoscrollInProgress check?

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:348: if (m_size != oldSize &&
document().isActive() && inDocument()) {
This change looks unnecessary.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:537: const HTMLOptionElement& option =
toHTMLOptionElement(*element);
This cast is unnecessary. isDisabledFormControl and renderer() are available in
Element.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:543: continue;
We prefer early continue.

if (!isHTMLOptionElement(*element))
    continue;

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:679: void
HTMLSelectElement::listBoxOnChange()
Is the change in this function necessary now?

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:906: void
HTMLSelectElement::optionRemoved(HTMLOptionElement* option)
probably the argument type should be |const HTMLOptionElement&|.

[keishi, 2014-07-10 09:48:04]

Even though accessibility tests were passing, there was a regression when used
with a screen reader so I fixed that.

https://codereview.chromium.org/347773002/diff/310001/Source/core/css/Selecto...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/css/Selecto...
Source/core/css/SelectorChecker.cpp:1132: if (!isHTMLOptionElement(element))
On 2014/07/08 02:20:59, tkent wrote:
> nit: you can write:
> return isHTMLOptionElement(element) &&
> toHTMLOptionElement(element).spatialNavigationFocused() &&
> isFrameFocused(element);

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/css/html.css
File Source/core/css/html.css (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/css/html.cs...
Source/core/css/html.css:645: outline: black dashed 1px;
On 2014/07/08 02:20:59, tkent wrote:
> four space indentation for consistency.

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:905: if (isHTMLOptionElement(&node) ||
isHTMLOptGroupElement(&node))
On 2014/07/08 04:43:45, tkent wrote:
> &s are unnecessary.

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptGroupElement.cpp (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:43:
PassRefPtrWillBeRawPtr<HTMLOptGroupElement>
HTMLOptGroupElement::create(Document& document)
On 2014/07/08 02:20:59, tkent wrote:
> nit: We had better put the factory function definition after the constructor
> definition to make inlining easier for compilers.

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:129: HTMLDivElement*
HTMLOptGroupElement::optGroupLabelElement() const
On 2014/07/08 04:43:45, tkent wrote:
> The return value can't be null.  We can make it HTMLDivElement&.

Done.

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

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:347: HTMLSelectElement* select =
Traversal<HTMLSelectElement>::firstAncestorOrSelf(*insertionPoint);
On 2014/07/08 02:20:59, tkent wrote:
> nit: You can write this as:
> if (HTMLSelectElement* select =
> Traversal<HTMLSelectElement>::firstAncestorOrSelf(*insertionPoint))
>     select->optionRemoved(this);

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:349: return select->optionRemoved(this);
On 2014/07/08 02:20:59, tkent wrote:
> This is weird for void function.
> Also, should we call HTMLElement::removedFrom()?

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptionElement.cpp:383: ShadowRoot* root =
userAgentShadowRoot();
On 2014/07/08 02:20:59, tkent wrote:
> nit: You can write this as:
> if (ShadowRoot* root = userAgentShadowRoot())
>     root->setTextContent(...);

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (left):

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:399: m_lastOnChangeSelection.clear();
On 2014/07/08 04:43:46, tkent wrote:
> Why do you remove this?

Reverted. See comment for HTMLSelectElement::listBoxOnChange.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1402: } else if (event->type() ==
EventTypeNames::mouseup && event->isMouseEvent() &&
toMouseEvent(event)->button() == LeftButton && renderer() &&
!toRenderBox(renderer())->autoscrollInProgress()) {
On 2014/07/08 04:43:46, tkent wrote:
> Is it safe to remove autoscrollInProgress check?

I added back this check and added a call to stopAutoscroll so it balances the
call to startAutoscrollForSelection I added above.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:348: if (m_size != oldSize &&
document().isActive() && inDocument()) {
On 2014/07/08 04:43:46, tkent wrote:
> This change looks unnecessary.

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:380: RenderObject*
HTMLSelectElement::createRenderer(RenderStyle* style)
On 2014/07/08 02:20:59, tkent wrote:
> This change looks unnecessary.

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:537: const HTMLOptionElement& option =
toHTMLOptionElement(*element);
On 2014/07/08 04:43:46, tkent wrote:
> This cast is unnecessary. isDisabledFormControl and renderer() are available
in
> Element.
> 

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:543: continue;
On 2014/07/08 04:43:46, tkent wrote:
> We prefer early continue.
> 
> if (!isHTMLOptionElement(*element))
>     continue;

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:679: void
HTMLSelectElement::listBoxOnChange()
On 2014/07/08 04:43:46, tkent wrote:
> Is the change in this function necessary now?

Because we animate scrolling now, RenderListBox::stopAutoscroll is called when
you click the scrollbar and extra change events fire in
listbox-click-on-scrollbar.html. I was trying to fix this by changing
listBoxOnChange to check more thoroughly if a selection change has happened.

I agree that this was wrong. In this new patch set I have created
handleMouseRelease() and made RenderListBox::stopAutoscroll use that.

RenderListBox::stopAutoscroll is called when dragging starts in the select
element by ends outside the select element.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:906: void
HTMLSelectElement::optionRemoved(HTMLOptionElement* option)
On 2014/07/08 04:43:46, tkent wrote:
> probably the argument type should be |const HTMLOptionElement&|.

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:906: void
HTMLSelectElement::optionRemoved(HTMLOptionElement* option)
On 2014/07/08 04:43:46, tkent wrote:
> probably the argument type should be |const HTMLOptionElement&|.

Done.

https://codereview.chromium.org/347773002/diff/310001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (left):

https://codereview.chromium.org/347773002/diff/310001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:780: 
On 2014/07/08 02:20:59, tkent wrote:
> This change looks unnecessary.

Done.

[tkent, 2014-07-11 00:35:33]

lgtm.

Probably the CL still have some regressions. But it's hard to find them in code
review.  Let's land this ASAP, get feedbacks, and fix issues.

https://codereview.chromium.org/347773002/diff/370001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/347773002/diff/370001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1281: 
I don't recommend adding entries at the bottom of TestExpectation.  It's
fragile.

[keishi, 2014-07-11 07:59:27]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-07-11 08:00:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/347773002/380001

[commit-bot: I haz the power, 2014-07-11 09:16:21]

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/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15033)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16412)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/31879)

[commit-bot: I haz the power, 2014-07-11 09:23:26]

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

[commit-bot: I haz the power, 2014-07-11 09:23:27]

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

[keishi, 2014-07-14 08:06:00]

The CQ bit was checked by keishi@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/347773002/400001

[commit-bot: I haz the power, 2014-07-14 09:12:16]

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/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15265)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16697)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/32552)

[commit-bot: I haz the power, 2014-07-14 09:22:41]

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

[commit-bot: I haz the power, 2014-07-14 09:22:43]

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

[keishi, 2014-07-16 14:05:34]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-07-16 14:05:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/347773002/540001

[commit-bot: I haz the power, 2014-07-16 15:00:59]

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/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15769)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17281)

[commit-bot: I haz the power, 2014-07-16 15:28:46]

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

[commit-bot: I haz the power, 2014-07-16 15:28:51]

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

[keishi, 2014-07-17 02:06:14]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-07-17 02:07:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/347773002/560001

[commit-bot: I haz the power, 2014-07-17 02:07:57]

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

[commit-bot: I haz the power, 2014-07-17 02:07:59]

Failed to apply patch for Source/core/rendering/RenderListBox.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderListBox.cpp
  Hunk #3 FAILED at 89.
  Hunk #4 FAILED at 268.
  2 out of 4 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderListBox.cpp.rej

Patch:       Source/core/rendering/RenderListBox.cpp
Index: Source/core/rendering/RenderListBox.cpp
diff --git a/Source/core/rendering/RenderListBox.cpp
b/Source/core/rendering/RenderListBox.cpp
index
f2b1b38d5871e199c48c012fba10ab5d2cdbfae9..50b49d35156e3334359ef3f31548a173ed4d1425
100644
--- a/Source/core/rendering/RenderListBox.cpp
+++ b/Source/core/rendering/RenderListBox.cpp
@@ -63,10 +63,6 @@ namespace WebCore {
 
 using namespace HTMLNames;
 
-const int rowSpacing = 1;
-
-const int optionsSpacingHorizontal = 2;
-
 // The minSize constant was originally defined to render scrollbars correctly.
 // This might vary for different platforms.
 const int minSize = 4;
@@ -74,40 +70,18 @@ const int minSize = 4;
 // Default size when the multiple attribute is present but size attribute is
absent.
 const int defaultSize = 4;
 
-// FIXME: This hardcoded baselineAdjustment is what we used to do for the old
-// widget, but I'm not sure this is right for the new control.
-const int baselineAdjustment = 7;
+const int defaultPaddingBottom = 1;
 
 RenderListBox::RenderListBox(Element* element)
     : RenderBlockFlow(element)
-    , m_optionsChanged(true)
-    , m_scrollToRevealSelectionAfterLayout(true)
-    , m_inAutoscroll(false)
-    , m_optionsWidth(0)
-    , m_indexOffset(0)
-    , m_listItemCount(0)
 {
     ASSERT(element);
     ASSERT(element->isHTMLElement());
     ASSERT(isHTMLSelectElement(element));
-
-    if (FrameView* frameView = frame()->view())
-        frameView->addScrollableArea(this);
 }
 
 RenderListBox::~RenderListBox()
 {
-    setHasVerticalScrollbar(false);
-
-    if (FrameView* frameView = frame()->view())
-        frameView->removeScrollableArea(this);
-}
-
-// FIXME: Instead of this hack we should add a ShadowRoot to <select> with no
insertion point
-// to prevent children from rendering.
-bool RenderListBox::isChildAllowed(RenderObject* object, RenderStyle*) const
-{
-    return object->isAnonymous() && !object->isRenderFullScreen();
 }
 
 inline HTMLSelectElement* RenderListBox::selectElement() const
@@ -115,177 +89,38 @@ inline HTMLSelectElement* RenderListBox::selectElement()
const
     return toHTMLSelectElement(node());
 }
 
-void RenderListBox::updateFromElement()
-{
-    FontCachePurgePreventer fontCachePurgePreventer;
-    if (m_optionsChanged) {
-        const WillBeHeapVector<RawPtrWillBeMember<HTMLElement> >& listItems =
selectElement()->listItems();
-        int size = static_cast<int>(listItems.size());
-
-        float width = 0;
-        m_listItemCount = 0;
-        for (int i = 0; i < size; ++i) {
-            const HTMLElement& element = *listItems[i];
-
-            String text;
-            Font itemFont = style()->font();
-            if (isHTMLOptionElement(element)) {
-                const HTMLOptionElement& optionElement =
toHTMLOptionElement(element);
-                if (optionElement.isDisplayNone())
-                    continue;
-                text = optionElement.textIndentedToRespectGroupLabel();
-                ++m_listItemCount;
-            } else if (isHTMLOptGroupElement(element)) {
-                if (toHTMLOptGroupElement(element).isDisplayNone())
-                    continue;
-                text = toHTMLOptGroupElement(element).groupLabelText();
-                FontDescription d = itemFont.fontDescription();
-                d.setWeight(d.bolderWeight());
-                itemFont = Font(d);
-                itemFont.update(document().styleEngine()->fontSelector());
-                ++m_listItemCount;
-            } else if (isHTMLHRElement(element)) {
-                // HTMLSelect adds it to its list, so we will also add it to
match the count.
-                ++m_listItemCount;
-                continue;
-            }
-
-            if (!text.isEmpty()) {
-                applyTextTransform(style(), text, ' ');
-
-                bool hasStrongDirectionality;
-                TextDirection direction = determineDirectionality(text,
hasStrongDirectionality);
-                TextRun textRun = constructTextRun(this, itemFont, text,
style(), TextRun::AllowTrailingExpansion);
-                if (hasStrongDirectionality)
-                    textRun.setDirection(direction);
-                float textWidth = itemFont.width(textRun);
-                width = std::max(width, textWidth);
-            }
-        }
-        m_optionsWidth = static_cast<int>(ceilf(width));
-        m_optionsChanged = false;
-
-        setHasVerticalScrollbar(true);
-
-        setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation();
-    }
-}
-
-void RenderListBox::selectionChanged()
-{
-    paintInvalidationForWholeRenderer();
-    if (!m_inAutoscroll) {
-        if (m_optionsChanged || needsLayout())
-            m_scrollToRevealSelectionAfterLayout = true;
-        else
-            scrollToRevealSelection();
-    }
-
-    if (AXObjectCache* cache = document().existingAXObjectCache())
-        cache->selectedChildrenChanged(this);
-}
-
-void RenderListBox::layout()
-{
-    RenderBlockFlow::layout();
-
-    if (m_vBar) {
-        bool enabled = numVisibleItems() < numItems();
-        m_vBar->setEnabled(enabled);
-        m_vBar->setProportion(numVisibleItems(), numItems());
-        if (!enabled) {
-            scrollToOffsetWithoutAnimation(VerticalScrollbar, 0);
-            m_indexOffset = 0;
-        }
-    }
-
-    if (m_scrollToRevealSelectionAfterLayout)
-        scrollToRevealSelection();
-}
-
-void RenderListBox::invalidateTreeAfterLayout(const PaintInvalidationState&
paintInvalidationState)
-{
-    repaintScrollbarIfNeeded();
-    RenderBox::invalidateTreeAfterLayout(paintInvalidationState);
-}
-
-void RenderListBox::scrollToRevealSelection()
-{
-    HTMLSelectElement* select = selectElement();
-
-    m_scrollToRevealSelectionAfterLayout = false;
-
-    int firstIndex =
listIndexToRenderListBoxIndex(select->activeSelectionStartListIndex());
-    int lastIndex =
listIndexToRenderListBoxIndex(select->activeSelectionEndListIndex());
-    if (firstIndex >= 0 && !listIndexIsVisible(lastIndex))
-        scrollToRevealElementAtListIndexInternal(firstIndex);
-}
-
-void RenderListBox::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth,
LayoutUnit& maxLogicalWidth) const
-{
-    maxLogicalWidth = m_optionsWidth + 2 * optionsSpacingHorizontal +
verticalScrollbarWidth();
-    if (!style()->width().isPercent())
-        minLogicalWidth = maxLogicalWidth;
-}
-
-void RenderListBox::computePreferredLogicalWidths()
-{
-    ASSERT(!m_optionsChanged);
-
-    m_minPreferredLogicalWidth = 0;
-    m_maxPreferredLogicalWidth = 0;
-    RenderStyle* styleToUse = style();
-
-    if (styleToUse->width().isFixed() && styleToUse->width().value() > 0)
-        m_minPreferredLogicalWidth = m_maxPreferredLogicalWidth =
adjustContentBoxLogicalWidthForBoxSizing(styleToUse->width().value());
-    else
-        computeIntrinsicLogicalWidths(m_minPreferredLogicalWidth,
m_maxPreferredLogicalWidth);
-
-    if (styleToUse->minWidth().isFixed() && styleToUse->minWidth().value() > 0)
{
-        m_maxPreferredLogicalWidth = std::max(m_maxPreferredLogicalWidth,
adjustContentBoxLogicalWidthForBoxSizing(styleToUse->minWidth().value()));
-        m_minPreferredLogicalWidth = std::max(m_minPreferredLogicalWidth,
adjustContentBoxLogicalWidthForBoxSizing(styleToUse->minWidth().value()));
-    }
-
-    if (styleToUse->maxWidth().isFixed()) {
-        m_maxPreferredLogicalWidth = std::min(m_maxPreferredLogicalWidth,
adjustContentBoxLogicalWidthForBoxSizing(styleToUse->maxWidth().value()));
-        m_minPreferredLogicalWidth = std::min(m_minPreferredLogicalWidth,
adjustContentBoxLogicalWidthForBoxSizing(styleToUse->maxWidth().value()));
-    }
-
-    LayoutUnit toAdd = borderAndPaddingWidth();
-    m_minPreferredLogicalWidth += toAdd;
-    m_maxPreferredLogicalWidth += toAdd;
-
-    clearPreferredLogicalWidthsDirty();
-}
-
 int RenderListBox::size() const
 {
     int specifiedSize = selectElement()->size();
     if (specifiedSize > 1)
-        return std::max(minSize, specifiedSize);
+        return max(minSize, specifiedSize);
 
     return defaultSize;
 }
 
-int RenderListBox::numVisibleItems() const
+LayoutUnit RenderListBox::defaultItemHeight() const
 {
-    // Only count fully visible rows. But don't return 0 even if only part of a
row shows.
-    return std::max<int>(1, (contentHeight() + rowSpacing) / itemHeight());
+    return style()->fontMetrics().height() + defaultPaddingBottom;
 }
 
-int RenderListBox::numItems() const
-{
-    return m_listItemCount;
-}
-
-LayoutUnit RenderListBox::listHeight() const
+LayoutUnit RenderListBox::itemHeight() const
 {
-    return itemHeight() * numItems() - rowSpacing;
+    HTMLSelectElement* select = selectElement();
+    if (!select)
+        return 0;
+    RenderObject* baseItemRenderer = firstChild();
+    if (!baseItemRenderer)
+        return defaultItemHeight();
+    if (baseItemRenderer->node() &&
isHTMLOptGroupElement(baseItemRenderer->node()))
+        baseItemRenderer = baseItemRenderer->slowFirstChild();
+    if (!baseItemRenderer || !baseItemRenderer->isBox())
+        return defaultItemHeight();
+    return toRenderBox(baseItemRenderer)->height();
 }
 
 void RenderListBox::computeLogicalHeight(LayoutUnit, LayoutUnit logicalTop,
LogicalExtentComputedValues& computedValues) const
 {
-    LayoutUnit height = itemHeight() * size() - rowSpacing;
+    LayoutUnit height = itemHeight() * size();
     // FIXME: The item height should have been added before updateLogicalHeight
was called to avoid this hack.
     updateIntrinsicContentLogicalHeight(height);
 
@@ -294,732 +129,12 @@ void RenderListBox:…
(message too large)

[keishi, 2014-07-17 04:35:26]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-07-17 04:36:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/347773002/580001

[commit-bot: I haz the power, 2014-07-17 05:48:58]

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

[commit-bot: I haz the power, 2014-07-17 06:37:42]

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

[commit-bot: I haz the power, 2014-07-17 06:37:46]

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

[keishi, 2014-07-17 07:26:04]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-07-17 07:26:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/347773002/600001

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

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

[keishi, 2014-07-17 08:51:32]

The CQ bit was checked by keishi@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/347773002/620001

[commit-bot: I haz the power, 2014-07-17 10:03:10]

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

[commit-bot: I haz the power, 2014-07-17 10:44:29]

Change committed as 178354

[johnme, 2014-07-17 13:28:44]

On 2014/07/17 10:44:29, I haz the power (commit-bot) wrote:
> Change committed as 178354

Oddly, this caused the following tests to fail:
- fast/forms/listbox-onchange.html
- fast/forms/select-clientheight-large-size.html
- fast/forms/select/listbox-overlay-scrollbar.html
- fast/text/international/listbox-width-rtl.html
- virtual/antialiasedtext/fast/text/international/listbox-width-rtl.html
And the following test to time out:
- fast/scroll-behavior/listbox-scrollTop.html
During this build:
-
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.7%20%28db...
-
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_7__...

However they passed on the following build, so I'm not sure what to make of
that; is it possible the bot was using the updated test expectations, but still
using the old code?

[tkent, 2014-07-17 23:36:53]

On 2014/07/17 13:28:44, johnme wrote:
> However they passed on the following build, so I'm not sure what to make of
> that; is it possible the bot was using the updated test expectations, but
still
> using the old code?

Yeah, it's possible if we have separated builder bot and tester bot.