[webkit-reviews] review denied: [Bug 115885] <label> element should send key and focus events if it has contenteditable attribute. : [Attachment 201311] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Yael <yael at webkit.org>'s
request for review:
Bug 115885: <label> element should send key and focus events if it has
contenteditable attribute.
https://bugs.webkit.org/show_bug.cgi?id=115885

Attachment 201311: Patch
https://bugs.webkit.org/attachment.cgi?id=201311&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=201311&action=review


> LayoutTests/fast/forms/label/labels-contenteditable.html:16
> +var parent = document.createElement('div');
> +
> +parent.innerHTML = '<label id="label1" for="input1">text</label><input
id="input1"> <label id="label2" contenteditable for="input2">text</label><input
id="input2">';

Why do you do that?
Why isn't that in the markup?

> LayoutTests/fast/forms/label/labels-contenteditable.html:34
> +label1.addEventListener('keydown', function () { alert("FAIL: keydown should
not be sent if the label does not have contenteditable attribute"); });
> +label1.addEventListener('keypress', function () { alert("FAIL: keypress
should not be sent if the label does not have contenteditable attribute"); });
> +label1.addEventListener('keyup', function () { alert("FAIL: keyup should not
be sent if the label does not have contenteditable attribute"); });
> +label1.addEventListener('focus', function () { alert("FAIL: focus should not
be sent if the label does not have contenteditable attribute"); });
> +label1.addEventListener('blur', function () { alert("FAIL: blur should not
be sent if the label does not have contenteditable attribute"); });
> +input1.addEventListener('focus', function () { alert("PASS: label passed the
focus to the corresponding control"); });
> +label2.addEventListener('keydown', function () { alert("PASS: keydown should
be sent if the label has contenteditable attribute"); });
> +label2.addEventListener('keypress', function () { alert("PASS: keypress
should be sent if the label has contenteditable attribute"); });
> +label2.addEventListener('keyup', function () { alert("PASS: keyup should be
sent if the label has contenteditable attribute"); });
> +label2.addEventListener('focus', function () { alert("PASS: focus should be
sent if the label has contenteditable attribute"); });
> +label2.addEventListener('blur', function () { alert("PASS: blur should be
sent if the label has contenteditable attribute"); });
> +input2.addEventListener('focus', function () { alert("PASS: label passed the
focus to the corresponding control"); });

You should use debug() logging instead of alert().

> LayoutTests/fast/forms/resources/label-test-util.js:80
> +function mouseMoveToLabel(labelId) {
> +    var label = document.getElementById(labelId);
> +    var itemHeight = Math.floor(label.offsetHeight / label.size);
> +    var offset = 5;
> +    if (window.eventSender)
> +	   eventSender.mouseMoveTo(label.offsetLeft + offset, label.offsetTop +
offset - window.pageYOffset);
> +}

There is already code to do that in LayoutTest. None that you can use?

> Source/WebCore/html/HTMLLabelElement.cpp:69
> -    return false;
> +    HTMLLabelElement* that = const_cast<HTMLLabelElement*>(this);
> +    return that->isContentEditable();

This is very much non-const.

Shouldn't you instead assert there is no pending style recalc and use
rendererIsEditable()???