Assigning text to a "value" property of a <textbox> element can broke it

[_yup_]

1. 1. The textbox (XML) element is a wrapper around the input (HTML) element:
      Code: Select all

      <binding id="textbox" extends="xul:box" role="xul:textbox">
        <content>
          <children/>
          <xul:hbox class="textbox-input-box" flex="1" xbl:inherits="context,spellcheck">
            <html:input class="textbox-input" anonid="input"
                        xbl:inherits="value,type,maxlength,disabled,size,readonly,placeholder,tabindex,accesskey,noinitialfocus,mozactionhint,spellcheck"/>
          </xul:hbox>
        </content>
      </binding>
      

      v0.png
   2. createElement("textbox") creates not a ready-to-use object, but a "blank", that lucks many necessary functions. All properties/functions from XBL bindings get added to it only at adding this element to DOM tree (only at this stage its __proto__ is being changed from XULElementPrototype to XBL prototype).
      Before adding to DOM tree:
      v1.png
      After adding to DOM tree:
      v2.png
      And XULElementPrototype becomes a grandparent:
      v3.png
   3. Also only at adding to DOM tree the textbox's constructor() is being executed.
      This constructor is quite remarkable. (Note manipulations with inputField.value - inputField is an underlying HTML input element):
      Code: Select all

      var str = this.boxObject.getProperty("value");
      if (str) {
        this.inputField.value = str;
        this.boxObject.removeProperty("value");
      }
      
      this._setNewlineHandling();
      
      if (this.hasAttribute("emptytext"))
        this.placeholder = this.getAttribute("emptytext");
      

   4. It looks like the problem stems from the fact that XBL bindings can only add new properties/methods, but not replace (overwrite) existed ones. So when we are assigning something to textbox.value before adding this element to DOM tree, we thus preventing to add a value property from the XBL binding (with its getter and setter).
2. On the other hand, cloneNode() returns a real clone of the source element - with all its currently existing properties.
   So if we are cloning an element that already is (or was) attached to DOM tree, we receive a fully functional element (or a tree of elements).
   And if we are cloning a textbox that never was attached to DOM tree, we receive a non-functional element without its XBL bindings.

3. Anyway, using setAttribute it works too.

   Only when setAttribute("value", ...) is used instead of direct assignment to .value.
   If both operations are used, then textbox will be broken as well.
   For example, with:
   Code: Select all

   var label = document.getElementById("label");
   var textBox = document.createElement("textbox");
   textBox.setAttribute('value', "initial value");
   textBox.value = "trash";
   
   label.before(textBox);
   

   the input field will appear on screen with "initial value", and you will be able to change this text, but textBox.value will contain "trash" all the time.

------------------------------------------------------------------------------
The possible fix of the textbox constructor is quite simple and very similar to what is implemented in it already - just look if a .value property is present in the object being processed, and if yes then transfer its content to the inputField.value and remove the existent field:

Code: Select all

<constructor><![CDATA[
  if (this.hasOwnProperty("value")) {
    try {
      this.inputField.value = this.value;
    } catch (e) {}
    delete this.value;
  } else {
    var str = this.boxObject.getProperty("value");
    if (str) {
      this.inputField.value = str;
      this.boxObject.removeProperty("value");
    }
  }

  this._setNewlineHandling();

  if (this.hasAttribute("emptytext"))
    this.placeholder = this.getAttribute("emptytext");
]]></constructor>

[_yup_]

find the bugzilla bug that fixed it (assuming Mozilla fixed it); e.g. using mozregression.

I have found only two related bug reports (opened 17 and 20 years ago):
textbox does not report correct value
Unnecessary to forward properties to textbox's HTML inputField
Both are still open. Looks like no one took care of this problem.

But last reply at the second link (it was very short discussion) was:

The only property we should really need to forward is 'value'

And such "forwarding" is almost exactly what I am proposing above.

[Moonchild]

And this "forwarding" is exactly what I am proposing above.

So what exactly are you proposing should be changed in the platform?

[_yup_]

Current texbox constructor is:

Code: Select all

      <constructor><![CDATA[
        var str = this.boxObject.getProperty("value");
        if (str) {
          this.inputField.value = str;
          this.boxObject.removeProperty("value");
        }

        this._setNewlineHandling();

        if (this.hasAttribute("emptytext"))
          this.placeholder = this.getAttribute("emptytext");
      ]]></constructor>

I propose to add a few lines at its start:

Code: Select all

      <constructor><![CDATA[
        if (this.hasOwnProperty("value")) {
          try {
            this.inputField.value = this.value;
          } catch (e) {}
          delete this.value;
        } else {
          var str = this.boxObject.getProperty("value");
          if (str) {
            this.inputField.value = str;
            this.boxObject.removeProperty("value");
          }
        }

        this._setNewlineHandling();

        if (this.hasAttribute("emptytext"))
          this.placeholder = this.getAttribute("emptytext");
      ]]></constructor>

This additional code looks whether .value already exists in the object, and if it is so and its content can be converted to string, then it forwards this content to the underplayed input element, and removes the .value (in order to allow to bind the regular XBL value property).

The affected file is textbox.xml.

[Kris_88]

Only when setAttribute("value", ...) is used instead of direct assignment to .value.

Yes, that's understandable.
Most likely, the developers simply did not intend for this to be used, and there are many such subtleties.
For example, the article you linked to states explicitly:

Checkboxes have a checked property which may be used to check or uncheck the checkbox. ...
Note: If you're creating the checkbox dynamically and it's not yet added to the DOM, you must use setAttribute("checked", "false") instead, because the XBL isn't initiated yet.)


https://udn.realityripple.com/docs/Arch ... _Interface

[Moonchild]

Thanks. I filed Issue #2787 (UXP) to track this and will consider taking up your proposed fix. Seems a logical approach.
Of note I don't necessarily want to fall into the trap of having to change this for all XUL controls, because it's simply not part of the way XUL works normally. I'd like some feedback on if this should be an exception or not (which I'm considering because textbox/input was, obviously, an open issue for Mozilla too).

[vannilla]

(Not sure if using this thread for feedback is fine, but in the meantime.)
In my personal experience, which is not that extensive but I'm not a newbie either, I think it should be considered an exception.
Other "static" elements like labels can be modified after being added to the DOM (just hold the reference) without major issues, but that's not always the case for "interactive" elements like input fields as exemplified by this thread, even if it's a very corner case.
Basically, do the least amount of work needed; no other element had a similar bug report in 20 years, anyway.

[Moonchild]

Not sure if using this thread for feedback is fine,

Yes it's fine!

it's a very corner case.
...
no other element had a similar bug report in 20 years, anyway.

Exactly my thinking.

[_yup_]

Note: If you're creating the checkbox dynamically and it's not yet added to the DOM, you must use setAttribute("checked", "false") instead, because the XBL isn't initiated yet.)

And it is the only control with such warning.
Moreover, it does not contain an unambiguous prohibition to have the element.checked (I can't treat "you must use setAttribute("checked", "false") instead" as such), whereas presence of this property at attaching to DOM tree moment renders a whole element useless.

Of note I don't necessarily want to fall into the trap of having to change this for all XUL controls, because it's simply not part of the way XUL works normally.

The problem is common for all elements with XBL bindings: an item (field/property/method) from XBL bindings will not be accessible if an item with the same name already exists in the object (element) when its constructor is being executed.

Of course, it is possible to impose an explicit ban ("just don't do this") in the documentation. But personally, I don't like such error-prone variant - it contradicts to all my professional background.

As for me, any element with XBL bindings should be ready to meet such situation and handle it properly. (After all, not so many XUL elements have XBL bindings, and proposed method of handling is quite general: check existence -> if exists, delete. And only in rare cases there will be an additional step before deletion: check if the value of the existed property is suitable -> if yes, use it (move to another place)).

All this also is a part of another general problem: many elements' attributes have correspondent properties. Assigning of some value to an attribute affects a property too. And this does not work in the opposite direction! So attributes have a priority over properties.

But this works when the element is already attached to a DOM tree. Whereas we are discussing a point between creation and attaching. What if the element has an attribute and a property, and their values are contradicting?
It is logical, that attribute should prevail. But how this is implemented in the current code? It seems, nohow (see one of my examples). Whereas deletion of property will do exactly this.

no other element had a similar bug report in 20 years, anyway.

On the other hand, Gecko (with its XUL "sub-engine") always was a moving target, whereas Goanna (with its XUL "sub-engine") claims to be a "monumental" one.

[Kris_88]

This is probably the rare case where I wouldn't change anything at all.
It's more of a feature of XUL than a bug.

[Moonchild]

Looking at the mixed feedback, I think there's arguments either way, but what I think needs to be done for something that isn't "a moving target" is that consistency should be given more weight than edge cases.
I'd prefer not to create ambiguity where it doesn't need to exist. XUL isn't HTML as I've already said, blurring the lines between a UI dfefinition language and content document language isn't necessarily a good thing, either.

As for me, any element with XBL bindings should be ready to meet such situation and handle it properly.

So yes, this means that either we should accept it everywhere (meaning we should change the XUL definition and all of its ctors) or we should accept it nowhere (the current situation). The proposed solution would also possibly cause interop issues with established code as it deletes the `.value` property that code might rely on in other situations.

It's more of a feature of XUL than a bug.

I tend to agree. It might not be documented very well but that isn't necessarily anything strange when it comes to XUL. This kind of dynamic alteration of XUL has also never been a design goal of it.

I also don't think there are specific pressing issues that would mandate a change, I'll be leaving things as-is. @_yup_ please use some of the alternative code that has been offered to deal with this behaviour, or stick to the paradigm of not altering XUL document trees out of principle.