Add global_attributes into both html and svg Props. #144

davesnx · 2022-02-14T10:18:18Z

Just including all global attributes on svg, so are all available as Props.

jchavarri

Looks good! 👍

jchavarri · 2022-02-14T18:39:55Z

lib/dom_global_attributes.ml

+
+let contentEditable = bool "contentEditable"
+
+let contextMenu = string "contextMenu"


This attr is obsolete and will be removed, I wonder if we should just not add it https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contextmenu

Done in aa8a2e5

jchavarri · 2022-02-14T18:40:51Z

lib/dom_global_attributes.ml

+
+let spellCheck = bool "spellCheck"
+
+let tabIndex = int "tabIndex"


What is the rule about camelCase / snake_case in general? I assume for DOM related stuff we keep the original names? cc @glennsl

Good question. HTML attributes are actually flatcase, so if we were restricted to those we could have just ignored the whole issue. Unfortunately our life is not that simple. SVG actually manages to mix both flatcase, camelCase and kebab-case in its attribute naming. And React of course has chosen camelCase even for flatcased names. Since there are other React-specific naming conventions we rely on as well (className, htmlFor etc.), it might be better to just stick with that, yeah.

Now I wonder if we should have stuck with camelCase for CSS properties too...

glennsl · 2022-02-14T19:36:05Z

lib/dom_html.ml

@@ -11,36 +12,6 @@ module Prop = struct

  let defaultValue = string "defaultValue"

-  (* global html attributes *)


Hold on, these are global HTML attributes, meaning that they apply to all HTML elements, but not necessarily to any other elements, like SVG. SVG does specify a set of "core" attributes which overlap quite a bit with these, but neither are a subset or superset of the other.

If some of these are missing from Svg, I'd suggest just adding them directly there.

Following the HTML spec, you are totally correct. By the spec SVGs aren't valid with those attributes attached. This isn't the same in React JSX where any of those are available and valid (in their TypeScript definitions as well): https://codesandbox.io/s/mutable-fire-cd5ro?file=/src/App.js

The only attribute that isn't valid on the sandbox is 'accessKey' (wasn't aware of that one).

I'm unsure what we prefer, to keep it close to the spec (which seems reasonable for me) or keep it as React. For now, I would argue that all props are available to all elements, which makes HTML validity (or SVG validity) a joke.

Part of the effort of #71 and https://github.com/ml-in-barcelona/jsoo-react/pull/66/files was to create a more HTML safe mechanism than reason-react. This effort with the Dom_Html got a little lost.

I'm unsure what we prefer, to keep it close to the spec (which seems reasonable for me) or keep it as React.

@davesnx Maybe a good middle ground is what I believe @glennsl is suggesting: keep the actual common global set in the new module, and the html- and svg-specific on either Html or Svg.

If I'm not wrong, I think the actual common set is id, tabindex, lang, className and style.

I was thinking it'd be better to just copy the missing one's to Svg. The problem with having a separate modules with "global" attributes is that it implies that these apply to everything. And so if at some point in the future we decide to add support for something like VML, for example (for the impending metaverse), or MathML, then odds are these won't all be applicable.

Part of the effort of #71 and https://github.com/ml-in-barcelona/jsoo-react/pull/66/files was to create a more HTML safe mechanism than reason-react. This effort with the Dom_Html got a little lost.

Thanks for pointing to these. I've seen this referenced elsewhere, but hadn't actually seen the PRs. They're very well-structured and pleasant to read too! What I still don't understand, however, is the rationale for this. I see correctness mentioned in one of the PRs, but correctness shouldn't be an end in and of itself (although if it's free then I certainly wouldn't mind!) I also don't entirely understand how this PR fits into the picture.

(for the impending metaverse)

🤣

I still don't understand, however, is the rationale for this

I believe (@davesnx can correct me) the rationale was to have a way to help users identify cases where the wrong attribute is being passed to some element.

but correctness shouldn't be an end in and of itself

could you elaborate? I don't understand this, I would always assume using correctness to have the tool help users build valid trees would be useful, all other things being equal.

Not necessarily. There are many situations where the price of correctness is higher than the benefit. And what exactly is "correct" isn't always so well defined either. As is the case with HTML, which is an ever-changing "living standard". It's also not just that new elements and attributes are added, some also get deprecated. Is it then correct to use a deprecated attribute? Is it correct to use an attribute that has just been standardized but isn't yet supported by any browser?

In addition comes the cost of maintaining such a complex and ever-changing system and the procedures for deciding what to include and what not to include in it. But that's still just one side of the equation, and why I'm asking about the other side. There might be benefits that outweigh these costs, but I can't really say that I see them.

I can see that rejecting attributes that are invalid on certain elements as a use case, but I don't actually see much value in that in practice. Looking at the codebase I'm working on now, we use very few non-global attributes. There's quite a few <a href=...>s and <img src=...>s, and some <input type=...>s, but that's really about it for HTML. SVG is much more interesting, but the challenge there is to remember the cryptic attribute names in the first place, and what mysterious sequence of characters to put in them, not so much which goes where. And I'll usually create a higher level API so as to avoid having to deal with that directly anyway.

That's not to say I'm against this idea. I just don't have a clear picture of either costs or benefits. I also think some of the costs will depend on what benefits are being targeted. For example, if this would enable completion of the reduced set of valid attributes for the current element, that would have significant value to me. But I can't see how to accomplish that without incurring costs that I would consider too high (like changing the DSL to something like Html.Div.(make Props[| ... |] [])).

davesnx · 2022-02-18T19:17:40Z

Updated the "missing" props into SVG and leave the discussion of correctness/safety out of this PR.

…rcelona/jsoo-react into Add-aria-into-global-attributes * 'Allow-global-attributes-on-svg' of github.com:/ml-in-barcelona/jsoo-react: feat: Moved global_attributes into svg feat: Remove contextMenu as global attrs

…ributes Add ARIA 1.1 into global-attributes

davesnx added 2 commits February 14, 2022 11:17

feat: Add global_attributes into both html and svg

f27c484

feat: Add ARIA 1.1 into global-attributes

192d440

jchavarri approved these changes Feb 14, 2022

View reviewed changes

feat: Remove contextMenu as global attrs

aa8a2e5

glennsl reviewed Feb 14, 2022

View reviewed changes

davesnx added 7 commits February 18, 2022 20:18

feat: Moved global_attributes into svg

fa040c8

refactor: Rename global_attr to aria_attr

61413de

feat: Include aria attributes on both html and svg

9b0e7f9

feat: Add aria mli

6279634

Merge pull request #145 from ml-in-barcelona/Add-aria-into-global-att…

cf8089d

…ributes Add ARIA 1.1 into global-attributes

fix: style prop on svg

132f9c2

glennsl approved these changes Feb 18, 2022

View reviewed changes

davesnx merged commit 8805668 into main Feb 19, 2022

davesnx deleted the Allow-global-attributes-on-svg branch February 19, 2022 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add global_attributes into both html and svg Props. #144

Add global_attributes into both html and svg Props. #144

davesnx commented Feb 14, 2022

jchavarri left a comment

jchavarri Feb 14, 2022

davesnx Feb 14, 2022

jchavarri Feb 14, 2022

glennsl Feb 14, 2022 •

edited

Loading

glennsl Feb 14, 2022

davesnx Feb 14, 2022

davesnx Feb 14, 2022

jchavarri Feb 15, 2022

glennsl Feb 15, 2022 •

edited

Loading

jchavarri Feb 16, 2022

glennsl Feb 16, 2022 •

edited

Loading

davesnx commented Feb 18, 2022


		let contentEditable = bool "contentEditable"

		let contextMenu = string "contextMenu"


		let spellCheck = bool "spellCheck"

		let tabIndex = int "tabIndex"

		@@ -11,36 +12,6 @@ module Prop = struct

		let defaultValue = string "defaultValue"

		(* global html attributes *)

Add global_attributes into both html and svg Props. #144

Add global_attributes into both html and svg Props. #144

Conversation

davesnx commented Feb 14, 2022

jchavarri left a 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

glennsl Feb 14, 2022 • 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

glennsl Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsl Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

davesnx commented Feb 18, 2022

glennsl Feb 14, 2022 •

edited

Loading

glennsl Feb 15, 2022 •

edited

Loading

glennsl Feb 16, 2022 •

edited

Loading