563637 - Represents attributes/methods/events availability dependency with the type attribute for the input element

Status: NEW

(Core :: DOM: Forms, defect)

Description

[Mounir Lamouri (:mounir)]

input element attributes, methods and events availability depends on the element type attribute. At the moment some parts of nsHTMLInputElement isn't explicitely checking if the attribute is available for the current type and some other parts (not landed yet) are using |Does${Attribute}Apply| which can be quite annoying if we use this method for the 20/30 attributes/methods/events.

We should have a table which represents available attributes, methods and events to make these checks easier and adding types/attributes simple.

There is a non-normative table representing available attributes/methods/events depending on the input element type. Open $URL and look a little bit below.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Add AttributeApplies method

This is a first patch adding a method checking for attribute availability for the current type. Probably cleaner than having one method per attribute...

I will probably add one or more patches adding IDL attributes check in AttributeApplies and add MethodApplies and EventApplies methods.

Comment 2

[Mounir Lamouri (:mounir)]

BTW, I wrote this patch on top of my html5 forms patch queue [1] so you might not found some code in the tree but it shouldn't make the patch harder to understand I think.

[1] https://hg.mozilla.org/users/mlamouri_mozilla.com/html5forms-patchqueue/summary

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Add IDL attributes to AttributeApplies

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Add MethodApplies method

Less useful than AttributeApplies but I don't see a nice function name that would fit for AttributeApplies and MethodApplies.

Comment 5

[Mounir Lamouri (:mounir)]

I forgot to say that the second patch is assuming that this bug against the specs will be accepted and solved:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12262

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Add EventApplies method

Comment 7

[Mounir Lamouri (:mounir)]

I will write some tests for the few changes in behavior.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So just looking at the first patch so far, it all looks good except the actual implementation of AttributeApplies.

The problem with that implementation, imo is the interleaving of the type and attr checks, leading to things like having to check mType again in the first case of the switch, etc.

I would prefer that we use an actual table here.  We can use mType as the index into an array.  What the array entries should be is an open question.  If we want to pass in nsIAtom, then the array entries should be a null-terminated nsIAtom** or something.  But if we're willing to have an enum, especially a bitfield-like one, then the array entries could be just the set of bits that indicate which attributes apply.

Does that make sense?

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to comment #8)
> The problem with that implementation, imo is the interleaving of the type and
> attr checks, leading to things like having to check mType again in the first
> case of the switch, etc.

I did that to prevent adding a case with mostly the same attributes.

> I would prefer that we use an actual table here.  We can use mType as the index
> into an array.  What the array entries should be is an open question.  If we
> want to pass in nsIAtom, then the array entries should be a null-terminated
> nsIAtom** or something.  But if we're willing to have an enum, especially a
> bitfield-like one, then the array entries could be just the set of bits that
> indicate which attributes apply.
> 
> Does that make sense?

I understand your proposals but I don't really understand why we need another solution. Is that for cosmetic/readability or optimization? I'm not sure I really follow what you tried to say in the first quoted chunk.
I think the null terminated list of nsIAtom** is inelegant and the bitfield seems a bit too much for what we want to do. In addition, some (at least one) attributes availability might depend on other attributes. For example, input.selectedOption will apply for most text fields (text, url, email, ...) but will not apply if the multiple attribute is present. This is not implemented yet but if we are using a bitfield or a list, it would require some dirty special casing which would be much less dirty in the current implementation (maybe because my implementation is already dirty by design? :)).

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I did that to prevent adding a case with mostly the same attributes.

Yes, I understand that.  I'm just saying the result is confusing and fragile.  For example, when you need to add an attribute that applies to some but not all of the things in that first case, life will suddenly get compicated.

> Is that for cosmetic/readability or optimization? 

It's for readability and more importantly maintainability.  The idea is to have a table of types and which attributes apply to each type, and then to have code that examines that table to answer whatever question we're asking.  I would really prefer that to the approach in the patch which has the code and data all mixed together.  When adding a new attribute you'd just add it to the right lines in the table and be done, whereas right now you have to sort through the logic in the switch, etc.

> In addition, some (at least one) attributes availability might depend on other
> attributes.

That's a much bigger problem for representing things in an actual data table, though we could add the ability to express that sort of thing in general in a table-driven way...

> maybe because my implementation is already dirty by design?

Well, yes.  ;)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 517334 [details] [diff] [review]
Add AttributeApplies method

r=me with the comments per our irc discussion.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 517598 [details] [diff] [review]
Add IDL attributes to AttributeApplies

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 517608 [details] [diff] [review]
Add MethodApplies method

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 517611 [details] [diff] [review]
Add EventApplies method

r=me

Comment 15

[JP Rosevear [:jpr]]

All dependencies are resolved.  Can this land now?

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to JP Rosevear [:jpr] from comment #15)
> All dependencies are resolved.  Can this land now?

The patch is very likely seriously bit-rotted. Given that it's only improving code readability, I prefer to not dedicate cycles on that for the moment. After basecamp will be achieved, I will try to dedicate some time to HTML5 Forms.