Removed lowercasing of DOM attributes

[mstahv]

Lowercasing of DOM attribute names is not required for html, but makes the element API incompatible with SVG elements.

I'm not 100% sure why lowercasing is done originally, but it can in some cases make development of components easier for lazy developers, who might use mixed case in their code (instead of using constant or constantly lowercase attribute name). If we make this change, there is a possibility (which I would doom to be tiny) that some application/component might have a regression. Thus, the change must be announced in release notes and applied in major release (we'd still have time for 24).

Related to #2842

Description

Please list all relevant dependencies in this section and provide summary of the change, motivation and context.

Together with https://github.com/vaadin/flow/tree/element-namespace should fix #2842 (but needs to be tested still!)

Type of change

• Bugfix
• Feature

Checklist

• I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
• I have added a description following the guideline.
• The issue is created in the corresponding repository and I have referenced it.
• I have added tests to ensure my change is effective and works as intended.
• New and existing tests are passing locally with my change.
• I have performed self-review and corrected misspellings.

Additional for Feature type of change

• Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

[Legioth]

I don't see why we would risk introducing breaking changes for existing applications here when we could just as well add an if around those conversions to only apply them together with the default (null) namespace. The Element API tries to match HTML semantics and HTML semantics are explicitly ignoring the case for attribute names.

[github-actions]

Test Results

   946 files  ±0     946 suites  ±0   53m 13s ⏱️ - 3m 55s
5 910 tests  - 3  5 875 ✔️  - 3  35 💤 ±0  0 ❌ ±0 
6 162 runs   - 3  6 120 ✔️  - 3  42 💤 ±0  0 ❌ ±0 

Results for commit e046857. ± Comparison against base commit d351ed1.

♻️ This comment has been updated with latest results.

[mstahv]

I see a place to remove code (simplicity, performance (not profiled if relevant) ) and to add functionality at the same time. If we can't find a single place that would break because of this change, I'd take risk as the other way to do it will make framework more complex. If we later on see major regressions, which would make me quite surprised, we can take the alternative approach.

So I don't see a reason why we wouldn't try this first.

[Legioth]

One interesting but slightly overkill approach might be to add some additional complexity in the next 23.3.x maintenance release to look for occurrences of attributes that aren't fully lower case and report those through the MAU tracker. This would help give us an upper bound for how many applications might be affected.

[mstahv]

That is a cool idea indeed. Didn't even come to my mind that we could "abuse" MAU tracker for this kind of purposes. But yeah, I'd consider it overkill for this case (as my hunch says the regressions will be so rare) and spend our precious time on some other enhancements.

The idea of doing this kind of research can/should probably be kept in mind for some bigger risk changes (cc: @mshabarov).

[Artur-]

The linked spec says though that matching attributes must work in a case insensitive way so that aA, aa and AA all are the same attribute. At least Chrome implements this so that attributes are lower cased in the DOM.

Attribute names for HTML elements may be written with any mix of lowercase and uppercase letters that are a case-insensitive match for the names of the attributes given in the HTML elements section of this document; that is, attribute names are case-insensitive.

[mstahv]

Yes @Artur-, exactly. So the setAttribute with whatever casing as key still works in the browser (after this change), but if the developer using the Element API would be trying to get that already set attribute with different casing, it would fail. I don't believe there are too many cases like that currently.

[Artur-]

I don't think we should merge this as we have no idea about the impact and we also do not have any feature implemented that would need this

[Artur-]

I am also sure that it can be implemented in a compatible way if a bit time is spent on it

[mshabarov]

@mstahv if this change makes SVG working, then I vote up for the proposed patch.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

[mshabarov]

I am also sure that it can be implemented in a compatible way if a bit time is spent on it

I have no ideas how we can decide whether to lowercase the attribute name or not. If you anticipate the breaking changes, we should spend more time on it to look for alternatives with no breaking changes.

[Artur-]

As you can see from the code, it simply changes promises on how the API works so likely things will break.

Can we e.g. set the attribute with whatever casing is given and then return it case insensitively? That would be compatible yet allow lower case attributes

[mstahv]

I'm confident dropping this "feature" of lowercasing attributes don't have too many regressions. I consider the current implementation as over-engineering and I think we should constantly look into removing obsolete parts of our code (simplicity, performance, security), even if the removal of code wouldn't provide additional benefit, like this does.

But, I wouldn't maybe land this today:

• v24 is quite close already (I think this would have been ok a month or two ago, but now we are rather late)
• this change alone don't provide new functionality (other things needed for SVG support as well)
• Leif created an additional feature to disable lowercasing in certain conditions for the branch where he has been prototyping SVG support for Element API. That adds even more complexity, that we probably don't need either, but we have a way to ship SVG support in 24 minor versions without "theoretically breaking changes", which is far more important to get in than this cleanup.

[Legioth]

If browser vendors think it's worthwhile to have the complexity in their implementations, then I'm sure it's reasonable for us to stay consistent.

The risk of regressions comes in any situation where application logic has been inconsistent with the casing of attribute names, e.g. setAttribute("ID", something) followed by getAttribute("id") in some other part of the application. We can assume and hope that this is uncommon but we cannot know.

Using upper case for attributes used to be quite common back in the days when HTML was typically written like <FONT COLOR="RED">Hello</FONT>.

[mstahv]

@Legioth Let me know when you find the first add-on/app code that calls calls getAttribute, and does that with a different casing than it has previously called setAttribute. At that point, I would be very surprised, but I'd probably still be ready to break that add-on (or send the fellow a PR to clean the code). When you find a second, I will lose my hope for developers and still call this "feature" over-engineering.

[Legioth]

Absence of proof is not proof of absence

[Artur-]

This is not backwards compatible so we cannot do it for 24.x, so let's close it