[Webkit-unassigned] [Bug 139179] HTMLSelectElement add() should support index as second argument
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Dec 31 06:42:13 PST 2014
https://bugs.webkit.org/show_bug.cgi?id=139179
--- Comment #46 from Shivakumar J M <shiva.jm at samsung.com> ---
(In reply to comment #37)
> Comment on attachment 243820 [details]
> Patch-Updated-Review5
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243820&action=review
>
> Nice work. I think we are almost there. review- because the Mac build is
> still failing
>
> At this point it’s a little unclear to me the total set of changes in
> behavior we are introducing. For changes that are incompatible with older
> versions of WebKit, I am concerned that there may be web content or perhaps
> apps on iOS or Mac that depend on the old behavior, even if it’s not
> consistent with other browsers. So I want to be really clear about what we
> are changing.
>
> The change that is explicit here is handling numeric indices as the second
> argument to both add functions.
>
> But I think another thing this changes is that the first argument to
> HTMLSelectElement's add is now type checked and we throw an exception if
> it's not the right type. But we have left it so you can omit this first
> argument and it will silently do nothing. Is that really what other web
> browsers do and what the standard calls for? Or are we just retaining a
> WebKit quirk by allowing the argument to be omitted?
>
> What about extra arguments? Where’s the test that covers extra arguments to
> see if we raise an exception for the extra ones or just ignore them
> silently? This is another thing that might have changed when we changed how
> the bindings worked.
>
> I can’t tell if we got all the expectations right or not because it’s hard
> for me to read through the test to see what is expected, and this worries me
> slightly about a change that affects web-visible API. What I want to know is
> exactly what we are changing, how this relates to what WebKit used to do,
> what other browsers do, and what the standard says.
>
> > Source/WebCore/ChangeLog:10
> > + Also This matches the behavior of Chrome and FireFox.
>
> Stray capital letter "This".
>
> > Source/WebCore/html/HTMLOptionsCollection.idl:32
> > + [ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] optional HTMLElement element,
> > + [Default=Undefined] optional HTMLElement? before);
>
> ObjCLegacyUnnamedParameters is incorrect here and should be omitted. That’s
> why the Mac build is failing.
>
> Indentation of the second line is wrong. Correct style for WebKit would be
> to put this all on one line, or to indent the second line by 4 spaces.
> Indenting by 24 spaces is strange.
>
> > Source/WebCore/html/HTMLSelectElement.cpp:235
> > + add(element, downcast<HTMLElement>(options()->item(beforeIndex)), ec);
>
> I would have written:
>
> add(element, item(beforeIndex), ec);
>
> I like my version better because there’s no need for an additional type cast.
>
> > Source/WebCore/html/HTMLSelectElement.idl:50
> > + [RaisesException] void add(HTMLElement element, long before);
>
> I think it’s a little strange to put this after the "remove" function just
> to share the #if. It puts "remove" between the two overloads of "add", and
> it puts "add" between the two alternative versions of "remove". Seems like
> we should move this at least above the "// In JavaScript ..." comment for
> clarity.
>
> > LayoutTests/ChangeLog:10
> > + * fast/dom/HTMLSelectElement/select-add-optgroup-options.html: Added.
>
> The name of this test is not so great. I would call it
> "options-collection-add.html" since it’s in the HTMLSelectElement directory.
> Or put it in a new HTMLOptionsCollection directory and name it "add.html".
>
> > LayoutTests/ChangeLog:11
> > + * fast/dom/HTMLSelectElement/select-add-optgroup.html:
>
> The name of this test is no longer all that good. The test covers many
> different aspects of the add function, not just the handling of optgroup.
> Also, since it’s in the HTMLSelectElement directory I don’t think the test
> needs "select" in its name. I would just name it "add.html".
>
> > LayoutTests/fast/dom/incompatible-operations-expected.txt:15
> > +PASS aSelect.add(aDOMImplementation, aDOMImplementation) threw exception TypeError: Type error.
>
> To me it seems strange that this test that seems to be intended to to cover
> DOM bindings in general is actually only testing HTMLSelectElement’s add
> function. I think we should be adding coverage of some other DOM function if
> the intent of this test is not specific to the add function. Worth
> researching when this test was added.
>
> > LayoutTests/fast/dom/HTMLSelectElement/select-add-optgroup-options.html:74
> > +mySelect.options.add(new Option("X", "Y", false, false), null);
> > +shouldBeEqualToString('deepCopy()', '0,1,2,Y');
>
> When creating a test like this one it's useful to have a substantive part of
> the test in the "should be" expression so we can see what is being tested.
> Typically we make helper functions so that we can write a small exception
> and it can be clear what we are testing. For example, parts of the test
> would look like this:
>
> shouldBeEqualToString('testAdd(0)', 'Y,0,1,2');
> shouldBeEqualToString('testAdd(1)', '0,Y,1,2');
>
> shouldBeEqualToString('testAdd("0")', 'Y,0,1,2');
> shouldBeEqualToString('testAdd("1")', 'Y,0,1,2');
>
> shouldBeEqualToString('testAdd(null)', '0,1,2,Y');
>
> And so on. Note how this makes both the test code itself, and the test
> output, easier to read. Just a tip for future tests. Not insisting that you
> improve this one, although you are welcome to.
>
> If you had written the test like this it would have been easier for me to
> read it and see what the expectations are. I’m having a bit of a hard time
> seeing what's being tested. It would be especially useful to use a helper
> function to create the option elements because the repetitive "new
> Option('X', 'X', false, false)" is distracting.
Dear Darin,
Thanks for the inputs, below are some observations:
Now patch is passing the build, will modify all the tests to use "testAdd(null)" kind of functions and helper functions to create the different option/group elements to make more readable. will upload new patch for modified tests to cover extra arguments also.
As per the Spec both HTMLSelectElement and HTMLOptionsCollection should support adding option element using index as well as using other element. But HTMLSelectElement was not supporting adding option element using index and
HTMLOptionsCollection was not supporting adding option element using other element ( i.e add( newelement, oldelement)).
So currently apps has to use HTMLSelectElement add(), if we want to add element using other element and to add option element using index, need to use HTMLOptionsCollection add().
Also now First argument cannot be Optional and second argument is optional, nullable also default is null. But setting [Default=Null] for second argument, did not compile, we need to find way to set default as null.
[TreatUndefinedAs=Null] also did not work. so with new change both HTMLSelectElement and HTMLOptionsCollection add() now supports adding option element using index as well as using other element.
1) The failed tests js/dom/select-options-add.html and fast/dom/HTMLSelectElement/select-add-optgroup.html needs to be updated as per new behavior. since now select2.options.add() with index as -2, -1/0, 0/0, 1/0 adds element to list so length needs to be updated.
2) In HTMLOptionsCollection.idl, now add() with index is defined to take HTMLOptionElement, since it gives Public API missing error in build.
"[RaisesException] void add([Default=Undefined] optional HTMLOptionElement option, unsigned long index);". So mySelect.options.add(group, 1), to add group failed, so may need to add one more add() function which takes HTMLOptGroupElement as a parameter as below, need to try these in build and test. "[RaisesException] void add([Default=Undefined] optional HTMLOptGroupElement option, unsigned long index);"
3) Also select2.options.add(option2, -2) is giving error, need to check these test for error case as done earlier in HTMLOptionsCollection add() ("if (index == -1 || unsigned(index) >= length())"), need to try these in build and test.
4) Need to explore if add is called with no inputs add(), should be undefined Or throw type error.
Thanks
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141231/c2b59e0c/attachment-0002.html>
More information about the webkit-unassigned
mailing list