Bug 80574 – [Forms] Re-factor label.for tests for extending test coverage

• Component: Forms
• Status: RESOLVED FIXED

Test files for "label.for" have cut-and-paste code. Extend these tests to support "meter" and "progress" element will add cut-and-paste code.

For ease of maintainability, it is better to do re-factor.

Re-factoring candidates are:
labels-add-htmlFor-label.html
labels-add-parent-label.html
labels-change-htmlFor-attribute.html
labels-custom-property.html
labels-item-index.html
labels-remove-htmlFor-attribute.html
labels-remove-htmlFor-label.html
labels-remove-parent-label.html
labels-set-htmlFor-attribute.html

Created attachment 130981 [details]
Patch 1

Please ignore patch 1. I'm still working this.

Sorry for confusion.
Patch 1 should work what I expected.
Please review them.
Thanks in advance.

P.S.
I tried to use closure as below. However, this didn't work due by eval function used by shouldBe.

forEachFormControl(
    function (element) {
        shouldBe('element.labels.length', '0');
        var label = document.getElementById("labelId");
        label.htmlFor = element.id;
        shouldBe('element.labels.length', '1');
    });

Created attachment 130983 [details]
Patch 2

Some files use CRLF at end of line. (><)

[Kent Tamura]

Comment on attachment 130983 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=130983&action=review

> LayoutTests/fast/forms/label/labels-add-htmlFor-label.html:18
> +    var id = name + '1';

This rule is very implicit.
I recommend passing formControlDesc to createAllFromControls(), and it fills id properties of formControlDesc.

> LayoutTests/fast/forms/resources/forms-test-util.js:1
> +// A list of labelable elements resides in http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#category-label

We already have forms/resources/common.js.  The content of this file should be moved to common.js.

> LayoutTests/fast/forms/resources/forms-test-util.js:3
> +const formControlClassNames = [

Polluting the global namespace in a utility file is not good. Please make a function like createFormControlDescription(), and avoid to make global variables.

> LayoutTests/fast/forms/resources/forms-test-util.js:20
> +var formControlDesc = {};

'Desc' is not good for variable names.  Use a full word, 'Description'

> LayoutTests/fast/forms/resources/forms-test-util.js:27
> +        labelable: true,

Let's follow C++ style rule.
lebelable -> isLabelable

> LayoutTests/fast/forms/resources/forms-test-util.js:28
> +        listable: true,

listable is not used in this patch.  Please do not add in this patch.

> LayoutTests/fast/forms/resources/forms-test-util.js:30
> +        supported: element + '' == '[object ' + className + ']',

+ '' looks ugly.  Please use .toString().

> LayoutTests/fast/forms/resources/forms-test-util.js:92
> +        // FIXME: How do we know supported input types?
> +        supported: true,

You can detect by
  inputElement.type = typeName;  if (inputElement.type == typeName) ...

> LayoutTests/fast/forms/resources/forms-test-util.js:97
> +formControlDesc.hiddentype.labelable = false;
> +formControlDesc.imagetype.listable = false;

identifier should be camelCase.  hiddentype -> hiddenType

> LayoutTests/fast/forms/resources/label-test-util.js:6
> +const WITH_NO_LABEL = 0;
> +const WITH_PARENT_LABEL = 1;
> +const WITH_SIBLING_LABEL = 2;
> +const WITH_SIBLING2_LABEL = 4;

Constant value names also should be camelCase.

> LayoutTests/fast/forms/resources/label-test-util.js:9
> +function createAllFormControls(labelRelation, preHtml, postHtml)

This name isn't represent the content.
appendAllFormControlsToBody(...) ?

Created attachment 131016 [details]
Patch 3

[Kent Tamura]

Comment on attachment 131016 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=131016&action=review

> LayoutTests/fast/forms/label/labels-add-htmlFor-label-expected.txt:22
> +Not labelable: label [object HTMLLabelElement]
> +FAIL element.labels should be undefined. Was [object NodeList]
> +Not labelable: legend [object HTMLLegendElement]
> +FAIL element.labels should be undefined. Was [object NodeList]

Some tests are failing.

> LayoutTests/fast/forms/label/labels-add-htmlFor-label.html:14
> +const tester = setupLabelsTest();

I don't recommend to use 'const'.
We sometimes run our tests on non-WebKit browsers to see compatibility, and IE doesn't support const.

> LayoutTests/fast/forms/label/labels-add-parent-label.html:16
> +    var data = tester.checkLabels(name);

The tester function name is not descriptive.  It should be getDescriptionDataIfLabelsIsSuported() or something.

> LayoutTests/fast/forms/label/labels-add-parent-label.html:17
> +    if (data == null)

if (!data)

if we follow C++ style rule.

> LayoutTests/fast/forms/label/labels-add-parent-label.html:24
> +    document.getElementById("div1").appendChild(label);

"div1" is implicit. It doesn't appear in this file.

Created attachment 131545 [details]
Patch 4

[Kent Tamura]

Comment on attachment 131545 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=131545&action=review

> LayoutTests/fast/forms/label/labels-add-htmlFor-label-expected.txt:53
> +Check hiddenType
> +PASS element.labels.length is 0
> +PASS element.labels.length is 1

Should they be FAIL?

(In reply to comment #10)
> (From update of attachment 131545 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131545&action=review
> 
> > LayoutTests/fast/forms/label/labels-add-htmlFor-label-expected.txt:53
> > +Check hiddenType
> > +PASS element.labels.length is 0
> > +PASS element.labels.length is 1
> 
> Should they be FAIL?

No at this time.

Bug 80403 will fix "hidden" type and update test results.

[Kent Tamura]

Comment on attachment 131545 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=131545&action=review

>>> LayoutTests/fast/forms/label/labels-add-htmlFor-label-expected.txt:53
>>> +PASS element.labels.length is 1
>> 
>> Should they be FAIL?
> 
> No at this time.
> 
> Bug 80403 will fix "hidden" type and update test results.

When we fix Bug 80403, these tests will become FAIL.  So these tests are incorrect at this moment.

We should write these tests so that they become PASS when Bug 80403 is fixed.

[Kent Tamura]

Comment on attachment 131545 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=131545&action=review

>>>> LayoutTests/fast/forms/label/labels-add-htmlFor-label-expected.txt:53
>>>> +PASS element.labels.length is 1
>>> 
>>> Should they be FAIL?
>> 
>> No at this time.
>> 
>> Bug 80403 will fix "hidden" type and update test results.
> 
> When we fix Bug 80403, these tests will become FAIL.  So these tests are incorrect at this moment.
> 
> We should write these tests so that they become PASS when Bug 80403 is fixed.

We discussed offline.  When Bug 80403 is fixed, These tests for hiddenType will be dropped because of a check in getLabelableElementData.  So my comments made no sense.

[WebKit Review Bot]

Comment on attachment 131545 [details]
Patch 4

Clearing flags on attachment: 131545

Committed r110542: <http://trac.webkit.org/changeset/110542>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

(In reply to comment #15)
> All reviewed patches have been landed.  Closing bug.

This patch broke fast/forms/label/labelable-elements.html on GTK at least:

http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r110549%20(19867)/fast/forms/label/labelable-elements-pretty-diff.html

--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/forms/label/labelable-elements-expected.txt 
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/forms/label/labelable-elements-actual.txt 
@@ -7,7 +7,7 @@
 Labelable: button [object HTMLButtonElement]
 PASS element.labels.length is 1
 
-Unsupported: datalist [object HTMLUnknownElement]
+Not labelable: datalist [object HTMLDataListElement]
 PASS element.labels is undefined.
 PASS element.labels is not null
 
@@ -45,7 +45,7 @@
 PASS element.labels is not null
 
 Labelable: output [object HTMLOutputElement]
-FAIL element.labels.length should be 1. Threw exception TypeError: Cannot read property 'length' of null
+FAIL element.labels.length should be 1. Threw exception TypeError: 'null' is not an object (evaluating 'element.labels.length')
 
 Labelable: progress [object HTMLProgressElement]
 PASS element.labels.length is 1

[Philippe Normand]

(In reply to comment #16)
> (In reply to comment #15)
> > All reviewed patches have been landed.  Closing bug.
> 
> This patch broke fast/forms/label/labelable-elements.html on GTK at least:
> 

Well, it's a new test but it fails :)

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > All reviewed patches have been landed.  Closing bug.
> > 
> > This patch broke fast/forms/label/labelable-elements.html on GTK at least:
> > 
> 
> Well, it's a new test but it fails :)

Oops, platform differences causes this. (~_~)

(1) ENABLE(DATALIST)
-Unsupported: datalist [object HTMLUnknownElement]
+Not labelable: datalist [object HTMLDataListElement]

(2) V8 and JavaScriptCore
Labelable: output [object HTMLOutputElement]
-FAIL element.labels.length should be 1. Threw exception TypeError: Cannot read property 'length' of null
+FAIL element.labels.length should be 1. Threw exception TypeError: 'null' is not an object (evaluating 'element.labels.length')

bug 80466 will fix the output element.

[Philippe Normand]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > All reviewed patches have been landed.  Closing bug.
> > > 
> > > This patch broke fast/forms/label/labelable-elements.html on GTK at least:
> > > 
> > 
> > Well, it's a new test but it fails :)
> 
> Oops, platform differences causes this. (~_~)
> 
> (1) ENABLE(DATALIST)
> -Unsupported: datalist [object HTMLUnknownElement]
> +Not labelable: datalist [object HTMLDataListElement]
> 

datalist is enabled by default in build-webkit so I think that part of the test results should be updated accordingly. If your port doesn't enable DATALIST you should provide platform-specific results for the test.

> (2) V8 and JavaScriptCore
> Labelable: output [object HTMLOutputElement]
> -FAIL element.labels.length should be 1. Threw exception TypeError: Cannot read property 'length' of null
> +FAIL element.labels.length should be 1. Threw exception TypeError: 'null' is not an object (evaluating 'element.labels.length')
> 
> bug 80466 will fix the output element.

Ok, I'll skip this test on GTK then.