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

Add HasLabel #3241

Closed
henrikerola opened this issue Jan 3, 2018 · 14 comments · Fixed by #8424
Closed

Add HasLabel #3241

henrikerola opened this issue Jan 3, 2018 · 14 comments · Fixed by #8424

Comments

@henrikerola
Copy link

I have the following Vaadin 7 code, which I ported to Flow.

private Field<?> createField(FilterBy filterBy) {
  if (filterBy.getType() == Date.class) {
    return new DateField();
  }
  return new TextField();
}

private Field<?> doCreateField(FilterBy filterBy) {
  Field<?> field = createField(filterBy);
  field.setCaption("Search For");
  return field;
}

I ended up with the following solution with Flow because there is no a superclass or an interface which would contain the setLabel method:

private Component createField(FilterBy filterBy) {
  if (filterBy.getType() == Date.class) {
      return new DatePicker("Search For");
  }
  return new TextField("Search For");
}

I think that there could be a HasLabel interface implemented by all Components which have a label. That would simplify the above solution.

@Artur-
Copy link
Member

Artur- commented Jan 8, 2018

Something like

/**
 * A component that supports a label.
 * <p>
 * A label is typically shown above or to the left of the component itself. In
 * Vaadin Framework 8, the term <code>caption</code> was used for a label.
 *
 * @author Vaadin Ltd
 */
public interface HasLabel extends HasElement {

    /**
     * Set the label of the component to the given text.
     * <p>
     * Any HTML is automatically escaped to prevent injection attacks.
     *
     * @param label
     *            the label text to set
     */
    default public void setLabel(String label) {
        getElement().setProperty("label", label == null ? "" : label);
    }

    /**
     * Gets the label of the component.
     *
     * @return the label of the component or <code>""</code> if no label has
     *         been set
     */
    default String getLabel() {
        return getElement().getProperty("label", "");
    }
}

would be useful.. Requires updates to the component generator etc

@jouni
Copy link
Member

jouni commented Sep 18, 2018

Would this apply to buttons and icons as well? Is this about accessibility labels or just visual field labels?

@pleku pleku modified the milestones: V13 Candidates, V14 Candidates Jan 29, 2019
@pleku pleku modified the milestones: V14 Candidates, Candidates Mar 5, 2019
@pleku
Copy link
Contributor

pleku commented Mar 5, 2019

Would this apply to buttons and icons as well? Is this about accessibility labels or just visual field labels?

I believe this was intended to the components with the label property that shows the visual field label.

Do you @jouni see this should also automatically map to the aria-label attribute ? I see that vaadin-text-field has the aria-labelledby attribute in the input element, but when the label attribute is not automatically mapped to aria-label there.

@jouni
Copy link
Member

jouni commented Mar 6, 2019

Do you @jouni see this should also automatically map to the aria-label attribute?

Possibly. Requires testing (all our components with labels) that it does not interfere with announcing other pieces of content inside an element. With a quick test (macOS Safari & VoiceOver) it doesn’t seem to do harm.

@pleku
Copy link
Contributor

pleku commented Mar 7, 2019

OK. @tomivirkki I don't know where to create an issue for testing and then possibly implementing this to some of our web components with the label property.

This issue can be kept for detecting the label property in the generator and then adding the HasLabel mixin interface for those components in Java.

@pleku
Copy link
Contributor

pleku commented Mar 7, 2019

Lets keep this issue just for the server side mixin interface HasLabel

Acceptance Criteria

Add interface HasLabel extends HasElement with methods

  • default void setLabel(String label) { ... }
  • and getter for label property

EDIT: just add the mixin interface and a unit test for it (so it won't be accidentally removed). And instead of adding the code for the generator, one could just make lots of PRs for all the web component integrations to add the HasLabel to the correct components.

There probably should be another similar but separate HasAriaLabel mixin interface. That can be left to another ticket, as there would be quite much testing involved for applying that one. At least it is not that straight forward that I'd take it along with this rather simple issue.

@tomivirkki
Copy link
Member

tomivirkki commented Mar 7, 2019

I don't why only the components that have a label property should have dedicated API for aria-label. For example, grid uses aria-label in some of the examples https://cdn.vaadin.com/vaadin-grid/5.3.3/demo/ even though it doesn't have a label property

Another example: https://cdn.vaadin.com/vaadin-button/2.1.3/demo/#button-accessibility-demos

@pleku
Copy link
Contributor

pleku commented Mar 7, 2019

Yes it would be a separate issue to add aria-labelJava API to e.g. Grid and Button.

@vaadin-miki
Copy link
Contributor

bringing this to attention, as I have yet another customer project in which having HasLabel would greatly help

@pleku
Copy link
Contributor

pleku commented Mar 16, 2020

As updated the acceptance criteria above #3241 (comment) the aria-label is not part of this and should be done separately as it needs a bit more thought. I think it can be added later on and doesn't need to be tied to this one.

@ghost
Copy link

ghost commented May 26, 2020

HI @pleku I can work on this issue.

@pleku pleku moved this from Ready for Community to Work In Progress in Vaadin Flow enhancements backlog (Vaadin 10+) May 27, 2020
Vaadin Flow enhancements backlog (Vaadin 10+) automation moved this from Work In Progress to Done / Pending Release Jun 18, 2020
pleku added a commit that referenced this issue Jun 18, 2020
Add new HasLabel interface to be used by components that supports label
definitions

Fixes #3241

Co-authored-by: Pekka Hyvönen <pekka@vaadin.com>
@pleku
Copy link
Contributor

pleku commented Jun 18, 2020

Thanks for the contribution @ovidiupopa91 !

Interface is added in Flow 3.2 and will be in Vaadin 17 but it is another story to take it into use in our components.

Basically all field components should get it, but it is an open question if those want to override the getter method and retain the existing behavior or not. If you're up for it @ovidiupopa91 , you can start with https://github.com/vaadin/vaadin-text-field-flow

@ghost
Copy link

ghost commented Jun 18, 2020

Thanks for the contribution @ovidiupopa91 !

Interface is added in Flow 3.2 and will be in Vaadin 17 but it is another story to take it into use in our components.

Basically all field components should get it, but it is an open question if those want to override the getter method and retain the existing behavior or not. If you're up for it @ovidiupopa91 , you can start with https://github.com/vaadin/vaadin-text-field-flow

Hi @pleku . No problem :) Yes, I can start with https://github.com/vaadin/vaadin-text-field-flow. Will you open a new issue in the repository page?

@mvysny
Copy link
Member

mvysny commented Sep 3, 2021

I see that the HasLabel interface has been created, but nothing implements it (not TextField, and most importantly not Checkbox which uses different label mechanism than the 'label' attribute), which makes the fix completely useless at the moment.

A ticket for that can be found here vaadin/flow-components#956

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Vaadin Flow enhancements backlog (Vaa...
  
Done / Pending Release
Development

Successfully merging a pull request may close this issue.

7 participants