:dir() support #4880

dfreedm · 2017-10-11T17:57:53Z

Reference Issue

Polymer 2.x fix for #4860

Goal

Allows for native and polyfilled ShadowDOM elements to use :dir CSS selector to provide text directionality based layouts.

Details

Modifies Polymer.ElementMixin to provide override points for stylesheet modification.
Elements can opt out of the global page text direction by setting the dir attribute directly in ready().
Automatic left-to-right or right-to-left styling is sync'd with the <html> element
Applications must set <html dir="ltr"> or <html dir="rtl"> to sync direction
Opting out of the global direction styling is permanent

Related to #4860

ShadyCSS version of the :dir transform is not good enough :(

Adapt scoped keyframes test for Safari 11 still failing on https://bugs.webkit.org/show_bug.cgi?id=166748

sorvell · 2017-10-11T23:09:22Z

lib/mixins/dir-mixin.html

+    function setRTL(instance) {
+      if (!instance.__origRTLStatus) {
+        const el = /** @type {!HTMLElement} */(instance);
+        if (document.documentElement.getAttribute('dir') === 'rtl') {


Let's capture this state so we can make this application function faster.

sorvell · 2017-10-11T23:10:18Z

lib/mixins/dir-mixin.html

+     * Element class mixin that allows elements to use the `:dir` CSS Selector to have
+     * text direction specific styling.
+     *
+     * With this mixin, any stylesheet provided in the template will be transform `:dir` into


will transform

sorvell · 2017-10-11T23:11:32Z

lib/mixins/dir-mixin.html

+     * directly in `ready()`.
+     *
+     * Caveats:
+     * - Automatic left-to-right or right-to-left styling is sync'd with the `<html>` element


<html> element only, setting the dir attribute on an element further up the DOM tree will have no effect.

sorvell · 2017-10-11T23:13:05Z

lib/mixins/dir-mixin.html

+     *
+     * Caveats:
+     * - Automatic left-to-right or right-to-left styling is sync'd with the `<html>` element
+     * - Applications must set `<html dir="ltr">` or `<html dir="rtl">` to sync direction


Let's put this as the first caveat and then say that changing dir on html is supported.

sorvell · 2017-10-11T23:13:31Z

lib/mixins/dir-mixin.html

+     * `:host([dir])` and sync direction with the page via the element's `dir` attribute.
+     *
+     * Elements can opt out of the global page text direction by setting the `dir` attribute
+     * directly in `ready()`.


or in HTML?

sorvell · 2017-10-11T23:18:31Z

lib/mixins/dir-mixin.html

+       * @extends {base}
+       * @implements {Polymer_ElementMixin}
+       */
+      const elementBase = Polymer.ElementMixin(base);


What about removing this dependency and requiring users to at least also use PropertyAccessors?

sorvell · 2017-10-11T23:25:25Z

lib/mixins/element-mixin.html

-          finalizeTemplate(/** @type {!PolymerElement} */(this.__proto__), this._template, baseURI,
-            /**@type {!HTMLElement}*/(this).localName);
-        }
+        this.constructor._finalizeTemplate(/** @type {!PolymerElement} */(this.__proto__), this._template,


Since this is a static now, let's not pass in the prototype.

sorvell · 2017-10-11T23:31:11Z

lib/mixins/dir-mixin.html

+         * @override
+         * @suppress {missingProperties} Interfaces in closure do not inherit statics, but classes do
+         */
+        static _processStyleText(is, template, baseURI) {


Let's only enable the runtime element support if we find dir in the css.

sorvell · 2017-10-11T23:34:45Z

lib/mixins/dir-mixin.html

+        /**
+         * @suppress {invalidCasts} Closure doesn't understand that `this` is an HTMLElement
+         */
+        ready() {


Let's make these runtime features opt-in via a flag.

Not sure if this is still needed if we use the presence of :dir to activate.

Seems ok. They are different and an element could have either or both.

sorvell · 2017-10-11T23:35:15Z

lib/mixins/dir-mixin.html

+     * @appliesMixin Polymer.ElementMixin
+     * @memberof Polymer
+     */
+    Polymer.DirMixin = Polymer.dedupingMixin((base) => {


Let's mix this into LegacyElement so we have hybrid support since 1.x has dir support now.

- Extend PropertyAccessors for greater flexibility - Detect if `:dir` is used in order to activate runtime features - Add to LegacyElement for 1.x compatibility - clean up `ElementMixin._finalizeTemplate`

sorvell · 2017-10-14T00:09:14Z

lib/mixins/dir-mixin.html

+    const DIR_INSTANCES = [];
+
+    /** @type {MutationObserver} */
+    let OBSERVER = null;


Let's not all caps for variables?

sounds good

sorvell · 2017-10-14T00:11:51Z

lib/mixins/dir-mixin.html

+        /**
+         * @suppress {invalidCasts} Closure doesn't understand that `this` is an HTMLElement
+         */
+        ready() {


Seems ok. They are different and an element could have either or both.

sorvell · 2017-10-14T00:13:56Z

lib/mixins/dir-mixin.html

+        /** @suppress {missingProperties} Closure's understanding of prototypes is bad */
+        connectedCallback() {
+          if (elementBase.prototype.connectedCallback) {
+            /** @type {!Polymer_ElementMixin} */(elementBase).prototype.connectedCallback.call(this);


Why not super.connectedCallback() (and same for disconnected...)

PropertyAccessors doesn't have connectedCallback or disconnectedCallback

Specifically if you find it, call super. rather than .call.

sorvell · 2017-10-14T00:15:13Z

lib/mixins/dir-mixin.html

+            /** @type {!Polymer_ElementMixin} */(elementBase).prototype.connectedCallback.call(this);
+          }
+          if (this.constructor.__activateDir) {
+            takeRecords();


Don't think this will fly (same in disconnected). We cannot update all elements as takeRecords does when one is connected

Ok, just means I'll have to pull document direction on connected.

sorvell · 2017-10-14T00:17:50Z

lib/mixins/dir-mixin.html

+subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt
+-->
+<link rel="import" href="../utils/style-gather.html">
+<link rel="import" href="../utils/resolve-url.html">


Doesn't seem like the imports above are used, remove?

dfreedm added 4 commits October 5, 2017 15:59

First draft of a :dir aware element mixin

2ef65aa

Related to #4860

remove bogus semicolon

7d044b4

Always do the :dir transform

31c0ebc

ShadyCSS version of the :dir transform is not good enough :(

Fix linting

624189a

Adapt scoped keyframes test for Safari 11 still failing on https://bugs.webkit.org/show_bug.cgi?id=166748

dfreedm requested a review from sorvell October 11, 2017 17:57

googlebot added the cla: yes label Oct 11, 2017

add some description of the dir mixin

f98ad11

sorvell suggested changes Oct 11, 2017

View reviewed changes

Address feedback

e71b84a

- Extend PropertyAccessors for greater flexibility - Detect if `:dir` is used in order to activate runtime features - Add to LegacyElement for 1.x compatibility - clean up `ElementMixin._finalizeTemplate`

sorvell reviewed Oct 14, 2017

View reviewed changes

dfreedm added 2 commits October 13, 2017 17:40

more feedback

15cbdff

super it and put back takeRecords

02e2f14

dfreedm merged commit e9e2cda into master Oct 16, 2017

dfreedm deleted the native-dir branch October 16, 2017 17:58

dfreedm mentioned this pull request Oct 16, 2017

:dir support for 1.x and native ShadowDOM 2.x #4860

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

:dir() support #4880

:dir() support #4880

dfreedm commented Oct 11, 2017

sorvell Oct 11, 2017

sorvell Oct 11, 2017

sorvell Oct 11, 2017

sorvell Oct 11, 2017

sorvell Oct 11, 2017

sorvell Oct 11, 2017

sorvell Oct 11, 2017

sorvell Oct 11, 2017

dfreedm Oct 13, 2017

sorvell Oct 11, 2017

dfreedm Oct 13, 2017

sorvell Oct 14, 2017

sorvell Oct 11, 2017

sorvell Oct 14, 2017

dfreedm Oct 14, 2017

sorvell Oct 14, 2017

sorvell Oct 14, 2017

dfreedm Oct 14, 2017

sorvell Oct 14, 2017

dfreedm Oct 14, 2017

sorvell Oct 14, 2017

dfreedm Oct 14, 2017

sorvell Oct 14, 2017

:dir() support #4880

:dir() support #4880

Conversation

dfreedm commented Oct 11, 2017

Reference Issue

Goal

Details

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

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

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