[FF][Chrome] attribute.specified is deprecated

[Piotrek Koszuliński]

1. Open stylesheetparser.html sample.
2. Focus editor by clicking on the line.
3. Apply first block style (h1.lightBlue).
4. Apply second block style (h3.green).

Warning is logged on console (note: native console, not Firebug):

[11:49:52.973] Use of attributes' specified attribute is deprecated. It always returns true. @ http://localhost/cksource/ckeditor-dev/core/dom/element.js:1648

I haven't noticed any issue following this warning, but it's worth checking.

[Jakub Ś]

According to ​https://developer.mozilla.org/en-US/docs/DOM/Attr, property specified is deprecated as of Gecko 7 and will be removed in the future.

Currently it always returns true in Firefox. We should probably think about some workaround for this browser.

[Jakub Ś]

Both v3 and v4 use it in couple of places in core/dome/element.js file.

[Wiktor Walc]

This thing gained my attention as well, I was about to report it then found this ticket, so +1 from me to fix this somehow to not scare other people.

[Wiktor Walc]

I just realized, that I in fact saw an error in a different place:

Use of attributes' specified attribute is deprecated. It always returns true. ​http://127.0.0.1/ckeditor-dev/core/selection.js Line 1595

​https://github.com/ckeditor/ckeditor-dev/blob/major/core/selection.js#L1595

To reproduce, simply click into wysiwygarea in the replacebycode sample.

[Piotr Jasiun]

On Firefox we do not need to check specified, because attributes contains only specified elements. On the other hand on IE 8 attributes contains not specified elements (see attachment), so we can not remove checking if attribute is specified and we should check it different way depending on the browser.

Changes in ​t/9504.

[Piotr Jasiun]

Since version 34 specified is also deprecated on Chrome

[Piotr Jasiun]

I removed checking specified on Webkit, checked tests on Chrome and Opera and everything seems to be fine.

Changes in ​t/9504.

[Piotrek Koszuliński]

1. The hasAttribute check in copyAttributes should be executed only on IEs.
2. Please write a test for the code added to hasAttribute - the || ( $attr.nodeValue && name == 'value' ) part.
3. The condition in hasAttribute may be simplified. Why do you have the last else part there, if first you check IE and then Gecko and Webkit. There's no more browsers. So it should look like:

if ( !$attr )
	return false;
else if ( CKEDITOR.env.ie ) {
	// IE BUG: value attribute is never specified even if it exists.
	return !!( $attr.specified || ( $attr.nodeValue && name == 'value' ) );
} else {
	// On other browsers specified property is deprecated and return always true,
	// but fortunately $.attributes contains only specified attributes.
	return true;
}

Note: I fixed 3 code style issues in this code.

[Piotr Jasiun]

I simplified hasAttribute and change copyAttribute so it calls hasAttribute only on IE.

I also prepared tests for the IE hack ($attr.nodeValue && name == 'value') but it looks like we do not need this hack (tests works fine without it). The only problem is that if value attribute has an empty string as a value then on IE 8 attribute does not exists, but this issue is not related with the hack.

I removed hack and pushed changes to ​t/9504 and corresponding test branch. Tests are only to prove that we do not need this hack anymore and will not be merged into master.

[Piotrek Koszuliński]

The tests check very little still.

1. There's no hasAttribute test for these two lines:

   else if ( CKEDITOR.env.ie )
       return $attr.specified;
   

Comment them out and you'll see some tests failing. You can get from them ideas how to write a test verifying the need for these two lines existence.

2. There's no test for copyAttributes whether this entire condition is needed:

   if ( !CKEDITOR.env.ie || this.hasAttribute( attrName ) )
   

I removed it (leaving the else {...} as it is) and all tests passed on IE8 and Chrome. But I think that this condition is really required - test should clarify this. Most likely the same kind of test as for 1. will be needed.

As for the implemented tests:

1. test_hasAttribute_value2 - this is unnecessary, because we don't need to test browser - we should check our API.
2. the commented out assertion from the first test may be removed. Indeed there's some bug in IE8, but we can ignore it.

[Piotrek Koszuliński]

As for the removed:

( $attr.nodeValue && name == 'value' )

Verify that all tests mentioned in ​http://dev.ckeditor.com/ticket/4527#comment:4 are ported to v4's tests.

[Piotr Jasiun]

Part of the tests from the #4527 (all from 2.html) were already in dt/core/dom/element/element.html. I ported rest of them as tt/4527/1.html, added more tests for hasAttribute and copyAttribute and rebased both branches.

[Piotrek Koszuliński]

Before closing, change a doctype in new test file to HTML5's.

[Piotr Jasiun]

Done and closed.

• ​git:997eff8
• tests:39046b9