A way for attributes to contain expressions without triggering browser errors or false loading #833

DmitrySharabin · 2022-04-18T20:25:44Z

Closes #475.

I believe this feature is a nice to have in the upcoming release. I tested it on several examples, including SVG Path Builder. It looks like it's working. :)

I liked the idea of using the mv-attr-* family of attributes for the feature, so I went that way.

Since I'm not 100% sure that I do everything correctly, let's consider this PR as the first step. :)

DmitrySharabin · 2022-04-19T11:26:41Z

Here is a couple of examples: https://codepen.io/dmitrysharabin/pen/gOoqOzj

DmitrySharabin · 2022-04-19T13:05:11Z

There is an issue I don't know how to address yet: I can't preserve an attribute case. E.g., mv-attr-veiwBox is converted to mv-attr-viewbox. And it doesn't let us use the mv-attr-* family of attributes with a bunch of SVG attributes.

I tried to add different attributes with letters in different cases to elements inside and outside a Mavo app, and it looks like browsers convert attributes to lower case automatically? Am I right?

@LeaVerou Are you aware of any way to bypass it?

DmitrySharabin · 2022-04-19T13:08:32Z

I tried to add different attributes with letters in different cases to elements inside and outside a Mavo app, and it looks like browsers convert attributes to lower case automatically? Am I right?

It looks like I am. 😭
https://developer.mozilla.org/en-US/docs/Web/API/Element/getAttribute#lower_casing

karger · 2022-04-19T13:16:11Z

awkward hack: define another attribute that converts its indicated attribute from hyphenated to camel case; e.g. mv-camelcase-view-box would become viewBox.

…

On 4/19/2022 9:08 AM, Dmitry Sharabin wrote: I tried to add different attributes with letters in different cases to elements inside and outside a Mavo app, and it looks like browsers convert attributes to lower case automatically? Am I right? It looks like I am. 😭 https://developer.mozilla.org/en-US/docs/Web/API/Element/getAttribute#lower_casing — Reply to this email directly, view it on GitHub <#833 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIWSXUMRDHEZAGOSNIW2XDVF2V5XANCNFSM5TWYYJYQ>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

karger · 2022-04-19T13:31:53Z

actually, here is another possible approach: apparently there is a specified list of tag adjustments that you could perform automatically *if the node with the attributes is an svg node* which I suspect can be checked somehow. https://html.spec.whatwg.org/multipage/parsing.html#adjust-svg-attributes search for "adjust svg attributes" in this doc if the anchor fails.

…

On 4/19/2022 9:08 AM, Dmitry Sharabin wrote: I tried to add different attributes with letters in different cases to elements inside and outside a Mavo app, and it looks like browsers convert attributes to lower case automatically? Am I right? It looks like I am. 😭 https://developer.mozilla.org/en-US/docs/Web/API/Element/getAttribute#lower_casing — Reply to this email directly, view it on GitHub <#833 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIWSXUMRDHEZAGOSNIW2XDVF2V5XANCNFSM5TWYYJYQ>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

LeaVerou · 2022-04-19T14:49:43Z

Oh I'm glad you're working on this.
Is the casing issue cosmetic or does it actually cause loss of functionality?

Due to the way Mavo resolves expressions, substituting gradually, I imagine (without having seen the code) that this solution may still produce errors when the expressions depend on other expressions, e.g.:

<input type=number value="[foo]">
<span property="foo" value="[bar]"></span>
<span property="bar" value="3"></span>

DmitrySharabin · 2022-04-19T15:27:57Z

Is the casing issue cosmetic or does it actually cause loss of functionality?

In the SVG Path Builder we won't be able to get rid of the error caused by this code: viewBox="0 0 [width] [height]".

Due to the way Mavo resolves expressions, substituting gradually, I imagine (without having seen the code) that this solution may still produce errors when the expressions depend on other expressions...

Do you mean trigger browser errors? The following code works without errors (I updated the pen). :) But I'm almost 100% sure I could miss some use cases.

<input type=number mv-attr-value="[foo]" />
<span property="foo" mv-value="bar"></span>
<span property="bar">3</span>

DmitrySharabin · 2022-04-19T15:36:03Z

actually, here is another possible approach: apparently there is a specified list of tag adjustments that you could perform automatically if the node with the attributes is an svg node which I suspect can be checked somehow.

That's interesting. Thanks, David. 🙏🏻

I was thinking of the approach you suggested and wondering: do we really need this at the core? If the casing issue becomes a problem for authors, we can add a plugin that will make the mv-attr-* family of attributes case-sensitive. What do you think?

LeaVerou · 2022-04-19T15:43:49Z

Is the casing issue cosmetic or does it actually cause loss of functionality?

In the SVG Path Builder we won't be able to get rid of the error caused by this code: viewBox="0 0 [width] [height]".

Why not? Can't we specify mv-attr-viewBox and then your code would specify viewbox (lowercase), which I think would still work?

DmitrySharabin · 2022-04-19T17:15:44Z

Why not? Can't we specify mv-attr-viewBox and then your code would specify viewbox (lowercase), which I think would still work?

Ah, yes. You are absolutely right—lowercase attributes are not even an issue. They work perfectly fine. Stupid me. 🤦‍♂️

Now I need to figure out why viewBox, for example, is not live if it's set via mv-attr-viewbox. That's why I thought attributes case is an issue.

Thank you. I'll continue my investigation then.

…ithout triggering browser errors or false loading

…at are part of the value, not only the value itself

…ercase

DmitrySharabin · 2022-04-20T10:09:39Z

Thanks to David, we know that browsers adjust SVG attributes that are not all lowercase. That means that when there is an expression inside the value of an attribute (e.g., inside viewBox), Mavo gets the case-sensitive version of the attribute name to work with (e.g., viewBox). As a result, viewBox="0 0 [width] [height]" works perfectly fine (the view box adjusts according to the values of width and height).

The mv-attr-* attribute, on the other hand, is not the standard SVG attribute. So, for the mv-attr-viewBox Mavo will get the mv-attr-viewbox (all lowercase) to work with. And even though SVG understands viewbox="0 0 [width] [height]" (when added directly to <svg>), if this attribute is added through mv-attr-viewbox="0 0 [width] [height]", the expressions don't have any effect on the SVG. Though, the viewbox attribute is being updated via expressions. The same thing happens with preserveAspectRatio.

We can conclude that the casing issue causes loss of functionality. So I decided to follow David's suggestion and adjust those kinds of attributes manually. And it worked. Yay!

I updated the demo so that it showcases different use cases.
UPDATE: Refactored SVG Path builder—no console errors! 🥳

I believe this PR is ready for review. 🤞🏻

LeaVerou · 2022-04-20T15:01:05Z

Are you using the correct namespace for these attributes? I wonder if that's why viewbox doesn't work.

DmitrySharabin · 2022-04-20T19:12:24Z

Are you using the correct namespace for these attributes? I wonder if that's why viewbox doesn't work.

Hmm. That's a good question. I don't know. I definitely need to figure it out.

DmitrySharabin · 2022-04-21T18:49:29Z

Here are some results of my research concerning SVG, its elements, attributes, and namespaces:

Regardless of whether we use viewBox or viewbox, the browser adjusts it to viewBox (and adds the corresponding attribute if needed).

Suppose we have (mind the attribute case):

<svg viewbox="0 0 100 100">
  ...
</svg>

In DevTools, we see (mind the attribute case):

After inspecting the <svg> element and invoking console.dir($0) in the console, we get:

Attributes of SVG elements don't have namespaceURI (even though the corresponding elements do). Except href, IIRC. See the previous screenshot.
Regardless the point 2 (it didn't stop me 😂), I searched through the codebase and found the method where we set attributes: Primitive.setValue(). There, I replaced this line

element.setAttribute(o.attribute, value);

with this one:

if (element.namespaceURI === "http://www.w3.org/2000/svg") {
  element.setAttributeNS(element.namespaceURI, o.attribute, value);
}
else {
  element.setAttribute(o.attribute, value);
}

Then I inspected <svg> added by the following code (mind the fallback for viewBox and its case):

<svg
  viewbox="-50 -50 50 50"
  mv-attr-viewBox="[list(0, 0, 100, 100)]">
  ...
</svg>

In DevTools we have

and

As we can see, the browser adjusted the fallback attribute (as expected), and the one added by Mavo has the namespaceURI property and is evaluated correctly (its value is the result of an expression evaluation), though it doesn't have any effect. It's simply ignored by the browser. Moreover, every other mv-attr-* attribute stopped working (even though without namespace they worked fine).

To conclude, namespaceURI doesn't have any positive effect in our case.

So, unfortunately I couldn't find the way to make mv-attr-* work with case-sensitive SVG attributes without manually tweaking them (as I do it in this PR). 😔 However, I can't deny that there might be something I still don't get. 🤷🏻‍♂️

LeaVerou · 2022-04-21T19:35:13Z

I believe namespaceURI is null when it can be determined via the parent, ancestor etc namespace. It's not null on href because href has the XLink namespace implicitly (in fact it used to be written xlink:href).

I wonder if something like this would work: https://codepen.io/leaverou/pen/ZEvPPaP?editors=1111
The problem is, you don't want to change casing if e.g. someone sets a viewbox attribute on an HTML element, so you can't just match on attribute names. Context matters.

DmitrySharabin · 2022-04-21T20:09:14Z

I wonder if something like this would work: https://codepen.io/leaverou/pen/ZEvPPaP?editors=1111

Whoa! That's cool!!! Thanks for the new knowledge.

Re:context. Got it. Totally agree. I will take that into account.

@LeaVerou

Huge thank you to @LeaVerou for the idea!

DmitrySharabin · 2022-04-22T07:55:35Z

Done!

DmitrySharabin · 2022-04-23T17:35:14Z

One complication is that not all attributes will work on the root element, some of them may need to be on specific elements. You should try and see if it works for e.g. refX and that sort of thing.

If I get your comment correctly, now it should work (after some fixes I made to the code): https://codepen.io/dmitrysharabin/pen/mdpgBvy

DmitrySharabin · 2023-09-23T19:57:09Z

@LeaVerou, I would also be grateful if you give this PR another look. I believe the feature it adds to Mavo might be very beneficial. We might make this feature experimental.

adamjanicki2 · 2023-09-25T14:35:07Z

I think as long as it's deterministic, there are many right choices. The main thing we should avoid is it being non-deterministic, i.e. sometimes it overrides it and sometimes it doesn't depending on race conditions.

Definitely on the same page here, a non-deterministic scenario would be very much not ideal 😂

I agree mv-attr-foo always having precedence over foo makes sense. I could also be persuaded that the order of attributes can specify precedence.

I think overall I like having mv-attr-attrname taking precedence or attrname, but also there is some precedence with ordering in other frameworks, where typically the last attr specified takes precedence (for example in react if you have something like <Element {...props} className="..." />), so I could also be persuaded for an ordering, but since Mavo has this specific feature for defining attributes, the goal should be encouraging users to use them over the default ones, which is why I like Mavo attrs taking precedence

Regardless of what we decide, we definitely will need to update the documentation to be very clear and explicit about how this works

src/domexpression.js

…dence

src/domexpression.js

…the corresponding objects)

DmitrySharabin · 2023-10-02T20:04:47Z

src/expressions.js

+			let attributes = $$(node.attributes);
+
+			// Iterate only over mv-attr-foo with expressions
+			for (let attribute of attributes.filter(attribute => attribute.name.startsWith("mv-attr-") && syntax.test(attribute.value))) {
+				let name = attribute.name.replace("mv-attr-", "");
+				let ignorable = attributes.find(attribute => attribute.name === name);
+
+				if (ignorable) {
+					// Ignore expressions in foo, if any, since mv-attr-foo has priority
+					ignore ??= new Set();
+					ignore.add(ignorable.name);
+				}
+			}
+
+			attributes.forEach(attribute => {


@LeaVerou, thank you so much for your hint—expressions.js is indeed the appropriate place for this logic. Sorry for requesting your review one more time. 🙈

I think this may be complicating things a bit. I just pushed a commit that tries to simplify the logic, but I haven't tested it, so you'd need to do that.

Also, we don't need to apply syntax.test() here, the values of mv-attr-*, like directives, are always expressions. If you haven't designed it like that, you should.

I re-designed the mv-attr-* feature to work as mv-value. Because of the wrong design decision, I wrote way too complicated logic. I knew that mv-attr-foo doesn't make sense if it's not an expression, but I chose the wrong path to implement it. Thank you for pointing this out. 🙏🏻

…them like that

src/domexpression.js

LeaVerou · 2023-10-03T15:28:01Z

src/domexpression.js

+				this.attribute = Mavo.getProperAttributeCase(this.element, this.attribute);
+			}
+
+			this.fallback = this.fallback || Mavo.Primitive.getValue(this.element, {attribute: this.attribute});


Suggested change

this.fallback = this.fallback || Mavo.Primitive.getValue(this.element, {attribute: this.attribute});

this.fallback ||= Mavo.Primitive.getValue(this.element, {attribute: this.attribute});

(also do we need || or ?? here?)

I assume that some falsy values (e.g., 0) might be perfectly valid fallbacks, and we don't want to replace them with any other value. So, I'm leaning towards ??=.

LeaVerou

Looks good, left some minor comments

LeaVerou · 2023-10-03T15:29:36Z

src/expressions.js

@@ -183,11 +183,11 @@ var _ = Mavo.Expressions = $.Class({
 	},

 	extract: function(node, attribute, path, syntax = Mavo.Expression.Syntax.default) {
-		if (attribute && _.skip.indexOf(attribute.name) > -1) {
+		if (_.skip.includes(attribute?.name)) {


Can we remove this entirely now that _.skip is taken into account in traverse()? As far as our own codebase is concerned, that seems ok, but not sure if any plugins call extract() and depend on this behavior

Good question. I searched for extract( in our repo with plugins and found no mentions. Neither could I find this in my repos. I'm unsure if we have any other plugins we might break. We can try to commit to this change and see what might happen. 🤓

I'm ready to remove it as soon as we decide to do that. I commented this block out, and everything still works.

Let's do it!

Done!

Now, I'm ready to request your review one more time. Thanks a ton for your patience and excellent ideas! 🙏🏻

src/expressions.js

… we already do it in traverse()

LeaVerou · 2023-10-04T13:48:56Z

src/domexpression.js

+			this.attribute = this.attribute.replace("mv-attr-", "");
+
+			if ([SVG_NAMESPACE_URI, MATHML_NAMESPACE_URI].includes(this.element.namespaceURI)) {
+				this.attribute = Mavo.getProperAttributeCase(this.element, this.attribute);


Just dawned on me: do we also need to do this in XHTML documents (with an actual XML MIME type)? If so, it would require a different check (not a blocker, we can fix later).

Probably we should. Our users will push us to do this in any case. 😅

LeaVerou · 2023-10-04T13:56:45Z

src/domexpression.js

+
+			this.fallback ??= Mavo.Primitive.getValue(this.element, {attribute: this.attribute});
+			let expression = this.element.getAttribute(this.originalAttribute);
+			this.element.removeAttribute(this.originalAttribute);


There seems to be a lot of repetition between this and the preceding block implementing mv-value. I'll push a commit to address it.

Done, please check?

Whoa! Cool! It's working with this beautiful piece of code. 🥳 Thank you!!

LeaVerou

Looks like this is done then?

DmitrySharabin · 2023-10-04T15:32:27Z

I think so! 🤞

DmitrySharabin · 2023-10-04T19:03:11Z

@LeaVerou, thanks a ton for your help! We made this awesome feature happen. Yay! 🥳

karger · 2023-10-06T02:01:21Z

I think as long as it's deterministic, there are many right choices. The main thing we should avoid is it being non-deterministic, i.e. sometimes it overrides it and sometimes it doesn't depending on race conditions.

I agree mv-attr-foo always having precedence over foo makes sense. I could also be persuaded that the order of attributes can specify precedence.

Thoughts, @karger @adamjanicki2 ?

I think the natural use case for foo= mv-attr-foo= is when you want to give the browser something to work with until the expression in mv-attr-foo is evaluated, but that once that happens mv-attr-foo should dominate.

DmitrySharabin · 2023-10-06T06:51:13Z

I think the natural use case for foo= mv-attr-foo= is when you want to give the browser something to work with until the expression in mv-attr-foo is evaluated, but that once that happens mv-attr-foo should dominate.

This is exactly how it’s designed and implemented. I’m glad we are all on the same page. ☺️

karger · 2023-11-05T18:22:12Z

I think the natural use case for foo= mv-attr-foo= is when you want to give the browser something to work with until the expression in mv-attr-foo is evaluated, but that once that happens mv-attr-foo should dominate.

This is exactly how it’s designed and implemented. I’m glad we are all on the same page. ☺️

Having considered further, I can see an argument for order of attributes, because that's how html works, right? So once the expression is evaluated. we could apply the standard order of attributes rules to decide if it overrides the foo attribute.

DmitrySharabin · 2023-11-05T19:52:52Z

@karger, do you mind filing a separate issue so we can discuss it there, not in the merged PR? There is always room for improvements we don't want to skip. :)

DmitrySharabin requested a review from LeaVerou April 18, 2022 20:25

DmitrySharabin marked this pull request as draft April 19, 2022 11:27

[domexpression] Provide a way for attributes to contain expressions w…

e347995

…ithout triggering browser errors or false loading

DmitrySharabin force-pushed the fix-for-475 branch 2 times, most recently from 85c212b to d378d55 Compare April 19, 2022 21:33

[domexpression] Make mv-attr-* more versatile: support expressions th…

e7e3786

…at are part of the value, not only the value itself

DmitrySharabin force-pushed the fix-for-475 branch from d378d55 to e7e3786 Compare April 19, 2022 21:34

[domexpression] Fixes the case of SVG attributes that are not all low…

41789d1

…ercase

DmitrySharabin marked this pull request as ready for review April 20, 2022 10:10

Get proper casing of attributes in a future-proof way

a619b2b

Huge thank you to @LeaVerou for the idea!

[util] Make sure attributes have the correct namespace

e8fb167

adamjanicki2 reviewed Sep 25, 2023

View reviewed changes

src/domexpression.js Outdated Show resolved Hide resolved

If both foo and mv-attr-foo have an expression, mv-attr-foo has prece…

418a7ab

…dence

DmitrySharabin commented Sep 28, 2023

View reviewed changes

src/domexpression.js Outdated Show resolved Hide resolved

Ignore expressions in foo when we have mv-attr-foo (and don't create …

c6aa213

…the corresponding objects)

DmitrySharabin commented Oct 2, 2023

View reviewed changes

LeaVerou and others added 3 commits October 2, 2023 18:01

Attempt to simplify the logic

15a7e62

Don't call extract() for globally skipped attributes

4eb61a2

The values of mv-attr-* are always expressions, and we should handle …

d5aac47

…them like that

DmitrySharabin requested a review from LeaVerou October 3, 2023 08:25

LeaVerou reviewed Oct 3, 2023

View reviewed changes

src/domexpression.js Outdated Show resolved Hide resolved

LeaVerou reviewed Oct 3, 2023

View reviewed changes

LeaVerou requested changes Oct 3, 2023

View reviewed changes

DmitrySharabin added 4 commits October 3, 2023 21:49

Add constants for namespaces URIs

2f2d0bc

Simplify the code a bit and make it more predictable

eb0aa6a

Make _.directives a list of strings and regexes

cbaa985

Don't check whether an attribute should be skipped in extract() since…

75ea4b9

… we already do it in traverse()

DmitrySharabin requested a review from LeaVerou October 4, 2023 10:58

LeaVerou reviewed Oct 4, 2023

View reviewed changes

Reduce repetition between mv-value and mv-attr-*

5dddf0a

LeaVerou approved these changes Oct 4, 2023

View reviewed changes

DmitrySharabin merged commit 555fae2 into master Oct 4, 2023

DmitrySharabin deleted the fix-for-475 branch October 4, 2023 19:01

	this.fallback = this.fallback \|\| Mavo.Primitive.getValue(this.element, {attribute: this.attribute});
	this.fallback \|\|= Mavo.Primitive.getValue(this.element, {attribute: this.attribute});

A way for attributes to contain expressions without triggering browser errors or false loading #833

A way for attributes to contain expressions without triggering browser errors or false loading #833

Conversation

DmitrySharabin commented Apr 18, 2022 • edited Loading

DmitrySharabin commented Apr 19, 2022 • edited Loading

DmitrySharabin commented Apr 19, 2022 • edited Loading

DmitrySharabin commented Apr 19, 2022

karger commented Apr 19, 2022 via email

karger commented Apr 19, 2022 via email

LeaVerou commented Apr 19, 2022

DmitrySharabin commented Apr 19, 2022

DmitrySharabin commented Apr 19, 2022

LeaVerou commented Apr 19, 2022

DmitrySharabin commented Apr 19, 2022

DmitrySharabin commented Apr 20, 2022 • edited Loading

LeaVerou commented Apr 20, 2022

DmitrySharabin commented Apr 20, 2022

DmitrySharabin commented Apr 21, 2022 • edited Loading

LeaVerou commented Apr 21, 2022 • edited Loading

DmitrySharabin commented Apr 21, 2022

DmitrySharabin commented Apr 22, 2022

DmitrySharabin commented Apr 23, 2022

DmitrySharabin commented Sep 23, 2023

adamjanicki2 commented Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

LeaVerou Oct 2, 2023 • edited Loading

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

LeaVerou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitrySharabin Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeaVerou Oct 4, 2023 • edited Loading

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

LeaVerou left a comment

Choose a reason for hiding this comment

DmitrySharabin commented Oct 4, 2023

DmitrySharabin commented Oct 4, 2023

karger commented Oct 6, 2023

DmitrySharabin commented Oct 6, 2023

karger commented Nov 5, 2023

DmitrySharabin commented Nov 5, 2023

DmitrySharabin commented Apr 18, 2022 •

edited

Loading

DmitrySharabin commented Apr 19, 2022 •

edited

Loading

DmitrySharabin commented Apr 19, 2022 •

edited

Loading

DmitrySharabin commented Apr 20, 2022 •

edited

Loading

DmitrySharabin commented Apr 21, 2022 •

edited

Loading

LeaVerou commented Apr 21, 2022 •

edited

Loading

adamjanicki2 commented Sep 25, 2023 •

edited

Loading

LeaVerou Oct 2, 2023 •

edited

Loading

DmitrySharabin Oct 3, 2023 •

edited

Loading

LeaVerou Oct 4, 2023 •

edited

Loading