Opened 5 years ago

Closed 3 years ago

#9504 closed Bug (fixed)

[FF][Chrome] attribute.specified is deprecated

Reported by: Reinmar Owned by: pjasiun
Priority: Normal Milestone: CKEditor 4.4.1
Component: General Version: 3.0
Keywords: Firefox Cc:

Description (last modified by Reinmar)

  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.

Attachments (1)

9504-specified.html (482 bytes) - added by pjasiun 3 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by Reinmar

  • Description modified (diff)

comment:2 Changed 5 years ago by Reinmar

  • Description modified (diff)

comment:3 Changed 5 years ago by j.swiderski

  • Keywords DOM removed
  • Status changed from new to confirmed

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.

comment:4 Changed 5 years ago by j.swiderski

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

comment:5 Changed 3 years ago by wwalc

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.

comment:6 Changed 3 years ago by Reinmar

  • Milestone set to CKEditor 4.4.1

comment:7 Changed 3 years ago by wwalc

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.

comment:8 Changed 3 years ago by pjasiun

  • Owner set to pjasiun
  • Status changed from confirmed to assigned

Changed 3 years ago by pjasiun

comment:9 Changed 3 years ago by pjasiun

  • Status changed from assigned to review

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.

comment:10 Changed 3 years ago by pjasiun

  • Status changed from review to assigned
  • Summary changed from [FF] Possible bug in element#copyAttributes because attribute.specified always is true to [FF][Chrome] attribute.specified is deprecated

Since version 34 specified is also deprecated on Chrome

comment:11 Changed 3 years ago by pjasiun

  • Status changed from assigned to review

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

Changes in t/9504.

comment:12 Changed 3 years ago by Reinmar

  • Status changed from review to review_failed
  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.

comment:13 Changed 3 years ago by pjasiun

  • Status changed from review_failed to review

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.

comment:14 Changed 3 years ago by Reinmar

  • Status changed from review to review_failed

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.

  1. 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.

comment:15 Changed 3 years ago by Reinmar

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.

comment:16 Changed 3 years ago by pjasiun

  • Status changed from review_failed to review

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.

Last edited 3 years ago by pjasiun (previous) (diff)

comment:17 Changed 3 years ago by Reinmar

  • Status changed from review to review_passed

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

comment:18 Changed 3 years ago by pjasiun

Done and closed.

comment:19 Changed 3 years ago by pjasiun

  • Resolution set to fixed
  • Status changed from review_passed to closed
Note: See TracTickets for help on using tickets.
© 2003 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy