search

ibm-js / delite

HTML Custom Element / Widget infrastructure
http://ibm-js.github.io/delite/
Other
68 stars 28 forks source link

Explicit setting of attribute dir #363

Closed semion1956 closed 9 years ago

semion1956 commented 9 years ago

Changes suggested to solve the problem from #362

wkeese commented 9 years ago

As I mentioned in #362, Widget.js already has code to set the d-rtl class on RTL widgets, so I don't think we need or want this change too.

semion1956 commented 9 years ago

d-rtl class is set only, if attribute dir is set to rtl (more exactly, it will be set, after you solve problem with shadow properties). If dir isn't set by user, d-rtl class isn't set too. So, after placing widget into rtl container, we will get mirrored widget with wrong styles even if css rules check only d-rtl. Alternatively I can suggest to set d-rtl class for newly created widget in accordance with the computed style of enclosing element (and as previously toggle it when dir attribute is changed)

wkeese commented 9 years ago

d-rtl class is set only, if attribute dir is set to rtl (more exactly, it will be set, after you solve problem with shadow properties)

Actually, d-rtl should be set if dir attribute is set to rtl OR the dir of <body> or <html> is set to rtl. When a widget is instantiated, Widget#postRender() executes:

this.notifyCurrentValue("dir", "baseClass");

which should cause an immediate call to refreshRendering() (by immediate I mean the next cycle, in ~1ms). Widget#refreshRendering() should then execute:

$(this).toggleClass("d-rtl", !this.isLeftToRight());

And isLeftToRight() does:

var doc = this.ownerDocument;
return !(/^rtl$/i).test(this.dir || doc.body.dir || doc.documentElement.dir);

Now, I know you want isLeftToRight() to act on the dir setting of any ancestor node rather than just <body> and <html>. That's a possible enhancement we could put in, but it also has a cost because in the common case, every widget needs to search up the DOM tree all the way to the <html> node. So that's something we'd have to consider carefully.

semion1956 commented 9 years ago

Currently notifyCurrentValue() doesn't work for dir - but it is not important here. If we add some widget without dir into rtl container from ltr page and vice versa (witch is not far-fetched case for bidi world), we get wrong result. Using getComputedStyle() once per widget will help us to avoid this problem. If we set dir explicitly in the time, when widget is attached to document, we at least get widget with predictable behavior (and isLeftToRight() will continue work correctly). Setting d-rtl is alternative solution, which require some additional changes in isLeftToRight()

semion1956 commented 9 years ago

Another proposal: if you think, that this can be considered as advanced bidi support, I can add these changes into Bidi.js (in general, rtl is supported without bidi flag).

wkeese commented 9 years ago

Currently notifyCurrentValue() doesn't work for dir

Ah right. OK, that should be fixed. Widget or CustomElement can override observe() something like:

observe: function (callback) {
    var propsToObserve = Object.create(this._ctor._propsToObserve);
    propsToObserve.dir = true;
    var h = new Stateful.PropertyListObserver(this, propsToObserve);
    h.open(callback, this);
    return h;
},

or alternately rather than calling notifyCurrentValue(), can call something like:

Observable.getNotifier(this).notify({
    type: "update",
    object: this,
    name: "dir",
    oldValue: ""
});

If we add some widget without dir into rtl container from ltr page and vice versa (witch is not far-fetched case for bidi world)), we get wrong result.

It still sounds farfetched to me, and even if we support that case, you will still get the wrong result if you change the dir attribute of an ancestor node after the widget has been attached. The workaround in both of these unusual cases is to explicitly set the widget's dir attribute.

Another proposal: if you think, that this can be considered as advanced bidi support, I can add these changes into Bidi.js (in general, rtl is supported without bidi flag).

I'll talk that over with the rest of the team. I'm still uneasy about degrading the performance even if it only applies for people that set the "bidi" flag to true.

wkeese commented 9 years ago

PS: In any case, it probably makes sense to replace the isLeftToRight() method with a computed property, either a string property named effectiveDir, or a boolean property named something like `effectiveLtr or effectiveRtl. Because currently, a widget state could become inconsistent in the unlikely case that the result of isLeftToRight() changed after the widget finished initializing.

semion1956 commented 9 years ago

It still sounds farfetched to me, and even if we support that case, you will still get the wrong result if you change the dir attribute of an ancestor node after the widget has been attached.

If I change dir attribute of an ancestor, I am responsible for changes in its content. We don't talk about and don't want any "automatic' changes. All what we need is correct dir and textDir initialization and possibility of their dynamic changes.

The workaround in both of these unusual cases is to explicitly set the widget's dir attribute.

Even if these cases look for you unusual, they exist, and we think, that related problems should be solved. However we think, that we can't require the user (or developer) to set explicitly dir attribute for any html element, added into some panel, which may have direction differ from direction of the page.

I'm still uneasy about degrading the performance

Are we both about calling getComputedStyle() one time per widget creating?

wkeese commented 9 years ago

We discussed this in our meeting last night. If you can add the code to Bidi.js (or a new file) and set it up to run w/a different has() flag, perhaps has("inherited-dir"), that will allow apps to turn it on/off as required.

My vision is that Widget.js will replace the isLeftToRight() method with:

effectiveDir: "ltr"

Then your new code can set effectiveDir based on this.dir and the inherited direction. You can have code in attachedCallback() as well as (pending #340) code in computeProperties(), like:

this.effectiveDir = this.dir || getComputedStyle(this).direction

or I suppose just

this.effectiveDir = getComputedStyle(this).direction

Then widgets can reference effectiveDir (rather than dir) in refreshRendering().

semion1956 commented 9 years ago

First of all, thank you for understanding our problems. However, there is another important addition that should be taken into account. HTML5 introduces one more possible value for attribute dir - 'auto' (already supported by FF and Chrome). So getComputedStyle().direction looks now as the only way to determine actual direction of the current html element (even in case, if its dir is defined explicitly). In this regard, it seems to me that additional has("inherited-dir") flag should not needed, because getComputedStyle() has no alternative. Finally, if we consider the use of this method as part of advanced bidi support, changes in the code may look as following: Widget.js:

...
                effectiveDir: "ltr",
...
        refreshDirection: function(root) {
            if (!root) {
                return;
            }
            this.findCustomElements(root).forEach(function(widget) {
                Observable.getNotifier(widget).notify({
                    type: "update",
                    object: this,
                    name: "effectiveDir",
                    oldValue: "none"
                });         
            });
        },
...
        attachedCallback: function () {
                ...
                this.refreshDirection(this);
                ...
                },
...
        _getDir: function () {
            var doc = this.ownerDocument;
            return this.dir || doc.body.dir || doc.documentElement.dir || "ltr";            
        },
        computeProperties: function (props) {
            if ("effectiveDir" in props) {
                this.effectiveDir = _getDir();
            }
        },
        refreshRendering: function (oldVals) {
                        ...
            if ("dir" in oldVals || (this.dir === "auto" && "textContent" in oldVals)) { 
                        // After problem with watching changes of shadow properties will be solved 
                this.refreshDirection(this);
            }
            if ("effectiveDir" in oldVals) {
                $(this).toggleClass("d-rtl", !this.isLeftToRight());
            }
        },
        isLeftToRight: function () {
            return this.effectiveDir === "ltr";
        },
...

And in Bidi.js:

...
        _getDir: function () {
            return getComputedStyle(this).direction;
        },
...

Is this suitable from your point of view?

wkeese commented 9 years ago

As I've said/implied before, we have no intention of supporting all the i18n features defined by the HTML spec, even with the bidi has() flags turned on. Two examples we aren't supporting are the lang attribute and responding to dynamic changes in the dir attribute of an ancestor node.

Likewise, we never said that we would support dir=auto. If we did support it, it would only be when has("bidi") is true, because the purpose of dir=auto is to deal with bidi text.

Also, we already said that we won't respond to dynamic changes to an ancestor's dir attribute, yet that's what you are trying to do by calling refreshDirection() from refreshRendering() (albeit only for ancestor nodes that are widgets)

Lesser points are:

  1. Your code above that calls Observable.getNotifier(widget).notify(...effectiveDir... doesn't really make sense, because you aren't changing the value of effectiveDir.
  2. computeProperties() should set effectiveDir if "dir" in props, not if "effectiveDir" in props.
  3. I meant to remove the isLeftToRight() method and replace it by effectiveDir
semion1956 commented 9 years ago

As I've said/implied before, we have no intention of supporting all the i18n features defined by the HTML spec, even with the bidi has() flags turned on.

We understand this and don't ask to implement all i18n features - but only those, that actually needed for our customers.

Two examples we aren't supporting are the lang attribute and responding to dynamic changes in the dir attribute of an ancestor node.

There are two different cases. "Dynamic changes in the dir attribute of an ancestor node" (more exactly - dynamic changes of the the direction of descendant widgets) is not a feature, but some implementation. We are dealing here with another feature - inheritance of direction - which currently is implemented only partially, and this is one of the missing parts. Current implementation doesn't prohibit the inheritance, but causes the wrong situation, when widget correctly directed, but its direction-related styles are wrong. We can't listen the changes, which performed out of Dojo, but we can offer the simple tool, which allows to solve this problem.

Likewise, we never said that we would support dir=auto. If we did support it, it would only be when has("bidi") is true, because the purpose of dir=auto is to deal with bidi text.

dir=auto depends on the text, but causes changes in the direction of elements. If you have <html> with dir=auto and 10 DIVs inside, direction of each of the DIV depends on the first strongly directed character found in one of the DIVs. Note: we are not against has("bidi") for this case but we don't see the difference (by sense) between support of rtl and auto.

Also, we already said that we won't respond to dynamic changes to an ancestor's dir attribute, yet that's what you are trying to do by calling refreshDirection() from refreshRendering() (albeit only for ancestor nodes that are widgets)

Here is another example, when partial implementation of some feature causes a wrong result. Current implementation 1) allows the widget to have and to change its own direction; 2) doesn't prohibit embedded custom widgets; 3) doesn't prohibit inheritance of direction; 4) it is aware of the changes in the ancestor - but for some reason doesn't inform descendants about these changes.

Your code above that calls Observable.getNotifier(widget).notify(...effectiveDir... doesn't really make sense, because you aren't changing the value of effectiveDir.

I change value of the effectiveDir in computeProperties(). Sense of the code that you write about is to initiate the recomputing rather than react on the changes.

computeProperties() should set effectiveDir if "dir" in props, not if "effectiveDir" in props.

About appearance "effectiveDir" in props - see the paragraph above. What about "dir" - I react on its change only by recomputing the "effectiveDir". In the code shippet from my previous comment this is done in refreshRendering(), but instead this may be done in computeProperties() (and I think, this will be better).

wkeese commented 9 years ago

Current implementation doesn't prohibit the inheritance, but causes the wrong situation, when widget correctly directed, but its direction-related styles are wrong.

I want to be clear that there are a number of things that aren't supported with delite. Delite's behavior is undefined (or "wrong") in these cases, and that is intentional and expected. Delite's behavior in these cases should not be considered a bug, but rather that it is operating according to spec. These limitations include:

Admittedly, the delite spec should explicitly list these limitations. But nonetheless, they are expected behavior.

dir=auto depends on the text, but causes changes in the direction of elements. ... Note: we are not against has("bidi") for this case but we don't see the difference (by sense) between support of rtl and auto.

I think the idea is to use has("bidi") or another flag for any features that slow down performance or bloat the code. We can achieve support for basic dir=rtl without either of these drawbacks.

I change value of the effectiveDir in computeProperties(). Sense of the code that you write about is to initiate the recomputing rather than react on the changes.

OK, "triggering a recomputation" is not well supported in the delite infrastructure. I guess what you did will work, but in any case, I don't see any reason to support dynamic changes to the dir of an ancestor node, even if that ancestor node is a widget.

semion1956 commented 9 years ago

It is important for us to have a good understanding of the bidi-related limitations in a new Dojo. So, few notes: 1) p#3 is unnecessary because of p#4. As far as I understand, it is enough to say, that Dojo will not support dynamic changes of the direction of widgets, caused by change of the direction of their ancestors, and this is responsibility of applications. 2) p#2 is unclear, because in a few comments before you agreed with the possibility to initiate the widget's direction with the computed style (at least in Bidi.js for has("bidi")). At least this possibility we would like to have, because lack of it significantly reduces the quality of bidi support. (BTW, if correct initial setting will be done, p#1 will become unnecessary too). I want to reiterate the importance of this item, and because we don't want to continue this discussion to infinity, simply ask : does it make sense to send you (in another PR) our suggestion connected with initial setting of widget's direction - or you are in any case not going to consider them?

wkeese commented 9 years ago

does it make sense to send you (in another PR) our suggestion connected with initial setting of widget's direction

Yes, but please make the PR with:

  1. just the isolated changes to support inheriting direction when attachedCallback() is called.
  2. only enabled when has("inherited-dir") is true