Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: deprecate Label and add NativeLabel as replacement #16708

Merged
merged 12 commits into from
May 16, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@
* A TestBench element representing a <code>&lt;label&gt;</code> element.
*
* @since 1.0
* @deprecated Use {@link NativeLabelElement} instead.
*/
@Element("label")
@Deprecated(since = "24.1", forRemoval = true)
public class LabelElement extends TestBenchElement {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright 2000-2023 Vaadin Ltd.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package com.vaadin.flow.component.html.testbench;

import com.vaadin.testbench.TestBenchElement;
import com.vaadin.testbench.elementsbase.Element;

/**
* A TestBench element representing a <code>&lt;label&gt;</code> element.
*
* @since 24.1
*/
@Element("label")
public class NativeLabelElement extends TestBenchElement {

}
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,20 @@

import java.util.Optional;

import com.vaadin.flow.component.AttachEvent;
import com.vaadin.flow.component.Component;
import com.vaadin.flow.component.Html;
import com.vaadin.flow.component.HtmlContainer;
import com.vaadin.flow.component.PropertyDescriptor;
import com.vaadin.flow.component.PropertyDescriptors;
import com.vaadin.flow.component.Tag;
import com.vaadin.flow.shared.Registration;
import org.slf4j.LoggerFactory;

/**
* Component for a <code>&lt;label&gt;</code> element, which represents a
* caption for an item in a user interface.
* <p>
* Note that Label components are not meant for loose text in the page - they
* should be coupled with another component by using the
* {@link #setFor(Component)} or by adding them to it with the
* {@link #add(Component...)} method.
* <p>
* Clicking on a label automatically transfers the focus to the associated
* component. This is especially helpful when building forms with
* {@link Input}s.
Expand All @@ -46,12 +44,29 @@
* @see <a href=
* "https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label">https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label</a>
* @since 1.0
* @deprecated Use {@link NativeLabel} instead, if you need the HTML
* <code>&lt;label&gt;</code> element, which is normally not needed
* within a Vaadin Flow application's high-level components. This
* component was deprecated because it was confusing users of
* earlier Vaadin Versions and users migrating from Swing to the
* modern Vaadin Flow Framework. The {@link Label} component /
* <code>&lt;label&gt;</code> element is not meant for loose text in
* the page - it should only be coupled with another component by
* using the {@link #setFor(Component)} or by adding them to it with
* the {@link #add(Component...)} method, for example if you use
* {@link Input}.
*
*/
@Tag(Tag.LABEL)
@Deprecated(since = "24.1", forRemoval = true)
tltv marked this conversation as resolved.
Show resolved Hide resolved
public class Label extends HtmlContainer {
private static final PropertyDescriptor<String, Optional<String>> forDescriptor = PropertyDescriptors
.optionalAttributeWithDefault("for", "");

private static Boolean productionMode = null;

private Registration checkForAttributeOnAttach;

/**
* Creates a new empty label.
*/
Expand Down Expand Up @@ -118,4 +133,55 @@ public void setFor(String forId) {
public Optional<String> getFor() {
return get(forDescriptor);
}

@Override
protected void onAttach(AttachEvent attachEvent) {
super.onAttach(attachEvent);
if (skipForAttributeCheck(attachEvent)
|| !attachEvent.isInitialAttach()) {
return; // skip check in production so that customer / clients /
// ops-teams are not complaining about this warning to the
// devs. This should be dealt with by devs in development
// mode.
}
checkForAttributeOnAttach = attachEvent.getUI()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that before adding a new listener by beforeClientResponse, it should check

if (checkForAttributeOnAttach == null) { 

otherwise I don't understand why we use beforeClientResponse at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

.beforeClientResponse(this, ctx -> {
// Label was not associated with a for-attribute
// AND
// Label was not associated by adding a nested component
if (getFor().isEmpty()
&& getChildren().findAny().isEmpty()) {
LoggerFactory.getLogger(Label.class.getName()).warn(
"The Label '{}' was not associated with a component. "
+ "Labels should not be used for loose text on the page. "
+ "Consider alternatives like Text, Paragraph, Span or Div. "
+ "See the JavaDocs and Deprecation Warning for more Information.",
getText());
}
checkForAttributeOnAttach.remove();
});
}

/**
* Checks if the application is running in production mode.
* <p>
* When unsure, reports that production mode is true so spam-like logging
* does not take place in production.
*
* @return true if in production mode or the mode is unclear, false if in
* development mode
**/
private static boolean skipForAttributeCheck(AttachEvent attachEvent) {
if (productionMode != null) {
return productionMode;
}

var session = attachEvent.getSession();
if (session == null) {
return true;
}

productionMode = session.getConfiguration().isProductionMode();
return productionMode;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/*
* Copyright 2000-2023 Vaadin Ltd.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package com.vaadin.flow.component.html;

import com.vaadin.flow.component.Component;
import com.vaadin.flow.component.Html;
import com.vaadin.flow.component.HtmlContainer;
import com.vaadin.flow.component.PropertyDescriptor;
import com.vaadin.flow.component.PropertyDescriptors;
import com.vaadin.flow.component.Tag;
import java.util.Optional;

/**
* Component for a <code>&lt;label&gt;</code> element, which represents a
* caption for an input field in a user interface.
* <p>
* Note that Label components are not meant for loose text in the page - they
* should be coupled with another component by using the
* {@link #setFor(Component)} or by adding them to it with the
* {@link #add(Component...)} method.
* <p>
* Clicking on a label automatically transfers the focus to the associated
* component. This is especially helpful when building forms with
* {@link Input}s.
* <p>
* For adding texts to the page without linking them to other components,
* consider using a {@link Span} or a {@link Div} instead. If the text should be
* interpreted as HTML, use a {@link Html} (but remember to guard against
* cross-site scripting attacks).
*
* @author Vaadin Ltd
* @see <a href=
* "https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label">https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label</a>
* @since 24.1
*/
@Tag(Tag.LABEL)
public class NativeLabel extends HtmlContainer {
private static final PropertyDescriptor<String, Optional<String>> forDescriptor = PropertyDescriptors
.optionalAttributeWithDefault("for", "");

/**
* Creates a new empty label.
*/
public NativeLabel() {
super();
}

/**
* Creates a new label with the given text content.
*
* @param text
* the text content
*/
public NativeLabel(String text) {
this();
setText(text);
}

/**
* Sets the component that this label describes. The component (or its id)
* should be defined in case the described component is not an ancestor of
* the label.
* <p>
* The provided component must have an id set. This component will still use
* the old id if the id of the provided component is changed after this
* method has been called.
*
* @param forComponent
* the component that this label describes, not <code>null</code>
* , must have an id
* @throws IllegalArgumentException
* if the provided component has no id
*/
public void setFor(Component forComponent) {
if (forComponent == null) {
throw new IllegalArgumentException(
"The provided component cannot be null");
}
setFor(forComponent.getId()
.orElseThrow(() -> new IllegalArgumentException(
"The provided component must have an id")));
}

/**
* Sets the id of the component that this label describes. The id should be
* defined in case the described component is not an ancestor of the label.
*
* @param forId
* the id of the described component, or <code>null</code> if
* there is no value
*/
public void setFor(String forId) {
set(forDescriptor, forId);
}

/**
* Gets the id of the component that this label describes.
*
* @see #setFor(String)
*
* @return an optional id of the described component, or an empty optional
* if the attribute has not been set
*/
public Optional<String> getFor() {
return get(forDescriptor);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ private static boolean isSpecialSetter(Method method) {
&& method.getParameterTypes()[0] == Component.class) {
return true;
}
if (method.getDeclaringClass() == NativeLabel.class
&& method.getName().equals("setFor")
&& method.getParameterTypes()[0] == Component.class) {
return true;
}

// Anchor.setTarget(AnchorTargetValue) -
// https://github.com/vaadin/flow/issues/8346
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright 2000-2023 Vaadin Ltd.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package com.vaadin.flow.component.html;

import org.junit.Assert;
import org.junit.Test;

public class NativeLabelTest extends ComponentTest {

// Actual test methods in super class

@Override
protected void addProperties() {
addOptionalStringProperty("for");
}

@Test
public void setForComponent() {
NativeLabel otherComponent = new NativeLabel();
otherComponent.setId("otherC");
NativeLabel l = (NativeLabel) getComponent();
l.setFor(otherComponent);
Assert.assertEquals(otherComponent.getId().get(), l.getFor().get());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import com.vaadin.flow.component.Tag;
import com.vaadin.flow.component.dependency.JavaScript;
import com.vaadin.flow.component.html.Div;
import com.vaadin.flow.component.html.Label;
import com.vaadin.flow.component.html.NativeLabel;
import com.vaadin.flow.router.Route;
import com.vaadin.flow.shared.ui.LoadMode;
import com.vaadin.flow.uitest.servlet.ViewTestLayout;
Expand All @@ -29,7 +29,7 @@
@JavaScript(value = "./consoleLoggingProxy.js", loadMode = LoadMode.INLINE)
public class BrowserLoggingView extends Div {
public BrowserLoggingView() {
Label label = new Label("Just a label");
NativeLabel label = new NativeLabel("Just a label");
label.setId("elementId");
add(label);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
package com.vaadin.flow.uitest.ui.template;

import com.vaadin.flow.component.html.Input;
import com.vaadin.flow.component.html.Label;
import com.vaadin.flow.component.html.NativeLabel;
import com.vaadin.flow.component.polymertemplate.EventHandler;
import com.vaadin.flow.component.polymertemplate.Id;
import com.vaadin.flow.component.polymertemplate.PolymerTemplate;
Expand All @@ -29,7 +29,7 @@ public abstract class AbstractAttachExistingElementByIdTemplate
private Input input;

@Id("label")
private Label label;
private NativeLabel label;

protected AbstractAttachExistingElementByIdTemplate(String id) {
setId(id);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import org.junit.Assert;
import org.junit.Test;

import com.vaadin.flow.component.html.testbench.LabelElement;
import com.vaadin.flow.component.html.testbench.NativeLabelElement;
import com.vaadin.flow.testutil.ChromeBrowserTest;

public class UrlValidationIT extends ChromeBrowserTest {
Expand All @@ -36,7 +36,8 @@ public void devModeUriValidation_uriWithDirectoryChange_statusForbidden()
throws Exception {
// open a view and wait till the expected label is displayed
open();
waitUntil(input -> $(LabelElement.class).id("elementId").isDisplayed());
waitUntil(input -> $(NativeLabelElement.class).id("elementId")
.isDisplayed());
// check the forbidden url
sendRequestAndValidateResponseStatusBadRequest(
"/VAADIN/build/%252E%252E");
Expand All @@ -47,7 +48,8 @@ public void staticResourceUriValidation_uriWithDirectoryChange_statusForbidden()
throws Exception {
// open a view and wait till the expected label is displayed
open();
waitUntil(input -> $(LabelElement.class).id("elementId").isDisplayed());
waitUntil(input -> $(NativeLabelElement.class).id("elementId")
.isDisplayed());
// check the forbidden url
sendRequestAndValidateResponseStatusBadRequest(
"/VAADIN/build/%252E%252E/some-resource.css");
Expand Down
Loading