Opened 7 years ago

Last modified 3 years ago

#17213 reopened task

consider stopping automatically applying Widget properties to DOMNode

Reported by: Randy Hudson Owned by:
Priority: high Milestone: 1.15
Component: Dijit Version: 1.7.0
Keywords: Cc: cjolif
Blocked By: Blocking:

Description

Starting in dojo 1.7 (I think) _WidgetBase's set method will map a property according to:

3. apply to focusNode or domNode if standard attribute name, excluding funcs like onClick.

Previously, a property would only be applied to the DOM if it was in the attribute map. My widget's attribute map is empty, so the property (the property was "className" in this case) was simply stored as a property of the widget and not applied to the DOM. Now that we have upgraded to 1.8.4, the property ends up overwriting an element's class in the template.

It looks like this new behavior tries to map hundreds of properties to the DOM, which will break a lot of existing widgets which happened to chose property names that collide with DOM node properties. Also, as browsers are upgraded, that number will grow and introduce regressions in previously working widgets.

Attachments (1)

sample.zip (13.0 KB) - added by manklu 6 years ago.
A small inspiration. It's terrible incomplete and has many loose ends. But it seems to work and should be usable. The biggest missing part is the integration of a polyfill.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 7 years ago by bill

Cc: cjolif Douglas Hays added
Milestone: tbd2.0
Summary: Widget properties are getting applied to DOMconsider stopping automatically applying Widget properties to DOMNode
Type: defecttask
Version: 1.9.01.7.0

Well, I agree that this new feature (like other new features, ex: AMD) could cause regressions in corner cases. There are even a few places in dijit where we have to disable this behavior, like in _FormWidget, which does:

// Override automatic assigning type --> focusNode, it causes exception on IE.
// Instead, type must be specified as ${type} in the template, as part of the original DOM
_setTypeAttr: null

However, I don't want to remove the feature now (i.e. before 2.0) because there's likely lots of code relying on it, whereas you are the only one that has complained about it, or at least, the only one to file a ticket.

Also, Doug wanted this code for a reason. It's because (as you said) there are hundreds of new DOMNode attributes showing up, especially HTML5 and aria attributes like aria-labelledby and role, and it's much more convenient to have them automatically mapped to the DOMNode than to (1) list them all within the widget definition or (2) tell users that something like <input data-dojo-type=dijit/TextBox placeHolder=...> doesn't work.

Having said that, the behavior is quite magical, and something we could consider removing for 2.0.

comment:2 Changed 7 years ago by Randy Hudson

Providing a function without a well-defined behavior isn't all that convenient. You would be surprised at how many people think they are doing something wrong and just change their code instead of doubting this behavior (or even trying to figure out what was happening).

The issue isn't just regressions due to moving up from dojo 1.6. Browsers like Firefox or Chrome could add new properties at any time and break code that was working previously. To prevent this, widgets have to write explicit _setFooAttr functions for every property.

comment:3 Changed 7 years ago by bill

The behavior is well defined (and you quoted it in the ticket description), but perhaps not well understood or well documented.

I agree that browsers like Firefox or Chrome could add new properties at any time, and that could break existing code. Note though the reason the feature was added in the first place was to deal with Firefox or Chrome adding new properties at any time, without having to retrofit dojo code to be able to process them.

This somehow reminds me of the problem that any time I add a new function or property to a class, I might be breaking subclasses (including user subclasses) that happen to define a function or property with the same name.

comment:4 Changed 7 years ago by bill

PS: Two other observations:

1) Normally copying an attribute to the DOMNode is harmless.

2) If version 1.7 had instead explicitly added customer setters to _WidgetBase like below, wouldn't you have the same problem?

_setClassAttr: function(val){ this.domNode.className = val; }

comment:5 in reply to:  4 ; Changed 7 years ago by Randy Hudson

Replying to bill:

2) If version 1.7 had instead explicitly added customer setters to _WidgetBase like below, wouldn't you have the same problem?

_setClassAttr: function(val){ this.domNode.className = val; }

In that case, "class[Name]" would probably have been documented and/or declared as a property of _WidgetBase. Maybe if my "subclass" redeclared the same property, I would hope to see at least a warning in the console.

What I quoted in the ticket description is how the implementation works. The actual behavior varies depending on the browser and the type of element. For example, a DIV has:

131 properties on Safari 6.0.4 195 properties on Firefox 21.0 254 properties on IE9

What the behavior of _WidgetBase if my widget has a boolean property called "draggable" set to true? It does nothing in Firefox (yet), nothing in IE, and used to do nothing in Safari 5. But then, when Safari 6 came out, suddenly my widget is being dragged around with the mouse.

comment:6 Changed 7 years ago by cjolif

Is this behavior clearly documented? Do we say somewhere that if you have a property named against an HTML attribute it will be copied into the DOM?

comment:7 in reply to:  5 ; Changed 7 years ago by bill

Replying to randyhudson:

Replying to bill:

2) If version 1.7 had instead explicitly added customer setters to _WidgetBase like below, wouldn't you have the same problem?

_setClassAttr: function(val){ this.domNode.className = val; }

In that case, "class[Name]" would probably have been documented and/or declared as a property of _WidgetBase. Maybe if my "subclass" redeclared the same property, I would hope to see at least a warning in the console.

Unfortunately there's no warning; since it's normal for subclasses to override things in the superclass, it's hard to print a warning.


What I quoted in the ticket description is how the implementation works. The actual behavior varies depending on the browser and the type of element. For example, a DIV has:

131 properties on Safari 6.0.4 195 properties on Firefox 21.0 254 properties on IE9

That's a lot of properties. Plus, you aren't even including the hundreds of aria attributes like listed in http://www.w3.org/TR/wai-aria/states_and_properties#state_prop_values. You can hopefully see why we didn't want to add 254 custom setters into _WidgetBase.js.

About behavior varying by browser, that's true, but imagine if we had custom setters for all 254 properties, it's the same thing.


What the behavior of _WidgetBase if my widget has a boolean property called "draggable" set to true? It does nothing in Firefox (yet), nothing in IE, and used to do nothing in Safari 5. But then, when Safari 6 came out, suddenly my widget is being dragged around with the mouse.

That's too bad. Again though, keep in mind that:

1) the same problem would have happened if we had added custom setter to _WidgetBase.js to copy "draggable" to the DOMNode

2) the same problem would have happened for non-templated widgets used with the parser like below, since the draggable=true attribute on the DOMNode is never erased:

<div dojoType=MyWidget draggable=true>


Replying to cjolif:

Is this behavior clearly documented? Do we say somewhere that if you have a property named against an HTML attribute it will be copied into the DOM?

Yes, actually it's part of the API doc for _WidgetBase:

//
//		If no custom setter is defined for an attribute, then it will be copied
//		to this.focusNode (if the widget defines a focusNode), or this.domNode otherwise.
//		That's only done though for attributes that match DOMNode attributes (title,
//		alt, aria-labelledby, etc.)

comment:8 in reply to:  7 ; Changed 7 years ago by Randy Hudson

Replying to bill:

That's a lot of properties. Plus, you aren't even including the hundreds of aria attributes like listed in http://www.w3.org/TR/wai-aria/states_and_properties#state_prop_values. You can hopefully see why we didn't want to add 254 custom setters into _WidgetBase.js.

About behavior varying by browser, that's true, but imagine if we had custom setters for all 254 properties, it's the same thing.

I don't really care whether custom setters are used or not. However, _WidgetBase should provide the same behavior across all supported browsers. If "draggable" can't be supported everywhere, I don't think a custom setter would have been added. Also, _WidgetBase needs to document its properties so subclasses can define properties without colliding.

The reality is that the current number of properties you can actually write to in a useful way across all of the supported browsers is pretty small (something like 10-20). The number of "simple" custom setters that would have to be added is probably no more than the number of places where this behavior has to be undone. Even _WidgetBase has:

	_setOwnerDocumentAttr: function(val){
		// this setter is merely to avoid automatically trying to set this.domNode.ownerDocument
		this._set("ownerDocument", val);
	},

_WidgetBase already has a baseClass property (and "class"?). If you set className, it ends up overwriting baseClass. Why have different properties that do the same thing?

comment:9 in reply to:  8 Changed 7 years ago by bill

Replying to randyhudson:

The reality is that the current number of properties you can actually write to in a useful way across all of the supported browsers is pretty small (something like 10-20).

It's more than that. As I said above, there are hundreds of aria properties. They work across all browsers.

_WidgetBase already has a baseClass property (and "class"?). If you set className, it ends up overwriting baseClass. Why have different properties that do the same thing?

They are different. baseClass is used to generate classes depending on state, such as dijitTextBoxHovered or dijitTreeNodeSelected.

comment:10 Changed 6 years ago by bill

Resolution: wontfix
Status: newclosed

Turns out this issue will be moot in 2.0. In 2.0, the widget and the DOMNode will be the same thing rather than separate objects (corresponding to the proposed web components standards), so naturally setting className etc. on the widget will affect the DOMNode.

comment:11 Changed 6 years ago by Randy Hudson

The last comment sounds extremely optimistic to me. Are you saying that in 2.0, no widgets will have any attributes related to their behavior, or anything else that's not directly set on its top-level DOMNode? Not to mention the fact that that "standard" is 10 years away from being implemented in all browsers

comment:12 Changed 6 years ago by bill

I think you misunderstood. We will react to properties set on the top DOMNode using custom setters, and do whatever we need to. It works in all modern browsers today, so no need to wait for 10 years.

comment:13 in reply to:  4 ; Changed 6 years ago by manklu

Just my 2 cents. Maybe someone reading it.

Replying to bill:

2) If version 1.7 had instead explicitly added customer setters to _WidgetBase like below, wouldn't you have the same problem?

_setClassAttr: function(val){ this.domNode.className = val; }

There is one big difference.

If you add a new method, it breaks when I decide to use a new Dojo Version. Now it breaks when our Customers decide to update the browser.

And don't tell me that they should omit security fixes.

comment:14 in reply to:  13 Changed 6 years ago by dylan

Cc: Douglas Hays removed
Milestone: 2.01.10
Resolution: wontfix
Status: closedreopened

Replying to manklu:

There is one big difference.

If you add a new method, it breaks when I decide to use a new Dojo Version. Now it breaks when our Customers decide to update the browser.

And don't tell me that they should omit security fixes.

To make sure I understand this issue, you are saying that when users upgrade to new browser versions, if that browser happens to set an HTML5 attribute identical to one chosen by Dijit as a property name, then things will break because of unexpected behavior being passed into the widget constructor?

If that's the case, given the current Dijit architecture, I think this is going to be a painful. We could backport changes each time a browser comes out with new attribute values, or we could make this auto copying of properties be something that is optional (though in the case of the latter, we could perhaps do that for 1.10, but couldn't backport that change to earlier releases).

comment:15 Changed 6 years ago by Randy Hudson

I thought one of the selling points of Dojo was to isolate developers from the differences between browsers, and even different versions of the same browser. This behavior goes against that goal.

What's even more disappointing, though, is that dojo decides to change course when there was already a working and superior mechanism for mapping properties to the DOM.

Dojo consumers are constantly forced to adopt new ways to do the same thing, even when that new way is no better, or worse, than before.

comment:16 Changed 6 years ago by dylan

Priority: undecidedhigh

I agree with your sentiment... we're not perfect, even though we try to be.

I know this was a long debated and discussed issue, and I don't remember all of the details as it's been a while, but it's probably worth revisiting as the current situation is not ideal.

comment:17 Changed 6 years ago by dylan

Milestone: 1.101.11

comment:18 Changed 6 years ago by bill

I can see why you're disappointed, but it doesn't make sense to change this now, for two big reasons:

  1. The only way to remove the auto-mapping yet maintain backwards compatibility is to add explicit custom setters to _WidgetBase for every existing DOM property / attribute (ex: draggable, value, min, max, aria-labelledby, aria-label, role, etc.). Then we basically end up in the same situation we're in now, where widget properties are unwantedly mapped to the DOM node.
  1. 2.0 will work the same way that dijit does now. Since in 2.0 the Widget == the DOM node, setting a property on the widget is literally the same thing as setting a property on the DOM node, and therefore unfortunately named widget properties (ex: draggable) will run into trouble.

So, if 1.10 and 2.0 work the same way, should 1.11 really work differently?

Last edited 6 years ago by bill (previous) (diff)

comment:19 in reply to:  18 Changed 6 years ago by Randy Hudson

Explicit attributeMap entries/custom setters are the only thing that makes sense. The result is a consistent and predictable behavior that doesn't change when between browsers, over time, or if the DOM element type is changed. I don't see how this could be confused this with the current behavior.

I don't know anything about what's coming in 2.0, but the only way that the above statement could be true would be if widgets could no longer have properties that are not reflected in their DOM.

comment:20 Changed 6 years ago by bill

Explicit attributeMap entries/custom setters ... behavior that doesn't change when if the DOM element type is changed.

Well, that's backwards incompatible with how it works now, and how 2.0 will work.

I don't know anything about what's coming in 2.0

You should google "custom elements", and you'll see why your statement about "reflected in their DOM" has no meaning. I tried to explain above but you didn't understand it, so I suggest reading the tutorials others have written.

comment:21 in reply to:  20 Changed 6 years ago by manklu

Replying to bill:

You should google "custom elements"

Please, tell me the standard document with the requirement to pollute the dom node. You need one attribute to hold the widget context, but that is more than enough.

and you'll see why your statement about "reflected in their DOM" has no meaning.

It makes perfect sense. If the separation of content and presentation is taken into account, most of the values are for nodes in the shadow dom. I don't see less setters and getters when all attributes are in the content node.

If the separation of content and presentation is not taken into account, I should leave now. dijit will be dead sooner or later.

Well, that's backwards incompatible with how it works now, and how 2.0 will work.

And the current behaviour is not backwards incompatible? I have upgraded from 1.6 to 1.9 and would say: One incompatible change more or less, who cares.

Personally I would never try to migrate dijit to web components. The live cycle is too different. All I would do is an extension for the parser, to define a mapping between custom tags and dijit widgets.

I would make the custom elements from scratch, without cooperative redering and with strict separation of content and presentation. And if communication between some of the components is necessary (e.g. StackContainer? and StackController?), you can still define an Api with custom attributes and events.

And if you use one of the polyfills, you may get a shiny new dijit which works in many browsers.

I know that there were already discussions. But I think the current direction is ... (No, I don't say the word)

Changed 6 years ago by manklu

Attachment: sample.zip added

A small inspiration. It's terrible incomplete and has many loose ends. But it seems to work and should be usable. The biggest missing part is the integration of a polyfill.

comment:22 Changed 4 years ago by dylan

Milestone: 1.111.12

comment:23 Changed 3 years ago by dylan

Milestone: 1.131.15

Ticket planning... move current 1.13 tickets out to 1.15 to make it easier to move tickets into the 1.13 milestone.

Note: See TracTickets for help on using tickets.