#394 ✓resolved
janiv

Form.Element.Methods not applied to button tags

Reported by janiv | October 17th, 2008 @ 07:42 PM | in 1.7

If you create a button tag and attempt to use any of the Form.Element methods allowed such as disable() or enable(), an error results.

Example:


<button type="button" id="addUserBtn">
  <img src="/img/user_add.png" alt="" />
  Add New User
</button>

<script type="text/javascript">
  $('addUserBtn').disable();
</script>

A proposed fix for this would be to modify Element.addMethods and change the following lines:


  if (!methods) {
    Object.extend(Form, Form.Methods);
    Object.extend(Form.Element, Form.Element.Methods);
    Object.extend(Element.Methods.ByTag, {
      "FORM":     Object.clone(Form.Methods),
      "INPUT":    Object.clone(Form.Element.Methods),
      "SELECT":   Object.clone(Form.Element.Methods),
      "TEXTAREA": Object.clone(Form.Element.Methods)
    });
  }

to:


  if (!methods) {
    Object.extend(Form, Form.Methods);
    Object.extend(Form.Element, Form.Element.Methods);
    Object.extend(Element.Methods.ByTag, {
      "FORM":     Object.clone(Form.Methods),
      "INPUT":    Object.clone(Form.Element.Methods),
      "SELECT":   Object.clone(Form.Element.Methods),
      "TEXTAREA": Object.clone(Form.Element.Methods),
      "BUTTON":   Object.clone(Form.Element.Methods)
    });
  }

Comments and changes to this ticket

  • Andrew Dupont

    Andrew Dupont October 22nd, 2008 @ 05:30 PM

    • Milestone set to 1.6.1
    • State changed from “new” to “bug”
    • Tag set to form
    • Assigned user set to “Andrew Dupont”
  • Andrew Dupont

    Andrew Dupont November 19th, 2008 @ 06:11 AM

    • Assigned user changed from “Andrew Dupont” to “Tobie Langel”

    Assigning to Tobie so he can roll it into the dom.js rewrite.

  • Andrew Dupont

    Andrew Dupont December 20th, 2008 @ 02:05 AM

    • Tag changed from form to discuss, form

    Hold on. Before we do this, actually, we need to decide how the button element will be serialized.

    In IE <= 7 the value of a button is its innerHTML, rather than its value attribute. I'd much prefer not to emulate this behavior when we serialize buttons, but is it too surprising to have our serialization logic do the "right" thing even when the browser does not?

  • Juriy Zaytsev

    Juriy Zaytsev December 20th, 2008 @ 07:31 AM

    Andrew, isn't "button" serialization defined in the specs?

  • Juriy Zaytsev

    Juriy Zaytsev February 23rd, 2009 @ 12:29 AM

    • Assigned user changed from “Tobie Langel” to “Juriy Zaytsev”

    Specs [1] say that button's value is what's submitted to the server (as long as a button element is considered a successful control [2], of course).

    I think it makes sense to make button's getValue return value for consistency; that's also what should be submitted to the server.

    disable and enable will also work as expected, since HTMLButtonElement implements disabled property.

    Does this sound reasonable? Any thoughts, objections?

    [1] http://www.w3.org/TR/html401/int....5

    [2] http://www.w3.org/TR/html401/int...

  • Tobie Langel

    Tobie Langel February 23rd, 2009 @ 09:17 AM

    • Milestone changed from 1.6.1 to 1.7

    We should definitely use value (if it is present) and fallback on innerHTML otherwise. IE's behaviour is a known AND painful bug.

  • Juriy Zaytsev

    Juriy Zaytsev March 3rd, 2009 @ 01:20 AM

    • Assigned user changed from “Juriy Zaytsev” to “Andrew Dupont”
    • Tag changed from discuss, form to discuss, form, patched, tested
    • Milestone changed from 1.7 to 1.6.1

    Attaching patch and tests. Also fixed writeAttribute not being able to set button's value (due to broken setAttribute in IE).

    Serialization uses value of a button, as per specs.

  • Tobie Langel

    Tobie Langel March 3rd, 2009 @ 03:24 AM

    • Tag changed from discuss, form, patched, tested to discuss, form, needs_patch, needs_tests

    Couple of comments:

    1. if we fix Element#writeAttribute('value', value) we should fix in the same release Element#readAttribute('value').
    2. Your current fix has a regression issue should the value be written on anything other than a form element. Doing $('my_div_id').writeAttribute('value', 'foo') currently sets the custom value attribute of div#my_div_id to "foo" , with your fix a TypeError (element.setValue is not a function) would be thrown.
    3. Element#writeAttribute('foo', false) actually removes the attribute. What would happen with your changes? Is that the desired behaviour? Is it consistant with the rest of the API?
    4. Attributes should be coerced to strings. That doesn't seem to be the case here.
    5. The DOM 2 specs imply a difference between the expando and attribute for value [1] (the former is dynamic, the latter isn't). What are your thoughts regarding this? How should we handle it ?
    6. The Element#writeAttribute changes lacks proper unit test.

    Oh... and before I come out as looking awfully rude... thanks a lot for your work, and the rest LGTM. :)

    [1] http://www.w3.org/TR/DOM-Level-2...

  • Juriy Zaytsev

    Juriy Zaytsev March 3rd, 2009 @ 06:57 AM

    Jeez, the problem was in update following Element creation and overwriting value : / I thought I was going crazy not being able to reproduce it.

    re: 1, readAttribute (or, essentially, getAttribute that it delegates was unaffected) so there was nothing to fix ; )

    re: 2, thanks for catching that; It was, indeed, a stupid thing to do.

    The rest seems irrelevant now that we know the actual problem, but how to work around the fact that setting button's contents overwrites value in IE (I tried both - innerHTML and textNode appending)?

  • Tobie Langel

    Tobie Langel March 3rd, 2009 @ 08:08 AM

    Wasn't even aware of these shortcomings. Ouch! Any clue how that's done elsewhere ?

  • Diego Perini

    Diego Perini March 3rd, 2009 @ 05:03 PM

    Tobie, since I don't see any mention of it here I wanted to let a note about IE and buttons. On IE the getAttributeNode() method can be used on a "button" to recover the previously set value, the original in-line attribute value set in the HTML code.

    It seems there are/was at least two or three bugs that seemed related to attributes and probably some can be solved by using getAttributeNode(). This is just for "reading" attributes, writing them is another issue... ;-)

    The same method can be used in some situations where id/names has been overwritten by other elements id/name or just happen to be conflicting (see ticket #576).

  • Tobie Langel

    Tobie Langel March 3rd, 2009 @ 05:23 PM

    Hi Diego,

    Thanks for the tip.

    FWIW, Element#getAttributeNode('foo').value is strictly identical to Element#attributes['foo'].value. We use the latter in quite a number of areas already.

    I'm more concerned about the particularities of the value attribute mentioned in the specs I linked above, and how it's implemented by the different vendors.

    My gut feeling is that this fix isn't ready for inclusion and needs more discussion.

    I think it would be safer to separate this issue into two (handling the button issue, and fixing the reading and writing of the value attribute).

  • Juriy Zaytsev

    Juriy Zaytsev March 3rd, 2009 @ 05:37 PM

    Tobie, I agree about discussing an issue of "value being overwritten by content in IE" separately. The attached patch, on the other hand, takes care of originally reported problem with button elements missing Form.Element.Methods - allowing buttons to be enableed, disable'd and, of course, serialized. If we remove writeAttribute change, does the patch/tests look good? If so, then I don't see a reason not to include this in 1.6.0.4 (I, for one, have been using similar patch for quite some time now - in a custom extensions file, as not being able to enable/disable buttons is quite frustrating)

  • Tobie Langel

    Tobie Langel March 3rd, 2009 @ 06:09 PM

    Question is: should Form.Serialize.button rely on element.value or element.getAttributeNode('value').value ?

    And what edge-cases do each bring about (especially in IE).

    I'm worried about this causing complicated debugging issues for people upgrading, and I'd like to make sure that we fix this correctly (if possible).

    Prototype code is based on code we use in real life. As I've always avoided working with button elements because of the issues they caused, I don't personally feel ready to include this in Prototype (which doesn't mean someone else wouldn't be).

    Unless you use identical value and innerHTML strings for button elements (which kind of defeats the purpose), you're forced to handle the discrepancies between the different implementations server-side.

    I'm worried that fixing this on the front-end for ajax requests might actually cause more harm than good.

    I'd like to have other core member's opinion on this matter.

    Also, a bit of research on what other libs do would be helpful.

  • Andrew Dupont

    Andrew Dupont March 9th, 2009 @ 02:32 AM

    • Milestone changed from 1.6.1 to 1.7

    I'm deferring this to the next version because we need to make a whole bunch of decisions about how button is handled.

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 02:01 AM

    • Tag changed from discuss, form, needs_patch, needs_tests to discuss, needs_patch, needs_tests, section:dom

    [not-tagged:"form" tagged:"section:dom" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 02:26 AM

    • Tag changed from discuss, needs_patch, needs_tests, section:dom to discuss, missing:tests, needs_patch, section:dom

    [not-tagged:"needs_tests" tagged:"missing:tests" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 02:28 AM

    • Tag changed from discuss, missing:tests, needs_patch, section:dom to discuss, missing:patch, missing:tests, section:dom

    [not-tagged:"needs_patch" tagged:"missing:patch" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 03:36 AM

    • Tag changed from discuss, missing:patch, missing:tests, section:dom to discuss, missing:patch, needs:tests, section:dom

    [not-tagged:"missing:tests" tagged:"needs:tests" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 03:37 AM

    • Tag changed from discuss, missing:patch, needs:tests, section:dom to discuss, needs:patch, needs:tests, section:dom

    [not-tagged:"missing:patch" tagged:"needs:patch" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 03:39 AM

    • Tag changed from discuss, needs:patch, needs:tests, section:dom to needs:discussion, needs:patch, needs:tests, section:dom

    [not-tagged:"discuss" tagged:"needs:discussion" bulk edit command]

  • T.J. Crowder

    T.J. Crowder November 16th, 2009 @ 04:50 PM

    • Assigned user cleared.

    [responsible:none bulk edit command]

  • GitHub Robot

    GitHub Robot October 18th, 2010 @ 12:52 AM

    • State changed from “bug” to “resolved”
    • Importance changed from “” to “”

    (from [1fcf2e029977869e3389f363607cbb1c0c36fd94]) Extend BUTTON elements with everything defined in Form.Element.Methods. Ensure BUTTON elements are traversed in Form.getElements and serialized in Form.serialize. [#394 state:resolved] [#688 state:resolved] (Luis Gomez, Samuel Lebeau, kangax, Andrew Dupont) http://github.com/sstephenson/prototype/commit/1fcf2e029977869e3389...

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

The Prototype JavaScript library.

Shared Ticket Bins

Attachments

Referenced by

Pages