fix(platform-server): generate correct stylings for camel case names #22263

CaerusKaru · 2018-02-16T21:04:13Z

Add correct mapping from camel case to kebab case for CSS style
names
Remove internal CSS methods in favor of native Domino APIs

Fixes #19235

CaerusKaru · 2018-02-16T21:53:26Z

Toxicable · 2018-02-16T21:56:42Z

packages/platform-server/package.json

    "tslib": "^1.7.1",
-    "domino": "^1.0.29",


Does Domino use semantic versioning? If so then this PR is also a breaking change

Domino uses "semantic versioning". The difference between v1 and v2 is an internal change in how they represent DOM nodes (they switched to a linked list implementation) and they added a fix for namespacing issues.

If we want to consider this a breaking change, I can add that to the PR description, but that just makes the timing of the PR even better with v6 in beta.

vicb · 2018-02-26T18:06:24Z

packages/platform-server/src/domino_adapter.ts

  getStyle(element: any, styleName: string): string {
-    const styleMap = this._readStyleAttribute(element);
-    return styleMap.hasOwnProperty(styleName) ? (styleMap as any)[styleName] : '';
+    return element.style[styleName] || element.style.getPropertyValue(styleName) || '';


remove the last || '' ?
https://github.com/fgnass/domino/blob/master/lib/CSSStyleDeclaration.js#L99

vicb · 2018-02-26T18:12:08Z

packages/platform-server/src/domino_adapter.ts

-    (styleMap as any)[styleName] = styleValue;
-    this._writeStyleAttribute(element, styleMap);
+    styleName = styleName.replace(/([a-z])([A-Z])/g, '$1-$2').toLowerCase();
+    element.style[styleName] = styleValue;


is it ok to access style[name] directly in domino ?
seems like there is style.properties[name]
What about using setPropertyValue instead ?

Yes, it's fine. It follows the spec for CSSStyleDeclaration, which allows for string indexing

@CaerusKaru What spec are you talking about ?
I see no mention of [] indexing in

https://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleDeclaration

https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration

https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration

@vicb The indexing is by number, not string as I originally thought. The item operator indicates number aliasing, like with NodeList.

In the case of platform-server, which is using an out-of-date form of CSSStyleDeclaration, Domino does not provide us with enough cover to use setPropertyValue and getPropertyValue. It needs to be updated to include modern CSS style properties. This change gets around that requirement for now by just directly indexing. While not spec compliant, it's a little more palatable than just writing huge style chunks as was previous. Plus if Domino does get updated in the near future, we can just replace the one-liners with their compatible get/setPropertyValue forms.

I agree it's not ideal, but we're a little handicapped by our server DOM implementation, which works well for just about everything else.

Ok wait, I found it! I knew I wasn't going crazy. In the CSSOM spec, found here:

partial interface CSSStyleDeclaration { [CEReactions] attribute [TreatNullAs=EmptyString] CSSOMString _camel_cased_attribute; };

Which in JavaScript means that for all possible camel-cased attributes, you have string indexing allowed.

vicb · 2018-02-26T18:20:43Z

Seems like el.stlye is a CSSStyleDeclaration then I wonder if accessing el.style[name] is valid ?

* Add correct mapping from camel case to kebab case for CSS style names * Remove internal CSS methods in favor of native Domino APIs Fixes angular#19235

CaerusKaru · 2018-02-26T18:55:59Z

@vicb As stated above, using string indexing is totally valid for CSSStyleDeclaration, which Domino implements.

CaerusKaru · 2018-02-26T19:25:04Z

@vicb Ok I've done a deeper dive into the issue, and here's where I've ended up. If you set a property that isn't specifically enumerated by Domino (which at this point is a little outdated re flex and grid, etc), it gets stored raw on the instance of the CSSStyleDeclaration. That means that if someone applies their own styles outside of the Angular renderer, the renderer may not pick up the styles if it just relies on get/setPropertyValue. Likewise, it means that using setPropertyValue won't allow for string index access for undefined properties (e.g. flex, grid) later on, which leads to undefined behavior.

The solution here essentially bypasses the Domino internal implementation and instead relies on the contract for CSSStyleDeclaration

…22263) * Add correct mapping from camel case to kebab case for CSS style names * Remove internal CSS methods in favor of native Domino APIs Fixes #19235 PR Close #22263

…ngular#22263) * Add correct mapping from camel case to kebab case for CSS style names * Remove internal CSS methods in favor of native Domino APIs Fixes angular#19235 PR Close angular#22263

jimjim2a · 2018-03-01T12:14:39Z

Since this update, it's no longer possible to apply background-image correctly on server, it is totally dropped.
It used to work properly on 5.2.6 but not on 5.2.7

* Partially reverts angular#22263 due to lack of total spec complicance on the server * Maintains the camel-case styles fix

CaerusKaru · 2018-03-01T15:33:25Z

@jimjim2a Thank you for pointing that out. I've put together a partial revert PR in #22527 that fixes that issue and adds a unit test to make sure it doesn't get introduced again.

…ngular#22263) * Add correct mapping from camel case to kebab case for CSS style names * Remove internal CSS methods in favor of native Domino APIs Fixes angular#19235 PR Close angular#22263

* Partially reverts angular#22263 due to lack of total spec compliance on the server * Maintains the camel-case styles fix PR Close angular#22527

angular-automatic-lock-bot · 2019-09-13T09:22:50Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Feb 16, 2018

CaerusKaru force-pushed the style branch 2 times, most recently from de930c7 to c0c5f48 Compare February 16, 2018 21:13

Toxicable reviewed Feb 16, 2018

View reviewed changes

This was referenced Feb 22, 2018

Using xmlns and version on svg elements breaks builds angular/universal#906

Closed

[Universal] NAMESPACE_ERR (14): the operation is not allowed by Namespaces in XML crisbeto/angular-svg-round-progressbar#164

Open

CaerusKaru force-pushed the style branch 2 times, most recently from 9d8635c to cd75881 Compare February 23, 2018 19:33

vikerman approved these changes Feb 26, 2018

View reviewed changes

vikerman added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Feb 26, 2018

vicb reviewed Feb 26, 2018

View reviewed changes

fix(platform-server): generate correct stylings for camel case names

0e66578

* Add correct mapping from camel case to kebab case for CSS style names * Remove internal CSS methods in favor of native Domino APIs Fixes angular#19235

CaerusKaru force-pushed the style branch from cd75881 to 0e66578 Compare February 26, 2018 18:55

alexeagle added the area: server Issues related to server-side rendering label Feb 26, 2018

chuckjaz approved these changes Feb 27, 2018

View reviewed changes

alexeagle closed this in 40ba009 Feb 27, 2018

CaerusKaru deleted the style branch February 27, 2018 02:45

CaerusKaru mentioned this pull request Mar 1, 2018

fix(platform-server): add styles to elements correctly #22527

Closed

CaerusKaru added a commit to CaerusKaru/angular that referenced this pull request Mar 1, 2018

chore: partial revert of angular#22263

c77b576

* Partially reverts angular#22263 due to lack of total spec complicance on the server * Maintains the camel-case styles fix

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(platform-server): generate correct stylings for camel case names #22263

fix(platform-server): generate correct stylings for camel case names #22263

CaerusKaru commented Feb 16, 2018 •

edited

Loading

CaerusKaru commented Feb 16, 2018

Toxicable Feb 16, 2018

CaerusKaru Feb 16, 2018

vicb Feb 26, 2018

CaerusKaru Feb 26, 2018

vicb Feb 26, 2018

CaerusKaru Feb 26, 2018

vicb Feb 27, 2018

CaerusKaru Feb 27, 2018

CaerusKaru Feb 27, 2018

vicb commented Feb 26, 2018

CaerusKaru commented Feb 26, 2018

CaerusKaru commented Feb 26, 2018

jimjim2a commented Mar 1, 2018

CaerusKaru commented Mar 1, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

fix(platform-server): generate correct stylings for camel case names #22263

fix(platform-server): generate correct stylings for camel case names #22263

Conversation

CaerusKaru commented Feb 16, 2018 • edited Loading

CaerusKaru commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented Feb 26, 2018

CaerusKaru commented Feb 26, 2018

CaerusKaru commented Feb 26, 2018

jimjim2a commented Mar 1, 2018

CaerusKaru commented Mar 1, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

CaerusKaru commented Feb 16, 2018 •

edited

Loading