Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSSStyleDeclaration.getPropertyValue() shall not return null #2154

Closed
DartBot opened this issue Mar 14, 2012 · 19 comments
Closed

CSSStyleDeclaration.getPropertyValue() shall not return null #2154

DartBot opened this issue Mar 14, 2012 · 19 comments
Assignees
Labels
web-libraries Issues impacting dart:html, etc., libraries
Milestone

Comments

@DartBot
Copy link

DartBot commented Mar 14, 2012

This issue was originally filed by @tomyeh


What steps will reproduce the problem?

  1. If toggling the style's border as follows, it causes a NullPointerException
    label.on.click.add((event) {
      event.target.style.border =
        event.target.style.border.isEmpty() ? "1px solid blue": "";
    });
  2. A simplified case:
    print("${node.style.border}"); //an empty string
    node.style.border = "1px solid blue";
    node.style.border = "";
    print("${node.style.border}"); //Wrong! it shall not be null

What is the expected output? What do you see instead?
getPropertyValue() shall always returns an empty string.

What version of the product are you using? On what operating system?
Dartium 125150

@DartBot
Copy link
Author

DartBot commented Mar 14, 2012

This comment was originally written by [email protected]


cc @jacob314.
Added Area-UI, Triaged labels.

@vsmenon
Copy link
Member

vsmenon commented Apr 13, 2012

Removed Area-UI label.
Added Area-DOM label.

@vsmenon
Copy link
Member

vsmenon commented Jul 12, 2012

Added this to the M1 milestone.

@iposva-google
Copy link
Contributor

Removed Area-DOM label.
Added Area-HTML label.

@vsmenon
Copy link
Member

vsmenon commented Aug 23, 2012

Removed Area-HTML label.
Added Area-Dartium label.

@DartBot
Copy link
Author

DartBot commented Aug 24, 2012

This comment was originally written by [email protected]


Set owner to [email protected].

@DartBot
Copy link
Author

DartBot commented Aug 28, 2012

This comment was originally written by [email protected]


WebKit DOM IDLs explicitly say that null values returned by CSSStyleDeclaration.getPropertyValue should translate to null.

Is there any additional considerations? Should we generalize this to other methods?


Added Invalid label.

@DartBot
Copy link
Author

DartBot commented Aug 29, 2012

This comment was originally written by @tomyeh


Sorry, as described http://www.w3.org/2003/01/dom2-javadoc/org/w3c/dom/css/CSSStyleDeclaration.html#getPropertyValue_java.lang.String_, it shall be an empty string. Which document do you refer to?

@DartBot
Copy link
Author

DartBot commented Aug 29, 2012

This comment was originally written by [email protected]


Dart rather follows JavaScript than Java in this regard, see http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSStyleDeclaration.idl#L39 which is our primary source for code generation.

Still, if you believe there is a reason to default to empty string than to null, we can override this.

I am concerned though for people coming from JS.

@DartBot
Copy link
Author

DartBot commented Aug 29, 2012

This comment was originally written by @tomyeh


Agree to be compatible with JavaScript, but the following doesn't match:

//JS
document.document.getElementById("aa").style.width; //return an empty string

//Dart
document.query("#aa").style.width; //return null

@DartBot
Copy link
Author

DartBot commented Aug 29, 2012

This comment was originally written by @tomyeh


On the other hand, getPropertyValue returns null in JS (and Dart). The difference between getPropertyValue("width") and width makes me wondering if it is a good spec.

@DartBot
Copy link
Author

DartBot commented Aug 29, 2012

This comment was originally written by @tomyeh


In additions, getPropertyValue shall return the value that was set by setProperty. It is easy to cause unpleasant surprise if the getter doesn't return the same value set to the setter.

BTW, I tested it a bit on different browsers. Both Opera and Firefox return an empty string (while Webkit browser returns null).

@DartBot
Copy link
Author

DartBot commented Aug 29, 2012

This comment was originally written by [email protected]


tom.yeh.m, I am definitely with you that difference between similar methods and esp. across browsers is suspicious.

But Dartium being WebKit based browser, we try hard to be closer to Chromium/Safari.

@DartBot
Copy link
Author

DartBot commented Aug 30, 2012

This comment was originally written by @tomyeh


Even though, style.width shall return an empty string, not null. It is still a bug.

@DartBot
Copy link
Author

DartBot commented Aug 30, 2012

This comment was originally written by [email protected]


tom.yeh.m,

getters like width and alike are just shortcuts for getProperty('width').

Why we should introduce a subtle semantic difference here?

@DartBot
Copy link
Author

DartBot commented Aug 31, 2012

This comment was originally written by @tomyeh


That is what I tried to raise my point if you read my comments closely. As I described in the previous comments, Webkit decides to return an empty string for style.width, while null for getProperty.

Therefore, there are two choices. Stick with Webkit and, as you said, be closer to Chromium/Safari. Then, style.width shall return an empty string.

Or, to have a consistent api. Like w3c.org, return an empty string for both cases.

In short, the current implementation in Dart has to change to adapt one of the above.

Here is the code you can verify the behavior of Chrome/Safari or whatever:

<html>
<head>
<title></title>
</head>
<body>
  <div id="a"/>
  <script>
  console.log("0>" + document.getElementById("a").style.width);
  console.log("1>" + document.getElementById("a").style.getPropertyValue("width"));
  document.getElementById("a").style.setProperty("width", "", "");
  console.log("2>" + document.getElementById("a").style.getPropertyValue("width"));
  </script>
</body>
</html>

@DartBot
Copy link
Author

DartBot commented Aug 31, 2012

This comment was originally written by [email protected]


Let's discuss it in wider context of other dart:html implementations.

Stephen, Vijay, any thoughts?


cc @rakudrama.
cc @vsmenon.
Removed the owner.
Removed Area-Dartium label.
Added Area-HTML, Waiting labels.

@vsmenon
Copy link
Member

vsmenon commented Sep 5, 2012

Set owner to @blois.
Added Triaged label.

@blois
Copy link
Contributor

blois commented Sep 6, 2012

Quick recap:

For unset CSS properties there are discrepancies in the return value of style.getPropertyValue:

In addition, Dart's API mimics the style.propertyName syntax, which returns empty string.

@DartBot DartBot added Type-Defect web-libraries Issues impacting dart:html, etc., libraries labels Sep 6, 2012
@DartBot DartBot added this to the M1 milestone Sep 6, 2012
copybara-service bot pushed a commit that referenced this issue Jun 26, 2023
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/4757339..e04a6b3):
  e04a6b30  2023-06-23  Sam Rawlins  Remove grind_test; not important (#3453)

ecosystem (https://github.com/dart-lang/ecosystem/compare/a2dac18..b1056e6):
  b1056e6  2023-06-13  Nate Bosch  Allow prerelease suffix in CHANGELOG versions (#112)

leak_tracker (https://github.com/dart-lang/leak_tracker/compare/2149e5c..c75b0a7):
  c75b0a7  2023-06-23  Yii Chen  make LeakTrackingConfiguration to const (#83)

webdev (https://github.com/dart-lang/webdev/compare/6fe17fe..8360d50):
  8360d50f  2023-06-22  Elliott Brooks  Shard MV2 and MV3 extension tests (#2153)
  12e8aa2d  2023-06-22  Elliott Brooks  Reset Webdev to 3.0.6-wip (#2154)
  8ca3a891  2023-06-22  Elliott Brooks  Fix the Puppeteer tests for the Debug Extension (#2152)

Change-Id: I8f9ec79d99f375898b3ecd3330512fd39579f37b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/311462
Reviewed-by: Konstantin Shcheglov <[email protected]>
Auto-Submit: Devon Carew <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
web-libraries Issues impacting dart:html, etc., libraries
Projects
None yet
Development

No branches or pull requests

5 participants