vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
617 stars 167 forks source link

Warn users about incorrect usage of Label #3532

Closed marcushellberg closed 1 year ago

marcushellberg commented 6 years ago

When I incorrectly use Label in V10, I want to get a warning message.

In Vaadin 8 and earlier Label was used for text content. In V10, it maps to <label>. This will likely lead to a lot of incorrect markup that's possibly confusing for screen readers.

Suggestion: Log a warning in the console if a Label is used without for or a child element.

Legioth commented 6 years ago

Is this really a problem in practice? How are screen readers interpreting <label> elements without for or children?

pleku commented 6 years ago

Warning message in console on client or server ?

marcushellberg commented 6 years ago

On the server. I don't think that developer using the Java API will look at the browser console so actively.

I don't know that screen readers will have problems with the incorrect usage, but I would guess they would try to use <label> info.

It may also lead to layout confusion as <label> is an inline block as opposed to block/inline-block that Label had.

Legioth commented 6 years ago

So the first step would be to investigate whether this is actually a problem at all?

marcushellberg commented 6 years ago

Well, it does lead to incorrect markup at least.

jreznot commented 6 years ago

Guys from business / enterprise development usually consider Label as ANY "textual message on screen". From my FW experience I would say that caption is associated with Label for UI field, but Flow has much more common with HTML5. That is the problem with terminology.

knoobie commented 3 years ago

@marcushellberg instead of logging a warning, I would propose to rename the current Label to NativeLabel and create a "new" Label that just extends Text.

Of course this is a breaking change, but it's worth for the end user and probably for the developer coming from other frameworks like swing.

@pleku what do you think?

Edit: My gut feeling, that this could be a major problem, is manifesting. It did not take one hour to see code that misuses label for displaying text in the community discord.

About Leif's screen reader point:

It's not a problem per se for AT, but it's a WCAG violation https://www.w3.org/TR/WCAG20-TECHS/H44.html

Edit 2: Okay, found the corresponding issue and some thoughts against renaming. https://github.com/vaadin/flow/issues/4384

knoobie commented 1 year ago

This is still an issue and people still abuse the Label making their application less accessible. I'm open to provide an PR for my suggestion from above (adding NativeLabel and deprecating of Label / removal in 25) if you wanna tackle this.

cc: @rolfsmeds @mshabarov

rolfsmeds commented 1 year ago

I would not be against the rename, but it would presumably have to wait until the next major.

A warning on the other hand could be added in a patch release even.

Legioth commented 1 year ago

It's not enough to only add a warning since there are also legitimate use cases for the functionality. One possibility would be to deprecate Label and add NativeLabel as non-deprecated.

knoobie commented 1 year ago

Yeah, that's what I wanna do if you guys are on board.

Step 1: Add a warning in onDetach() if the Labelhas no forattribute set (Using detach instead of attach to make sure that it works reliable even if people add the input field later) Step 2: Add NativeLabel and deprecate Label Step 3: Remove Label

rolfsmeds commented 1 year ago

How about also deprecating Text and introducing TextNode as a non-deprecated alias, so that we can later introduce a Text component to serve as a generic text element replacing V8 Label?

knoobie commented 1 year ago

I really like that, even tho I probably have to refactor / copy paste some of my code because I'm heavily relying on Text currently. Worth another issue I would say!

rolfsmeds commented 1 year ago

There is one and I'll link it here as soon as I find it amongst the hundreds of other issues containing the term "text".

knoobie commented 1 year ago

Created a PR to push the discussion forward. Deprecating and later removal of Label could also help the modernization team and the design system team.

The design system could add their own "Label" component that has way more and better feature than the regular label and could be used more widely and replace most of the direct need of creating spans or divs to show text (looking a Rolfs idea about a styled text component)

In addition, the modernization team could more easily migrate from the old to the new label / swing users and old Vaadin users already know this term.

For developers this would (hopefully) get them a new and better label component in the long run and should drastically reduce accessibility problems people have created over the last years when building applications with this component in a wrong way. (Because of a history with V8, a swing history or just because they didn't know better and used it because they can)

Creating html labels should not be needed in normal use cases and the migration path to the new native label is literally a simple replacement that can be done by a single IDE replacement call.

vaadin-bot commented 1 year ago

This ticket/PR has been released with Vaadin 24.1.0.