The link should be activated by enter key on focusing the child element inside the anchor(relanding)

The link should be activated by enter key on focusing the child element inside the anchor.
The handing of Enter key has moved from keydown to keypress in anchor element.
And this makes behavior consistent with Firefox and IE.

BUG=350738, 425859
R=pdr@chromium.org
R=rob@robwu.nl
TEST=LayoutTests/fast/events/press-enter-key-on-focused-element-inside-link.html

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

[pdr., 2014-10-01 20:31:40]

pdr@chromium.org changed reviewers:
+ rob@robwu.nl

[pdr., 2014-10-01 20:32:37]

I've cc'e Rob Wu just as an FYI, as he was in this code recently as well.

https://codereview.chromium.org/619613005/diff/1/Source/core/html/HTMLAnchorE...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/619613005/diff/1/Source/core/html/HTMLAnchorE...
Source/core/html/HTMLAnchorElement.cpp:152: if ((focused() || (target->focused()
&& contains(target))) && isEnterKeyKeydownEvent(event)) {
This does not seem correct.

1) This will break existing content. For example, "<a
href="foo"><textarea></textarea></a>": if the user presses enter in the textarea
to create a new line, they will now incorrectly navigate.

2) Does it make sense for contains(target) to ever be false?

My feeling is that you're in the right area but I am not sure of the correct
solution here.

[robwu, 2014-10-01 21:25:54]

FYI: Even after fixing the bug that is associated with this CL, there will still
be a major inconsistency in focus handling between Chrome and Firefox/IE
(https://crbug.com/417636#c3).

https://codereview.chromium.org/619613005/diff/1/Source/core/html/HTMLAnchorE...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/619613005/diff/1/Source/core/html/HTMLAnchorE...
Source/core/html/HTMLAnchorElement.cpp:152: if ((focused() || (target->focused()
&& contains(target))) && isEnterKeyKeydownEvent(event)) {
On 2014/10/01 20:32:37, pdr wrote:
> This does not seem correct.
> 
> 1) This will break existing content. For example, "<a
> href="foo"><textarea></textarea></a>": if the user presses enter in the
textarea
> to create a new line, they will now incorrectly navigate.

Keyboard input events bubble, so I would expect that the textarea consumes the
Enter and doesn't send it to the parent <a> any more.

This default event handling is implemented in
EventDispatcher::dispatchEventPostProcess. I think that you have to check
whether the Node::willCallDefaultEventHandler(Event) implementations correctly
call Event::setDefaultHandled(true).

[pdr., 2014-10-01 21:43:40]

On 2014/10/01 at 21:25:54, rob wrote:
> On 2014/10/01 20:32:37, pdr wrote:
> > This does not seem correct.
> > 
> > 1) This will break existing content. For example, "<a
> > href="foo"><textarea></textarea></a>": if the user presses enter in the
textarea
> > to create a new line, they will now incorrectly navigate.
> 
> Keyboard input events bubble, so I would expect that the textarea consumes the
Enter and doesn't send it to the parent <a> any more.

I would expect so too but it doesn't seem to work that way:
http://jsfiddle.net/t07nzoxr

[Miyoung Shin(g), 2014-10-02 05:12:45]

On 2014/10/01 21:43:40, pdr wrote:
> On 2014/10/01 at 21:25:54, rob wrote:
> > On 2014/10/01 20:32:37, pdr wrote:
> > > This does not seem correct.
> > > 
> > > 1) This will break existing content. For example, "<a
> > > href="foo"><textarea></textarea></a>": if the user presses enter in the
> textarea
> > > to create a new line, they will now incorrectly navigate.
> > 
> > Keyboard input events bubble, so I would expect that the textarea consumes
the
> Enter and doesn't send it to the parent <a> any more.
> 
> I would expect so too but it doesn't seem to work that way:
> http://jsfiddle.net/t07nzoxr

Thank you for kindly your review. This is a good point. I found the reference
condition for form controls and elements having contentEditable in
HTMLElement::handleKeypressEvent.

void HTMLElement::handleKeypressEvent(KeyboardEvent* event)
    // if the element is a text form control (like <input type=text> or
<textarea>)
    // or has contentEditable attribute on, we should enter a space or newline
    // even in spatial navigation mode instead of handling it as a "click"
action.
    if (isTextFormControl() || isContentEditable())

I think I can give the same condition to HTMLAnchorElement::defaultEventHandle.


Regarding second comment, if I understand correctly, you have been wondering
about using contains().
The reason why I use contains() is to know if
HTMLAnchorElement::defaultEventHandler is called by handling event bubble.
But I can't get it form phaseType of event because phaseType value is set by
'NONE' from EventDispatcher::dispatchEventPostProcess.
So, I use contains() to detect if this event phaseType is 'bubble'


If you are ok with that, I will upload the patch to add the condition for form
controls and elements having contentEditable.

[pdr., 2014-10-03 00:35:11]

On 2014/10/02 at 05:12:45, myid.o.shin wrote:
> On 2014/10/01 21:43:40, pdr wrote:
> > On 2014/10/01 at 21:25:54, rob wrote:
> > > On 2014/10/01 20:32:37, pdr wrote:
> > > > This does not seem correct.
> > > > 
> > > > 1) This will break existing content. For example, "<a
> > > > href="foo"><textarea></textarea></a>": if the user presses enter in the
> > textarea
> > > > to create a new line, they will now incorrectly navigate.
> > > 
> > > Keyboard input events bubble, so I would expect that the textarea consumes
the
> > Enter and doesn't send it to the parent <a> any more.
> > 
> > I would expect so too but it doesn't seem to work that way:
> > http://jsfiddle.net/t07nzoxr
> 
> Thank you for kindly your review. This is a good point. I found the reference
condition for form controls and elements having contentEditable in
HTMLElement::handleKeypressEvent.
> 
> void HTMLElement::handleKeypressEvent(KeyboardEvent* event)
>     // if the element is a text form control (like <input type=text> or
<textarea>)
>     // or has contentEditable attribute on, we should enter a space or newline
>     // even in spatial navigation mode instead of handling it as a "click"
action.
>     if (isTextFormControl() || isContentEditable())
> 
> I think I can give the same condition to
HTMLAnchorElement::defaultEventHandle.
> 

That sounds reasonable to me.

> 
> Regarding second comment, if I understand correctly, you have been wondering
about using contains().
> The reason why I use contains() is to know if
HTMLAnchorElement::defaultEventHandler is called by handling event bubble.
> But I can't get it form phaseType of event because phaseType value is set by
'NONE' from EventDispatcher::dispatchEventPostProcess.
> So, I use contains() to detect if this event phaseType is 'bubble'

We should try to find a better way to do this without walking back down the
tree. I recommend looking for other instances in the code

> 
> 
> If you are ok with that, I will upload the patch to add the condition for form
controls and elements having contentEditable.

Due to the timezone differences, it may be fastest to just prepare patches.
Completely up to you though.

[Miyoung Shin(g), 2014-10-03 11:54:46]

1. I removed to use contains() at PatchSet2.
I realized I didn't need to use it because
HTMLAnchorElement::defaultEventHandler is called by handling only target and
bubbling event.
It means that event->target is always itself or its child element in
HTMLAnchorElement::defaultEventHandler.

2. I add the condition of isFormControlElement instead of isTextFormControl to
keep a test case[1] of layout
[1] fast/events/domactivate-sets-underlying-click-event-as-handled.html
Actually, the test case[1] showes the difffent result with FF.

when input is focused and enter key occurs, the behaviors are,

<a><input text></a> : FF - anchor is activated, Chrome : anchor isn't activated
<a><input submit></a> : FF - anchor isn't activated, Chrome : anchor isn't
activated
<a><input image></a> : FF - anchor isn't activated, Chrome : anchor isn't
activated
<a><button></a> : FF - anchor isn't activated, Chrome : anchor isn't activated
<a><input reset></a> : FF - anchor isn't activated, Chrome : anchor isn't
activated
<a><button reset></a> : FF - anchor isn't activated, Chrome : anchor isn't
activated
<a><input checkbox></a> : FF - anchor is activated, Chrome : anchor isn't
activated
<a><input radio></a> : FF - anchor is activated, Chrome : anchor isn't activated
<a><input file></a> : FF - anchor is activated, Chrome : anchor isn't activated

I'm not sure which one is right, I think we should check seperately this
situation.

[robwu, 2014-10-03 13:28:40]

On 2014/10/03 11:54:46, Miyoung Shin(g) wrote:
> I'm not sure which one is right, I think we should check seperately this
> situation.

Read the HTML5 specification and check the behavior of other major browser
engines such as Trident (Internet Explorer) and Presto (Opera 12) to see what
makes most sense.

https://codereview.chromium.org/619613005/diff/20001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/619613005/diff/20001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:378: if (target->focused() &&
!((target->isElementNode() && toElement(target)->isFormControlElement()) ||
target->isContentEditable()))
This logic is insufficient, because some form control elements can contain child
elements, and isFormControlElement() does not take the parent nodes into account
(isContentEditable() does look at the parent nodes though).

Here is an example:
<a> href="javascript:alert(0)">
  <button>
    Hello
    <b tabindex="1">
      world
    </b>
  </button>
</a>

Besides that, the Enter key should not be intercepted by every form control
element. For instance, in Firefox, the following code triggers an alert when
Enter is pressed:

<a href="javascript:alert('This is shown in Firefox')">
  <input type=text>
</a>

Track down all relevant defaultEventHandler calls (including non-form elements,
just in case), and check whether its defaultEventHandler implementation is
correct in terms of setting the eventHandled flag.
E.g. HTMLTextAreaElement::defaultEventHandler(Event* event) captures input
events using

    else if (renderer() && event->isBeforeTextInsertedEvent())
        handleBeforeTextInsertedEvent( ... event ... );

but does not call event->setDefaultHandled(true);
Consequently, the event is currently passed up in the DOM tree and ultimately
reaches the <a href> element, which causes a navigation to occur. This could be
fixed using

    else if (renderer() && event->isBeforeTextInsertedEvent()) {
        handleBeforeTextInsertedEvent( ... event ... );
        event->setDefaultHandled(true);
    }

[Miyoung Shin(g), 2014-10-06 06:31:38]

Thank you for your comment.

> Read the HTML5 specification
Sure, I read the specification of DOM Event 3, writing that,

=====================
default action
A default action is an optional supplementary behavior that an implementation
must perform in combination with the dispatch of the event object. Each event
type definition, and each specification, defines the default action for that
event type, if it has one. An instance of an event may have more than one
default action under some circumstances, such as when associated with an
activation trigger.

3.5 Activation triggers and behavior
Both HTML and SVG have an <a> element which indicates a link. Relevant
activation triggers for an <a> element are a click event on the text or image
content of the <a> element, or a keydown event with a key attribute value of
'Enter' key when the <a> element has focus.
=====================

I couldn't find to specify a case that <a> element's child has focus.
And I understand 'Enter' key treats with click event and click event's default
action is specified by "varies" in DOM spec.
It's not clear. I think this is the reason why each browsers has a different
behavior.

> and check the behavior of other major browser
> engines such as Trident (Internet Explorer) and Presto (Opera 12) to see what
> makes most sense.
As I know, Presto(Opera12) hasn't been released any more since Opera announced
to stop the development of Presto. 
I can't find the installation file for Presto from official Opera site. And I'm
not sure that I should refer to Presto.
BTW, I checked IE and they has a another behavior as well.
I mean that Chrome, FF, IE shows the different result. Please refer to next
comment.

> This logic is insufficient, because some form control elements can contain
child
> elements, and isFormControlElement() does not take the parent nodes into
account
> (isContentEditable() does look at the parent nodes though).
That's good point. I will check it again.

> Besides that, the Enter key should not be intercepted by every form control
> element. For instance, in Firefox, the following code triggers an alert when
> Enter is pressed:
> 
> <a href="javascript:alert('This is shown in Firefox')">
>   <input type=text>
> </a>
Please refer to next comment. 

> Track down all relevant defaultEventHandler calls (including non-form
elements,
> just in case), and check whether its defaultEventHandler implementation is
> correct in terms of setting the eventHandled flag.
> E.g. HTMLTextAreaElement::defaultEventHandler(Event* event) captures input
> events using
> 
>     else if (renderer() && event->isBeforeTextInsertedEvent())
>         handleBeforeTextInsertedEvent( ... event ... );
> 
> but does not call event->setDefaultHandled(true);
> Consequently, the event is currently passed up in the DOM tree and ultimately
> reaches the <a href> element, which causes a navigation to occur. This could
be
> fixed using
> 
>     else if (renderer() && event->isBeforeTextInsertedEvent()) {
>         handleBeforeTextInsertedEvent( ... event ... );
>         event->setDefaultHandled(true);
>     }
I'm not sure if we should add setDefaultHandled whenever input or textarea is
changed.
if we reach to follow totally the behavior of someone, it would be fixed.
Please let me know your opinion.

[Miyoung Shin(g), 2014-10-06 06:32:03]

Test case1.
<a href="javascript:alert('a')">
<form action="javascript:alert('form')">
<xxxxx onclick="alert('click')"> 
</form></a>

if xxxxx is one of input type or button, when enter key is pressed, the result
of event flow is shown like below.
xxxxx                | chrome    | FireFox     | IE 
----------------------------------------------------------
<input type=text>    |click,form |click,form,a |form  
<input type=submit>  |click,form |click,form   |click,form
<input type=image>   |click,form |click,form   |click,form
<button>             |click,form |click,form   |click
<input type=reset>   |click      |click        |click
<button type=reset>  |click      |click        |click
<input type=checkbox>|click,form |click,form,a |click,form
<input type=radio>   |click,form |click,form,a |click,form
<input type=file>    |click      |click,a      |click
<textarea>           |-          |-            |-
<div cotentediable>  |-          |a            |-

Test case2 - remove <form>
<a href="javascript:alert('a')">
<xxxxx onclick="alert('click')"> 
</a>

if xxxxx is one of input type or button, when enter key is pressed, the result
of event flow is shown like below.
xxxxx                | chrome | FireFox | IE 
---------------------------------------------
<input type=text>    |-       |a        |-  
<input type=submit>  |a       |a        |-
<input type=image>   |a       |a        |-
<button>             |a       |a        |-
<input type=reset>   |a       |a        |-
<button type=reset>  |a       |a        |-
<input type=checkbox>|-       |a        |-
<input type=radio>   |-       |a        |-
<input type=file>    |-       |a        |a
<textarea>           |-       |-        |-
<div cotentediable>  |-       |a        |-

[robwu, 2014-10-06 09:33:09]

It turns out that there is a reason for this inconsistency between browsers. The
markup that we have discussed is invalid.

https://html.spec.whatwg.org/multipage/semantics.html#the-a-element
"Content model: Transparent, but there must be no interactive content
descendant."

https://html.spec.whatwg.org/multipage/dom.html#interactive-content-2
"Interactive content is content that is specifically intended for user
interaction.

  => a, audio (if the controls attribute is present), button, details, embed,
iframe, img (if the usemap attribute is present), input (if the type attribute
is not in the Hidden state), keygen, label, object (if the usemap attribute is
present), select, textarea, sorting interface th elements, video (if the
controls attribute is present)

The tabindex attribute can also make any element into interactive content."

The last quoted line tells us that the "bug" that you are trying to fix is not
necessarily a bug, because the markup is invalid. I'm inclined to close the bug
that is associated with this CL. If you or pdr@ agrees with my assessment, then
you can also close this CL.

[Miyoung Shin(g), 2014-10-06 13:35:21]

pdr@, Could you give me your opinion?

[pdr., 2014-10-08 03:30:02]

On 2014/10/06 at 13:35:21, myid.o.shin wrote:
> pdr@, Could you give me your opinion?

Excellent analysis! Thank you for really looking into this.

I disagree with Rob. Specs exist to facilitate interoperability between
implementations and when there is disagreement we should question whether the
spec makes sense in the real world. Following the spec here would break real
sites. Given your findings of cross browser inconsistencies, matching Firefox,
IE, and Presto seems like the best option for our users (with a followup to
update the spec, of course). That said, fixing this sounds like it will require
an involved rethinking of how events and form controls interact in blink which
may not be the best project for a newer engineer on the project :/ Do you think
we could fix some of the cross-browser inconsistencies you found without
rewriting everything?

FYI: I found some of your analysis in test 1 and test 2 to be incorrect, at
least on OSX. There may be platform inconsistencies here where
OSX/linux/android/win behavior differs.

[Miyoung Shin(g), 2014-10-08 05:19:12]

> Do you think
> we could fix some of the cross-browser inconsistencies you found without
> rewriting everything?

Sorry, What does 'rewriting everything' mean ?

> FYI: I found some of your analysis in test 1 and test 2 to be incorrect, at
> least on OSX. There may be platform inconsistencies here where
> OSX/linux/android/win behavior differs.

Oh... actually I tested it on Window OS. currently I don't have android
environment or device but I can try to check them on OSX/linux/win.
And I will remain each browser version for the next time.

[pdr., 2014-10-08 06:28:58]

On 2014/10/08 at 05:19:12, myid.o.shin wrote:
> > Do you think
> > we could fix some of the cross-browser inconsistencies you found without
> > rewriting everything?
> 
> Sorry, What does 'rewriting everything' mean ?

I was just referring to how much complexity this patch will involve. Rob pointed
out that you'll need to check all 100+ defaultEventHandler calls, and the
cross-platform issues, for example. Updating this patch may be as simple as
taking Rob's suggestions, I just don't know.

> 
> > FYI: I found some of your analysis in test 1 and test 2 to be incorrect, at
> > least on OSX. There may be platform inconsistencies here where
> > OSX/linux/android/win behavior differs.
> 
> Oh... actually I tested it on Window OS. currently I don't have android
environment or device but I can try to check them on OSX/linux/win.
> And I will remain each browser version for the next time.

You don't have to worry about testing everything on other platforms quite yet,
as we can catch those with tests :) It would be good to double-check a few on
one other platform though, just as a sanity check. For example,
http://jsfiddle.net/do5rv9fx/ (your first of test case 2) is the same for me in
firefox and chromium.

[robwu, 2014-10-08 08:46:19]

On 2014/10/08 03:30:02, pdr wrote:
> I disagree with Rob. Specs exist to facilitate interoperability between
> implementations and when there is disagreement we should question whether the
> spec makes sense in the real world. Following the spec here would break real
> sites. Given your findings of cross browser inconsistencies, matching Firefox,
> IE, and Presto seems like the best option for our users (with a followup to
> update the spec, of course). That said, fixing this sounds like it will
require
> an involved rethinking of how events and form controls interact in blink which
> may not be the best project for a newer engineer on the project :/ Do you
think
> we could fix some of the cross-browser inconsistencies you found without
> rewriting everything?

Firefox and IE already vary wildly in behavior. Chrome's current behavior
(form+link) matches IE's, changing it to match Firefox's behavior does not
improve the situation at all. Besides, the discussed markup is not only invalid,
but also illogical. Think for a second what it means to have a button in a form
in a link.

In the form control-in-link test cases, the only consistent result appeared at
<input type=file> and <textarea>. Chrome's current behavior already matches the
textarea test, but not the <input type=file>.

Considering that there is no standard or logic in the handling of Enter in this
markup, I recommend to focus this CL on:
1. Implementing the new behavior for <a><xxx tabindex> (bug 350738).
2. Not change the behavior for <a><xxx> where xxx is a form control element (in
particular, <textarea>).

This task is relatively easy compared to the monstrous task of getting
interoperability in a situation where the reference implementations already fail
to be interoperable among each other ;)

On 2014/10/08 06:28:58, pdr wrote:
> > > FYI: I found some of your analysis in test 1 and test 2 to be incorrect,
at
> > > least on OSX. There may be platform inconsistencies here where
> > > OSX/linux/android/win behavior differs.
> > 
> > Oh... actually I tested it on Window OS. currently I don't have android
> environment or device but I can try to check them on OSX/linux/win.
> > And I will remain each browser version for the next time.
> 
> You don't have to worry about testing everything on other platforms quite yet,
> as we can catch those with tests :) It would be good to double-check a few on
> one other platform though, just as a sanity check. For example,
> http://jsfiddle.net/do5rv9fx/ (your first of test case 2) is the same for me
in
> firefox and chromium.

In Firefox 32 (Linux), a dialog shows up upon hitting ENTER after focusing the
text field.
In Chromium 40.0.2179.0 (Linux), nothing happens.
In Safari 8 and Chrome 39 (Mac, via browserstack.com), nothing happens either.
Did you run the test in the same way as we did?

[Miyoung Shin(g), 2014-10-09 15:37:56]

> 1. Implementing the new behavior for <a><xxx tabindex> (bug 350738).
> 2. Not change the behavior for <a><xxx> where xxx is a form control element
(in
> particular, <textarea>).

I agree with Rob's suggestion. if Pdr also agrees with it, I'd like to change
bug 350738's status back.

> Did you run the test in the same way as we did?

I also retested it on Firefox32(Ubuntu), but I couldn't see the alert after
focusing the text field by tab key.
Even though I pressed "enter" key during cursor is blinking, I couldn't see it,
just line feed.
But if I click the input area for focusing the text field, I can see the alert.
I don't know why the behavior is different from yours.

http://jsfiddle.net/do5rv9fx/
<a href="javascript:alert('a')">
    <input type="text" onclick="alert('click')"></input>
    <textarea> aaa </textarea>
</a>

[Miyoung Shin(g), 2014-10-13 06:05:18]

I've uploaded the patch.

My major change is to replace keydown event to keypress event for click event of
anchor element not to change the behavior for <a><form><xxx> where xxx is a form
control element.
Before my patch, form control element and form action are working based on
keypress event but anchor element is working based on keydown event.
And, as I checked on FireFox, onclick event occurs when keypress event is fired.
In case of IE10, I couldn't test it because onclick event doesn't occur when key
event is created by JS. I don't know why.
I think it's better to handle keypress event for click event considering the
event flow of form control element.

Additionally, I need to check anchor element in SVG whether there is this origin
problem in SVG as well,
But I hope to investigate it out of this patch.

[robwu, 2014-10-13 09:19:48]

We're getting close! Could you add a <textarea> test case to avoid regressions,
and a test to cover SVG <a> elements?

And also add a note to the CL description that highlights that the event is now
handled on keypress instead of keydown. (I've confirmed your observation that
Enter is handled at keypress in Firefox - http://jsfiddle.net/1es7z4ft/)

https://codereview.chromium.org/619613005/diff/40001/LayoutTests/fast/events/...
File
LayoutTests/fast/events/keydown-enter-key-on-focused-element-inside-link.html
(right):

https://codereview.chromium.org/619613005/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/keydown-enter-key-on-focused-element-inside-link.html:1:
<!DOCTYPE html>
This file should be renamed to "keypress-enter....", or (more genericly and
implementation-independent) "press-enter-...".

https://codereview.chromium.org/619613005/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/keydown-enter-key-on-focused-element-inside-link.html:16:
document.activeElement.dispatchEvent(event);
You should also be simulating the keydown/keyup events to model a realistic
situation.

Just use eventSender.keyDown('\r'); - this will simulate a keydown, keypress and
keyup event.

https://codereview.chromium.org/619613005/diff/40001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (left):

https://codereview.chromium.org/619613005/diff/40001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:360: bool isEnterKeyKeydownEvent(Event*
event)
And remove this method declaration (and its defenition in the .h file), since it
is no longer used after this CL.

https://codereview.chromium.org/619613005/diff/40001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/619613005/diff/40001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:154: if ((focused() || target->focused())
&& isEnterKeyKeypressEvent(event)) {
Don't forget to update similar code in
src/third_party/WebKit/Source/core/svg/SVGAElement.cpp.

[Miyoung Shin(g), 2014-10-13 11:05:01]

https://codereview.chromium.org/619613005/diff/40001/LayoutTests/fast/events/...
File
LayoutTests/fast/events/keydown-enter-key-on-focused-element-inside-link.html
(right):

https://codereview.chromium.org/619613005/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/keydown-enter-key-on-focused-element-inside-link.html:1:
<!DOCTYPE html>
On 2014/10/13 09:19:48, robwu wrote:
> This file should be renamed to "keypress-enter....", or (more genericly and
> implementation-independent) "press-enter-...".

Oh, right, I will change it

https://codereview.chromium.org/619613005/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/keydown-enter-key-on-focused-element-inside-link.html:16:
document.activeElement.dispatchEvent(event);
On 2014/10/13 09:19:48, robwu wrote:
> You should also be simulating the keydown/keyup events to model a realistic
> situation.

OK. I will add keydown/keyup as well

> Just use eventSender.keyDown('\r'); - this will simulate a keydown, keypress
and
> keyup event.

I couldn't, because keypress event doesn't get keycode when I use
eventSender.keyDown('\r');,
because there is the code to reset the keycode of '\r'in implemenation of
eventSender.keyDown()

https://codereview.chromium.org/619613005/diff/40001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (left):

https://codereview.chromium.org/619613005/diff/40001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:360: bool isEnterKeyKeydownEvent(Event*
event)
On 2014/10/13 09:19:48, robwu wrote:
> And remove this method declaration (and its defenition in the .h file), since
it
> is no longer used after this CL.

Sure, I remember it as well.
After checking SVG is done, I will get rid of it.

https://codereview.chromium.org/619613005/diff/40001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/619613005/diff/40001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:154: if ((focused() || target->focused())
&& isEnterKeyKeypressEvent(event)) {
On 2014/10/13 09:19:48, robwu wrote:
> Don't forget to update similar code in
> src/third_party/WebKit/Source/core/svg/SVGAElement.cpp.

Sure, I remember it.
But I mentioned earlier, I hope to separate the patch with HTML and SVG because
I need to understand SVG first.
Can I do it?

[robwu, 2014-10-13 11:29:33]

https://codereview.chromium.org/619613005/diff/40001/LayoutTests/fast/events/...
File
LayoutTests/fast/events/keydown-enter-key-on-focused-element-inside-link.html
(right):

https://codereview.chromium.org/619613005/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/keydown-enter-key-on-focused-element-inside-link.html:16:
document.activeElement.dispatchEvent(event);
On 2014/10/13 11:05:01, Miyoung Shin(g) wrote:
> On 2014/10/13 09:19:48, robwu wrote:
> > You should also be simulating the keydown/keyup events to model a realistic
> > situation.
> 
> OK. I will add keydown/keyup as well
> 
> > Just use eventSender.keyDown('\r'); - this will simulate a keydown, keypress
> and
> > keyup event.
> 
> I couldn't, because keypress event doesn't get keycode when I use
> eventSender.keyDown('\r');,
> because there is the code to reset the keycode of '\r'in implemenation of
> eventSender.keyDown()

The test code in the following files suggest that Enter+keydown/keypress
(implying Enter) is correctly supported (on buttons)
src/third_party/WebKit/LayoutTests/fast/events/key-events-in-input-button.html
src/third_party/WebKit/LayoutTests/fast/events/key-events-in-input-button-expected.txt

https://codereview.chromium.org/619613005/diff/40001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/619613005/diff/40001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.cpp:154: if ((focused() || target->focused())
&& isEnterKeyKeypressEvent(event)) {
On 2014/10/13 11:05:01, Miyoung Shin(g) wrote:
> On 2014/10/13 09:19:48, robwu wrote:
> > Don't forget to update similar code in
> > src/third_party/WebKit/Source/core/svg/SVGAElement.cpp.
> 
> Sure, I remember it.
> But I mentioned earlier, I hope to separate the patch with HTML and SVG
because
> I need to understand SVG first.
> Can I do it?

There is some duplicate logic in the implementations of SVGAElement and
HTMLAnchorElement. It makes sense to update both implementations at once, since
it's usually a matter of copy-paste (plus a few changes, occasionally).

[Miyoung Shin(g), 2014-10-13 15:55:59]

> Could you add a <textarea> test case to avoid regressions?

I can't create keypress event to use initKeyboardEvent for <textarea> because
initKeyboardEvent doesn't support keycode.
if there isn't keycode in the key event, <textarea> doesn't insert any key
value.
For entering the some key event in <textarea>, I should use eventSender.keyDown
instead of initKeyboardEvent.

> There is some duplicate logic in the implementations of SVGAElement and
> HTMLAnchorElement. It makes sense to update both implementations at once,
since
> it's usually a matter of copy-paste (plus a few changes, occasionally).wa

OK, I got it. I've updated the patch considering SVGAElement.

[robwu, 2014-10-13 21:57:04]

https://codereview.chromium.org/619613005/diff/210001/LayoutTests/fast/events...
File LayoutTests/fast/events/press-enter-key-on-focused-element-inside-link.html
(right):

https://codereview.chromium.org/619613005/diff/210001/LayoutTests/fast/events...
LayoutTests/fast/events/press-enter-key-on-focused-element-inside-link.html:13:
<svg width="200" height="50" onclick="debug(eventInfo(event))">>
Remove the trailing >.

https://codereview.chromium.org/619613005/diff/210001/LayoutTests/fast/events...
LayoutTests/fast/events/press-enter-key-on-focused-element-inside-link.html:25:
function test(eventType) {
Next time, please choose a more descriptive name than "test" ;)

I have submitted a patch that allows keypress to report "Enter" for the
keyIdentifier (https://codereview.chromium.org/649343002/), so you can now
remove this entire function, and use eventSender.keyDown("\n"); to generate
Enter keydown/press/up.

https://codereview.chromium.org/619613005/diff/210001/LayoutTests/fast/events...
LayoutTests/fast/events/press-enter-key-on-focused-element-inside-link.html:31:
target = event.srcElement ? event.srcElement : event.target;
`srcElement` is a non-standard property and only needed in ancient IE browsers.
Get rid of this line and use "event.target" instead.

https://codereview.chromium.org/619613005/diff/210001/LayoutTests/fast/events...
LayoutTests/fast/events/press-enter-key-on-focused-element-inside-link.html:32:
if (event.type == "keydown" || event.type == "keypress" || event.type ==
"keyup")
Nit: Try to be consistent with quotes. Either use single quotes or double
quotes, but don't use then interchangeably.

[Miyoung Shin(g), 2014-10-14 05:11:22]

Rob, thank you for your kindly review.
I've upload the patch to apply your comment.
This patch should be landed after the patch that you submitted at
https://codereview.chromium.org/649343002/ lands

[robwu, 2014-10-14 09:27:24]

Looks good to me.

Star https://crbug.com/423270 to be notified when the event_sender bug is fixed.
This takes 3 patches. First a patch to disable the tests, then the actual patch
and finally a patch to re-enable the tests; your CL can be landed after the
second patch.

https://codereview.chromium.org/619613005/diff/300001/LayoutTests/fast/events...
File LayoutTests/fast/events/press-enter-key-on-focused-element-inside-link.html
(right):

https://codereview.chromium.org/619613005/diff/300001/LayoutTests/fast/events...
LayoutTests/fast/events/press-enter-key-on-focused-element-inside-link.html:25:
target = event.target;
By "use event.target instead", I meant using "event.target" in the places below.

If you prefer to use a separate variable, declare the variable by putting "var"
in front of it.

[Miyoung Shin(g), 2014-10-14 11:50:18]

> Star https://crbug.com/423270 to be notified when the event_sender bug is
fixed.
> This takes 3 patches. First a patch to disable the tests, then the actual
patch
> and finally a patch to re-enable the tests; your CL can be landed after the
> second patch.

OK. I got it. I added to disable the test case.

> By "use event.target instead", I meant using "event.target" in the places
below.

OK. I changed the test case using event.target.

[pdr., 2014-10-20 17:28:34]

On 2014/10/14 at 11:50:18, myid.o.shin wrote:
> > Star https://crbug.com/423270 to be notified when the event_sender bug is
fixed.
> > This takes 3 patches. First a patch to disable the tests, then the actual
patch
> > and finally a patch to re-enable the tests; your CL can be landed after the
> > second patch.
> 
> OK. I got it. I added to disable the test case.
> 
> > By "use event.target instead", I meant using "event.target" in the places
below.
> 
> OK. I changed the test case using event.target.

LGTM, thanks for working through this.

[pdr., 2014-10-20 17:28:40]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-10-20 17:29:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/619613005/320001

[commit-bot: I haz the power, 2014-10-20 17:29:59]

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

[commit-bot: I haz the power, 2014-10-20 17:30:00]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1457.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
3507dad12e743ff12f7619beee12db2a44a0a89f..e9a60b23b121d6386095bde8bc7d9db22365c40b
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1457,3 +1457,6 @@ crbug.com/422278 [ XP ]
http/tests/serviceworker/fetch-canvas-tainting.html [ Pa
 crbug.com/422278 [ XP ] http/tests/serviceworker/fetch-cors-xhr.html [ Pass
Timeout ]
 
 crbug.com/388375 http/tests/serviceworker/fetch-event.html [ NeedsRebaseline ]
+
+# Temporarily skipped until event_sender is changed not to set the
keyIdentifier into an empty string
+crbug.com/423270
fast/events/press-enter-key-on-focused-element-inside-link.html [ Pass Failure ]
\ No newline at end of file

[robwu, 2014-10-20 19:05:21]

https://codereview.chromium.org/619613005/diff/320001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/619613005/diff/320001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1462: crbug.com/423270
fast/events/press-enter-key-on-focused-element-inside-link.html [ Pass Failure ]
Put these three lines at some random place in the TestExpectations file to avoid
errors in applying the patch. See
http://www.chromium.org/developers/testing/webkit-layout-tests/testexpectatio...
for guidelines on writing TestExpectations.

[Miyoung Shin(g), 2014-10-21 03:37:41]

>
https://codereview.chromium.org/619613005/diff/320001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:1462: crbug.com/423270
> fast/events/press-enter-key-on-focused-element-inside-link.html [ Pass Failure
]
> Put these three lines at some random place in the TestExpectations file to
avoid
> errors in applying the patch. See
>
http://www.chromium.org/developers/testing/webkit-layout-tests/testexpectatio...
> for guidelines on writing TestExpectations.

Thank you for your kind description.
I put my change into a random spot referred to "Hint: Put new changes into a
random spot in the file to reduce the chance of merge conflicts when landing
your patch.",

[pdr., 2014-10-21 03:46:26]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-10-21 03:46:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/619613005/340001

[commit-bot: I haz the power, 2014-10-21 03:50:41]

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

[commit-bot: I haz the power, 2014-10-21 03:50:43]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
  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...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/27840)

[Miyoung Shin(g), 2014-10-21 06:37:45]

Patchset #8 (id:360001) has been deleted

[Miyoung Shin(g), 2014-10-21 06:41:51]

The CQ bit was checked by myid.o.shin@gmail.com

[commit-bot: I haz the power, 2014-10-21 06:42:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/619613005/380001

[commit-bot: I haz the power, 2014-10-21 07:44:23]

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

[commit-bot: I haz the power, 2014-10-21 07:44:25]

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

[Miyoung Shin(g), 2014-10-21 08:57:02]

I've added fragment-activation-focuses-target.html in TestExpectations
The reason is the same problem with eventSender.keyDown("\r");

[robwu, 2014-10-21 09:12:53]

On 2014/10/21 08:57:02, Miyoung Shin(g) wrote:
> I've added fragment-activation-focuses-target.html in TestExpectations
> The reason is the same problem with eventSender.keyDown("\r");

Coincidentally, I have written that test, and seeing this test failure indicates
that your patch breaks something.
When you press Enter on an anchor (<a href="#some-existing-target">click
here</a><input id="some-existing-target">), then the target should be focused.
This seems not to happen after your patch any more. My event_sender.cc only
changes the value of keyIdentifier for keypress events, it does not change the
behavior of dispatched events.

Your patch moves the handling of the default event from keydown to keypress,
check whether there is any keydown/keypress logic that you've overlooked.

[Miyoung Shin(g), 2014-10-21 09:50:54]

On 2014/10/21 09:12:53, robwu wrote:
> On 2014/10/21 08:57:02, Miyoung Shin(g) wrote:
> > I've added fragment-activation-focuses-target.html in TestExpectations
> > The reason is the same problem with eventSender.keyDown("\r");
> 
> Coincidentally, I have written that test, and seeing this test failure
indicates
> that your patch breaks something.
> When you press Enter on an anchor (<a href="#some-existing-target">click
> here</a><input id="some-existing-target">), then the target should be focused.
> This seems not to happen after your patch any more. My event_sender.cc only
> changes the value of keyIdentifier for keypress events, it does not change the
> behavior of dispatched events.
> 
> Your patch moves the handling of the default event from keydown to keypress,
> check whether there is any keydown/keypress logic that you've overlooked.

Sorry, I can't reproduce the problem you said on my patch
I used like,
<a href="#some-existing-target">click here</a><input id="some-existing-target">

When I press Enter on an anchor, I can see input element(=target) is focused.
Could you give me the sample page to be clear more?

[robwu, 2014-10-21 09:58:30]

On 2014/10/21 09:50:54, Miyoung Shin(g) wrote:
> On 2014/10/21 09:12:53, robwu wrote:
> > On 2014/10/21 08:57:02, Miyoung Shin(g) wrote:
> > > I've added fragment-activation-focuses-target.html in TestExpectations
> > > The reason is the same problem with eventSender.keyDown("\r");
> > 
> > Coincidentally, I have written that test, and seeing this test failure
> indicates
> > that your patch breaks something.
> > When you press Enter on an anchor (<a href="#some-existing-target">click
> > here</a><input id="some-existing-target">), then the target should be
focused.
> > This seems not to happen after your patch any more. My event_sender.cc only
> > changes the value of keyIdentifier for keypress events, it does not change
the
> > behavior of dispatched events.
> > 
> > Your patch moves the handling of the default event from keydown to keypress,
> > check whether there is any keydown/keypress logic that you've overlooked.
> 
> Sorry, I can't reproduce the problem you said on my patch
> I used like,
> <a href="#some-existing-target">click here</a><input
id="some-existing-target">
> 
> When I press Enter on an anchor, I can see input element(=target) is focused.
> Could you give me the sample page to be clear more?

src/third_party/WebKit/LayoutTests/fast/dom/fragment-activation-focuses-target.html
The failing part is

<a href="#fragment1" id="link1" tabindex="0">link1</a>
<div id="fragment1" name="fragment1" tabindex="0">fragment1</div>

Focus the anchor, then press Enter. The <div> should be focused.

[Miyoung Shin(g), 2014-10-21 10:12:14]

>
src/third_party/WebKit/LayoutTests/fast/dom/fragment-activation-focuses-target.html
> The failing part is
> 
> <a href="#fragment1" id="link1" tabindex="0">link1</a>
> <div id="fragment1" name="fragment1" tabindex="0">fragment1</div>
> 
> Focus the anchor, then press Enter. The <div> should be focused.

When I test manually it, it works fine even if my patch is applied.

Anyway, if I change eventSender.keyDown("\r"); to 

function dispatchKeyEvent(eventType, keyIdentifier) {
  var event = document.createEvent('KeyboardEvents');
  event.initKeyboardEvent(eventType, true, true, window, keyIdentifier, 0, 0, 0,
0, 0, false);
  document.activeElement.dispatchEvent(event);
}
dispatchKeyEvent('keydown', 'Enter');
dispatchKeyEvent('keypress', 'Enter');
dispatchKeyEvent('keyup', 'Enter');

then the expected result is shown.
This is why I added this TC in TestExpectations.

[robwu, 2014-10-21 10:18:55]

On 2014/10/21 10:12:14, Miyoung Shin(g) wrote:
> >
>
src/third_party/WebKit/LayoutTests/fast/dom/fragment-activation-focuses-target.html
> > The failing part is
> > 
> > <a href="#fragment1" id="link1" tabindex="0">link1</a>
> > <div id="fragment1" name="fragment1" tabindex="0">fragment1</div>
> > 
> > Focus the anchor, then press Enter. The <div> should be focused.
> 
> When I test manually it, it works fine even if my patch is applied.
> 
> Anyway, if I change eventSender.keyDown("\r"); to 
> 
> function dispatchKeyEvent(eventType, keyIdentifier) {
>   var event = document.createEvent('KeyboardEvents');
>   event.initKeyboardEvent(eventType, true, true, window, keyIdentifier, 0, 0,
0,
> 0, 0, false);
>   document.activeElement.dispatchEvent(event);
> }
> dispatchKeyEvent('keydown', 'Enter');
> dispatchKeyEvent('keypress', 'Enter');
> dispatchKeyEvent('keyup', 'Enter');
> 
> then the expected result is shown.
> This is why I added this TC in TestExpectations.

This will then indeed be fixed by my event_sender.cc patch.
lgtm

[Miyoung Shin(g), 2014-10-21 10:21:08]

The CQ bit was checked by myid.o.shin@gmail.com

[commit-bot: I haz the power, 2014-10-21 10:22:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/619613005/360002

[commit-bot: I haz the power, 2014-10-21 11:38:22]

Committed patchset #9 (id:360002) as 184074

[Miyoung Shin(g), 2014-10-22 08:48:37]

I got a mail that sheriff reverted this patch.
I will investigate it.

Reviewers: Miyoung Shin(g),

Description:
Revert 184074 "The link shoud be activated by enter key on focus..."

This caused interactive_ui_tests WebViewInteractiveTest.NewWindow_OpenInNewTab
always fail on many bots (Win7 dbg, Mac 10.6/10.8, Linux, Chrome OS) and
prevented
Blink CLs to roll to chromium.

Please investigate why the test above fails with this change.

[Miyoung Shin(g), 2014-10-26 02:14:40]

The CQ bit was checked by myid.o.shin@gmail.com

[Miyoung Shin(g), 2014-10-26 02:14:40]

The CQ bit was unchecked by myid.o.shin@gmail.com

[Miyoung Shin(g), 2014-10-26 02:16:40]

The CQ bit was checked by myid.o.shin@gmail.com

[commit-bot: I haz the power, 2014-10-26 02:16:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/619613005/360002

[commit-bot: I haz the power, 2014-10-26 02:17:51]

Committed patchset #9 (id:360002) as 184411

[kochi, 2014-10-27 04:32:08]

On 2014/10/26 02:17:51, I haz the power (commit-bot) wrote:
> Committed patchset #9 (id:360002) as 184411

FYI, this caused interactive_ui_tests failure again (even with r184409 from Rob)
on OSX 10.6/10.8 bots.

http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/2...

As I've suggested in the revert (r184423) message,
please run non-blink (chromium) trybots to make sure this doesn't break
Chromium builds.

[robwu, 2014-10-27 10:56:20]

Miyoung, could you edit the issue description and mention that the handing of
Enter has moved from keydown to keypress? This is a significant change. And
while you're at it, remove the obsolete "Commited: ..." lines.

[pdr., 2014-10-27 18:54:51]

On 2014/10/27 at 04:32:08, kochi wrote:
> On 2014/10/26 02:17:51, I haz the power (commit-bot) wrote:
> > Committed patchset #9 (id:360002) as 184411
> 
> FYI, this caused interactive_ui_tests failure again (even with r184409 from
Rob)
> on OSX 10.6/10.8 bots.
> 
>
http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/2...
> 
> As I've suggested in the revert (r184423) message,
> please run non-blink (chromium) trybots to make sure this doesn't break
> Chromium builds.

I've just nominated Miyoung for tryjob access. This will take ~2 days to go
through. Until then, Miyoung, please ask someone on #blink to run these for you.

[Miyoung Shin(g), 2014-11-02 16:41:50]

pdr@,

Thank you for your nomination. Now I can run tryjob by myid.shin@samsung
account. 
But this patch was uploaded by myid.o.shin@gmail.com account, unfortunately, I
can't run it for this patch by myself.

I disabled WebViewInteractiveTest.NewWindow_OpenInNewTab.
(https://codereview.chromium.org/692113002/)
So, I think I can reland this patch. but I make sure about it before relanding.

Could you run tryjob for interactive_ui_tests on MAC, Linux, Win ?
I tested it manually on MAC and it passed.

[pdr., 2014-11-02 19:10:37]

On 2014/11/02 at 16:41:50, myid.o.shin wrote:
> pdr@,
> 
> Thank you for your nomination. Now I can run tryjob by myid.shin@samsung
account. 
> But this patch was uploaded by myid.o.shin@gmail.com account, unfortunately, I
can't run it for this patch by myself.

Please email amenig@google.com and ask for tryjob access from your other
account.

> 
> I disabled WebViewInteractiveTest.NewWindow_OpenInNewTab.
(https://codereview.chromium.org/692113002/)
> So, I think I can reland this patch. but I make sure about it before
relanding.
> 
> Could you run tryjob for interactive_ui_tests on MAC, Linux, Win ?
> I tested it manually on MAC and it passed.

I added a bunch of tryjobs but please double-check that these run
interactive_ui_tests.

[robwu, 2014-11-02 20:18:28]

The Blink patch that disabled the failing tests
(https://codereview.chromium.org/641233004/) has been merged with Chromium
(https://codereview.chromium.org/699533002), so I'm going to land the fix for
the Enter key (https://codereview.chromium.org/649343002/).

Could you wait until that patch lands, then rebase and remove your changes from
TestExpectations?

[Miyoung Shin(g), 2014-11-03 01:20:18]

@pdr, 
> I added a bunch of tryjobs but please double-check that these run
> interactive_ui_tests.

I checked to run interactive_ui_tests in tryjobs  you added.

@robwu
> Could you wait until that patch lands, then rebase and remove your changes
from
> TestExpectations?

I rebased and removed the changing TestExpectations in my patch.
Can I commit it ?

[robwu, 2014-11-03 01:22:25]

On 2014/11/03 01:20:18, Miyoung Shin(g) wrote:
> @pdr, 
> > I added a bunch of tryjobs but please double-check that these run
> > interactive_ui_tests.
> 
> I checked to run interactive_ui_tests in tryjobs  you added.
> 
> @robwu
> > Could you wait until that patch lands, then rebase and remove your changes
> from
> > TestExpectations?
> 
> I rebased and removed the changing TestExpectations in my patch.
> Can I commit it ?

Sure, go ahead.

[Miyoung Shin(g), 2014-11-03 06:45:09]

The CQ bit was checked by myid.o.shin@gmail.com

[commit-bot: I haz the power, 2014-11-03 06:45:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/619613005/410001

[commit-bot: I haz the power, 2014-11-03 07:58:12]

Committed patchset #10 (id:410001) as 184783