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

fix(inputs): display title attribute on root element #12776

Merged
merged 3 commits into from
Mar 6, 2021
Merged

Conversation

scscgit
Copy link
Contributor

@scscgit scscgit commented Dec 12, 2020

Display hover effect of title attr even on label of v-text-field, which may have moved outside (above) the input when not outlined, by enabling pointer-events only when it's active (not otherwise).

Fix another v-switch bug of ripple effect's event blocking hover.

Fix displaying title at all in v-radio. (Also fixed ripple here.)

Move title attr to a parent object, making it trigger even on the threshold of input boundaries, increasing its accessibility scope to avoid blind zones. Remove the duplicate title in v-text-field, v-radio, v-switch, and v-checkbox after moving to parent element; specifically VInput, or in the v-radio's case via its own parent. Modified VInput's genInputSlot(): it's being called by components VAutocomplete, VRadioGroup, VSelect, and VTextField (maybe more).

This also fixed v-select's issue of title not appearing.

Description

Fixes #12775, #11253

Motivation and Context

Title issues, details are in the issue #12775.

Implementation details

There's a related issue for v-select: #11253 where the pointer-events: none implementation was described as a workaround to achieve expected behavior. In my implementation I enabled pointer-events: auto on active label, but there may be a better solution; I was unsuccessful in trying to use something like & :not(&--active) or & :not(.v-label--active) with pointer-events: none instead, intending to avoid setting auto as an override in case the developer explicitly changes it in a parent component, so you may also reconsider this.

The title attribute was moved to the parent, and then removed from child elements by copying the { ...this.attrs$ } and delete-ing title field. This shouldn't be necessary, as the duplicate doesn't cause any harm, but will produce cleaner output.

How Has This Been Tested?

visually, but without exhaustive testing, so I'd appreciate any help.

Markup:

template>
  <v-container>
    <v-text-field title="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details" label="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details"></v-text-field>
    <v-text-field outlined title="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details" label="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details"></v-text-field>
    <v-textarea title="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details" label="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details"></v-textarea>
    <v-switch title="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details" label="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details"></v-switch>
    <v-checkbox title="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details" label="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details"></v-checkbox>
    <v-radio title="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details" label="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details"></v-radio>
    <v-select title="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details" label="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details"></v-select>
    <v-combobox title="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details" label="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details"></v-combobox>
    <v-autocomplete title="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details" label="A very long label that doesn't fit on the screen, so the user may have to hover on the label to read more details"></v-autocomplete>
  </v-container>
</template>

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any features but makes things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes and documentation updates, dev for new features and backwards compatible changes and next for non-backwards compatible changes).
  • This may introduce potential issues and isn't a fix for a critical functionality, so I'd conservatively classify it as a new feature (of label tooltips) and submit to dev first
  • My code follows the code style of this project.
  • I've added relevant changes to the documentation (applies to new features and breaking changes in core library)

Copy link
Member

@johnleider johnleider left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. I think this is a good start but there is a lot of duplication between the components that I'm sure can be DRYed up.

@scscgit
Copy link
Contributor Author

scscgit commented Dec 19, 2020

@johnleider As I'm not too knowledged about the Vuetify's component/mixin hierarchy, do you have any recommendation about the placements of those title attr exclusions?

  • Also I think someone should make the decision whether moving the label to parent is the best solution (or alternatively using it in all children, which could even be parametrized, making label-only or input-only attributes possible; btw. I think in one case I've already had to place the title on both child components due to a lack of parent).
  • On the other hand, the :title.prop="'text'" workaround that I've been recommended in the issue seems to make most of these changes redundant; but only as long as having title on parent is the expected behavior, which removes the option of only using it on a child. It'd be probably better to just take the input-specific CSS (pointer-events) fixes and then either add a new option of putting title on label, or just recommend the .prop within docs for new users.

@MajesticPotatoe MajesticPotatoe added this to the v2.4.x milestone Dec 31, 2020
packages/vuetify/src/components/VRadioGroup/VRadio.ts Outdated Show resolved Hide resolved
packages/vuetify/src/components/VRadioGroup/VRadio.ts Outdated Show resolved Hide resolved
packages/vuetify/src/components/VInput/VInput.ts Outdated Show resolved Hide resolved
packages/vuetify/src/components/VRadioGroup/VRadio.ts Outdated Show resolved Hide resolved
packages/vuetify/src/components/VSwitch/VSwitch.ts Outdated Show resolved Hide resolved
packages/vuetify/src/components/VTextField/VTextField.ts Outdated Show resolved Hide resolved
@@ -76,6 +76,7 @@
left: -12px
top: calc(50% - 24px)
margin: 7px
pointer-events: none // Enable title attr hover effect, as ripple blocks the input
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested this in all of our supported browsers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually now that you mentioned it, I've noticed that it causes a new bug of ripple not disappearing (probably on mouseout); but because we've already moved the title to the parent v-input, the ripple already inherits the title hover effect, so this workaround isn't necessary anymore, and I've removed it in a new commit.

scscgit and others added 3 commits January 6, 2021 12:02
)

Display hover effect of title attr even on label of v-text-field,
which may have moved outside (above) the input when not outlined,
by enabling pointer-events only when it's active (not otherwise).

Fix another v-switch bug of ripple effect's event blocking hover.

Fix displaying title at all in v-radio. (Also fixed ripple here.)

Move title attr to a parent object, making it trigger even on the
threshold of input boundaries, increasing its accessibility scope
to avoid blind zones. Remove the duplicate title in v-text-field,
v-radio, v-switch, and v-checkbox after moving to parent element;
specifically VInput, or in the v-radio's case via its own parent.
Modified VInput's genInputSlot(): it's being called by components
VAutocomplete, VRadioGroup, VSelect, and VTextField (maybe more).

This also fixed v-select's issue of title not appearing (vuetifyjs#11253).
Co-authored-by: John Leider <9064066+johnleider@users.noreply.github.com>
)

The `pointer-events: none` workaround had caused a new issue of
ripple not disappearing on mouseout event and has been removed.

As we've moved the title attribute to a parent v-input, it's no
longer necessary to disable the ripple's hover effect override,
because ripple now properly inherits the title. We'd still need
some new workaround if we had to explicitly put title on input.
Copy link
Member

@johnleider johnleider left a comment

Choose a reason for hiding this comment

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

Is there a specific reason for not having a separate computed property that reduces the repeating of process.

@scscgit
Copy link
Contributor Author

scscgit commented Feb 10, 2021

@johnleider if you mean the process of removing title from this.attrs$, its used only once per component. Would you prefer moving each of them to a separate computed property, or is there some viable place for a mixin inheritance where I should move them?

@KaelWD KaelWD changed the title fix: display v-text-field title on VLabel; v-radio; etc (#12775) fix(inputs): display title attribute on root element Mar 6, 2021
@KaelWD KaelWD merged commit 17bd263 into vuetifyjs:dev Mar 6, 2021
@KaelWD KaelWD modified the milestones: v2.4.x, v2.5.0 Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Selection controls C: VInput VInput C: VSelect VSelect C: VTextField VTextField T: bug Functionality that does not work as intended/expected
Projects
None yet
4 participants