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(b-form-tags): add ignoreInputFocusSelector prop to make input focus behavior configurable (closes #5425) #5429

Conversation

paweltatarczuk
Copy link
Contributor

@paweltatarczuk paweltatarczuk commented May 22, 2020

Describe the PR

This PR adda a new ignoreInputFocusSelector prop which defaults to .b-form-tag.

With this new prop users are able to define a selector for all elements that should not focus the <b-form-tags> input on click.

Closes #5425.

PR checklist

What kind of change does this PR introduce? (check at least one)

  • Bugfix (fixes a boo-boo in the code) - fix(...), requires a patch version update
  • Feature (adds a new feature to BootstrapVue) - feat(...), requires a minor version update
  • Enhancement (augments an existing feature) - feat(...), requires a minor version update
  • ARIA accessibility (fixes or improves ARIA accessibility) - fix(...), requires a patch or minor version update
  • Documentation update (improves documentation or typo fixes) - chore(docs), requires a patch version update
  • Other (please describe)

Does this PR introduce a breaking change? (check one)

  • No
  • Yes (please describe since breaking changes require a minor version update)

The PR fulfills these requirements:

  • It's submitted to the dev branch, not the master branch
  • When resolving a specific issue, it's referenced in the PR's title (i.e. [...] (fixes #xxx[,#xxx]), where "xxx" is the issue number)
  • It should address only one issue or feature. If adding multiple features or fixing a bug and adding a new feature, break them into separate PRs if at all possible.
  • The title should follow the Conventional Commits naming convention (i.e. fix(alert): not alerting during SSR render, docs(badge): update pill examples, chore(docs): fix typo in README, etc). This is very important, as the CHANGELOG is generated from these messages, and determines the next version type (patch or minor).

If new features/enhancement/fixes are added or changed:

  • Includes documentation updates
  • Includes component package.json meta section updates (prop, slot and event changes/updates)
  • Includes any needed TypeScript declaration file updates
  • New/updated tests are included and passing (required for new features and enhancements)
  • Existing test suites are passing
  • CodeCov for patch has met target (all changes/updates have been tested)
  • The changes have not impacted the functionality of other components or directives
  • ARIA Accessibility has been taken into consideration (Does it affect screen reader users or keyboard only users? Clickable items should be in the tab index, etc.)

If adding a new feature, or changing the functionality of an existing feature, the PR's
description above includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

@vercel
Copy link

vercel bot commented May 22, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/bootstrap-vue/bootstrap-vue/ks54gzb81
Preview: https://bootstrap-vue-git-fork-paweltatarczuk-bugfix-5425-fix-i-29e876.bootstrap-vue.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 22, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5ae0812:

Sandbox Source
BootstrapVue Starter Project Configuration

@tmorehouse
Copy link
Member

tmorehouse commented May 22, 2020

This will need to be tested on mobile/touch-based iOS Safari (as some versions of safari have issues with focus events when a click/touch event is not registered between the <body> and the event with the focus handler).

@jackmu95 could you test with Safari?

@tmorehouse tmorehouse changed the title fix(b-form-tags): fix input focus upon clicking on nested element fix(b-form-tags): fix input focus upon clicking on nested element (fixes #5425) May 22, 2020
@tmorehouse
Copy link
Member

tmorehouse commented May 22, 2020

The other issue is that users can create their own custom interface, and forcing focus to the input may break their custom interface (unless they prevent focus from bubbling)

@paweltatarczuk
Copy link
Contributor Author

paweltatarczuk commented May 23, 2020

The other issue is that users can create their own custom interface, and forcing focus to the input may break their custom interface (unless they prevent focus from bubbling)

Maybe instead of changing the type of event add the onClick handler explicitly to ul and div elements or include these elements in the initial onClick condition? But then a custom interface might encounter the same issue.


Also this change seems to introduce a breaking change - click trigger on wrapper no longer results with input focus.

@jacobmllr95
Copy link
Member

jacobmllr95 commented May 25, 2020

The current implementation in this PR completely breaks tab handling when clicking on a tag.

I think we should add the same click handling we currently have on the outer wrapper to the <ul> and the "Add button" when in invisible state.

@codecov
Copy link

codecov bot commented May 25, 2020

Codecov Report

Merging #5429 into dev will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               dev     #5429   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          277       277           
  Lines         9014      9022    +8     
  Branches      2494      2497    +3     
=========================================
+ Hits          9014      9022    +8     
Flag Coverage Δ
#unittests 100.00% <100.00%> (ø)
Impacted Files Coverage Δ
src/components/form-tags/form-tags.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ee9830...5ae0812. Read the comment docs.

@jacobmllr95
Copy link
Member

jacobmllr95 commented May 25, 2020

@paweltatarczuk I tweaked the original click handler to also look for other click events inside the <b-form-tags> component and only exclude clicks from or from within <b-tag>.

Can you please check out the deployment preview and see if the behavior works for you like it should?

The onClick() handler could be tweaked a bit further to exclude other elements that are likely to be able to gain focus. Maybe we should make this configurable by a prop.

@paweltatarczuk
Copy link
Contributor Author

paweltatarczuk commented May 25, 2020

Can you please check out the deployment preview and see if the behavior works for you like it should?'

Works great for my case!

The onClick() handler could be tweaked a bit further to exclude other elements that are likely to be able to gain focus. Maybe we should make this configurable by a prop.

Maybe don't re-focus when clicked element already has a focus?

if (... evt.target !== document.activeElement)

@tmorehouse
Copy link
Member

tmorehouse commented May 25, 2020

Or filter out elements such as button, input, select, a? or elements with a tabindex set?

@jacobmllr95
Copy link
Member

jacobmllr95 commented May 25, 2020

I've added a check to skip the input focus when the event target has focus and the new ignoreInputFocusSelector prop matches a element (defaults to '.b-form-tag').

@jacobmllr95 jacobmllr95 added Type: Enhancement Type: Feature PR: Minor Requires minor version bump and removed Type: Fix PR: Patch Requires patch version bump labels May 28, 2020
@jacobmllr95 jacobmllr95 changed the title fix(b-form-tags): fix input focus upon clicking on nested element (fixes #5425) feat(b-form-tags): add ignoreInputFocusSelector prop to make input focus behavior configurable (fixes #5425) May 28, 2020
@jacobmllr95 jacobmllr95 changed the title feat(b-form-tags): add ignoreInputFocusSelector prop to make input focus behavior configurable (fixes #5425) feat(b-form-tags): add ignoreInputFocusSelector prop to make input focus behavior configurable (closes #5425) May 28, 2020
@tmorehouse
Copy link
Member

tmorehouse commented May 28, 2020

Would this be a breaking change for people who might have a custom render content?

@jacobmllr95 jacobmllr95 self-requested a review Jun 7, 2020
@jacobmllr95 jacobmllr95 merged commit 26d5953 into bootstrap-vue:dev Jul 15, 2020
12 of 13 checks passed
@jacobmllr95
Copy link
Member

jacobmllr95 commented Jul 15, 2020

Thanks @paweltatarczuk!

jacobmllr95 added a commit that referenced this issue Jul 28, 2020
* chore(deps): update all non-major dependencies (#5430)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* fix(b-form-checkbox-group): only emit `input` when value loosely changes (#5432)

* fix(b-form-checkbox-group, b-form-radio-group): only emit `input` when value loosely changes

* Update loose-equal.js

* Update form-checkbox-group.spec.js

* chore(deps): update all non-major dependencies (#5440)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update devdependency vue-router to ^3.3.0 (#5443)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update all non-major dependencies (#5445)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update devdependency rollup to ^2.11.2 (#5446)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore: Change Bootstrap v4.3.x to 4.5.x in README (#5447)

Since v2.15 Bootstrap-Vue supports Bootstrap v4.5

* chore(deps): update all non-major dependencies (#5451)

* chore(deps): update all non-major dependencies

* Use `toBeEmptyDomElement()` instead of deprecated `toBeEmpty()`

* Correct typo

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(deps): update devdependency vue-router to ^3.3.2 (#5454)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): remove unused `gh-pages` dependency (#5455)

* chore(deps): update devdependency gh-pages to v3

* Remove `gh-pages` dependency

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(deps): update all non-major dependencies (#5458)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* Update all bootstrap doc links to latest version (#5450)

Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(deps): update devdependency rollup to ^2.12.1 (#5463)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update all non-major dependencies (#5466)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update devdependency @nuxtjs/sitemap to ^2.3.1 (#5468)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update devdependency lint-staged to ^10.2.9 (#5470)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(b-avatar): convert line endings to Unix (#5469)

Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore: convert all line endings to unix (#5474)

* chore(deps): update all non-major dependencies (#5478)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update all non-major dependencies (#5482)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update devdependency eslint-plugin-import to ^2.21.2 (#5487)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update devdependency @testing-library/jest-dom to ^5.10.0 (#5493)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update all non-major dependencies (#5495)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* first attempt (#5462)

Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(deps): update all non-major dependencies (#5499)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update devdependency eslint-plugin-prettier to ^3.1.4 (#5501)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update devdependency @nuxtjs/sitemap to ^2.3.2 (#5503)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update devdependency terser to ^4.8.0 (#5505)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update all non-major dependencies (#5508)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(docs): fix typo in sidebar README (#5494) (#5510)

* chore: update auto format config (#5526)

* chore(deps): update all non-major dependencies (#5511)

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(deps): update all non-major dependencies (#5531)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* fix typo (#5534)

* remove mention of `router-tag` from button docs (#5535)

Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* fix(b-table): prevent endless reevaluation when using v-model and object/array literal prop values (#5554)

* Update devDependency sass-loader to v9 (#5546)

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* fix(b-img): Allow empty `alt` prop (fixes #5524) (#5545)

* Allow empty `alt`

* default to null to avoid check

* remove unused import

* add avatar support

* add test cases

* spelling

Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(deps-dev): bump standard-version from 8.0.0 to 8.0.1 (#5576)

Bumps [standard-version](https://github.com/conventional-changelog/standard-version) from 8.0.0 to 8.0.1.
- [Release notes](https://github.com/conventional-changelog/standard-version/releases)
- [Changelog](https://github.com/conventional-changelog/standard-version/blob/master/CHANGELOG.md)
- [Commits](conventional-changelog/standard-version@v8.0.0...v8.0.1)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat(b-form-tags): add `ignoreInputFocusSelector` prop to make input focus behavior configurable (closes #5425) (#5429)

* fix(b-form-tags): fix input focus upon clicking on nested element

* Update form-tags.js

* Add `ignoreInputFocusSelector` prop

* Update form-tags.js

* Add comment and more selectors to ignoreInputFocusSelector in form-tags.js

Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(deps): update all non-major dependencies (#5533)

* chore(deps): update all non-major dependencies

* Bump BundleWatch limits for new Bootstrap Icons

* Regenerate icon files

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(docs): add an example to `<b-input-group>`'s using icons (#5537)

* Adding an example to input-groups using icons

* Update README.md

Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(deps): update devdependency @nuxtjs/google-analytics to ^2.4.0 (#5583)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* fix(b-icon): use `aria-label` attribute instead of `alt` (#5581)

* fix(b-tags): replace spacing utility with static CSS (fixes #5523) (#5544)

* remove spacing utility

* use mt-auto for better centering

* update

* add new class to avoid issues with custom rendering

Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(docs): improve icons page (#5579)

* feat(docs): improve icons page

* Actually use `bootstrapIconsCount` variable

* Move icon explorer to the bottom

* chore: regenerate `yarn.lock` (#5585)

* fix(b-form-tags): unit test (#5586)

* chore(deps): update devdependency rollup to ^2.22.0 (#5589)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update all non-major dependencies (#5590)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* feat(docs): launch themes page with first BootstrapVue theme (#5549)

* docs(footer): uncomment link for themes

* docs(header): uncomment link for themes

* docs(sidebar): uncomment link for themes

* docs(intro/README): uncomment link for themes

* docs(theming/README): uncomment link for themes

* feature(themes): add first Bootstrap Vue & Creative Tim theme

* style(themes): prettify themes files

* fix(themes): solve typo

* Use `@nuxt/content` for themes

* Update index.vue

* Update themes.vue

* Don't pin `@nuxt/content`

* Update themes.vue

* Update themes.vue

Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* fix: properly handle special characters in user-provided IDs (closes #4927, #5561) (#5564)

* fix(b-form-group): make it work for ids with special characters like "/"

Special characters are allowed in HTML5 (https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id) but need to be escaped when used in a selector for usage in e.g. "querySelector"

Refs #5561

* Use own `cssEscape()` util + use/test everywhere needed

Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(ci): update `actions/cache` to v2 (#5580)

* chore(ci): update `actions/cache` to v2

* Update test.yml

* Update test.yml

* fix(b-form-tags): unit tests

* Revert "fix(b-form-tags): unit tests"

This reverts commit 20ebc04.

* Split actions

* Run BundleWatch during build

* Update build.yml

* Revert "Update build.yml"

This reverts commit ed4ad3d.

* Update build.yml

* chore: replace `packagequality` badge with `codacy` in README (#5596)

* chore: replace `packagequality` badge with `codacy` in README

* Update README.md

* chore(deps): update devdependency rollup to ^2.22.2 (#5597)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update devdependency rollup to ^2.23.0 (#5603)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* remove redundant height declaration in .b-sidebar (#5606)

* chore(deps): update devdependency eslint-plugin-jest to ^23.18.2 (#5607)

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>

* chore(deps): update all non-major dependencies (#5609)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(docs): fix Bootstrap browser and devices link

* chore(ci): fix BundleWatch token name

* chore: add back `packagequality` badge to README

* chore: prettify

* chore: update contributors

* chore(deps): update devdependency eslint-plugin-jest to ^23.19.0 (#5611)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore: add script to generate release notes (#5612)

* chore: bump version to 2.16.0 and update changelog (#5614)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: Troy Morehouse <troymore@nbnet.nb.ca>
Co-authored-by: TitanFighter <bear_ukraine@bigmir.net>
Co-authored-by: Hiws <rni@nova-c.dk>
Co-authored-by: Vitaly Slobodin <vitaly_slobodin@fastmail.com>
Co-authored-by: Sergey Skrynnikov <minotaar.hh@gmail.com>
Co-authored-by: James George <jamesgeorge998001@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Paweł Tatarczuk <paweltatarczuk@gmail.com>
Co-authored-by: Ivan Gonzalez <scratchmex@gmail.com>
Co-authored-by: Hiws <hiws@live.dk>
Co-authored-by: Nazare Emanuel-Ioan <emanuelioannazare@gmail.com>
Co-authored-by: Dieter Geerts <dieter@dworks.be>
Co-authored-by: michel milano <mmilano@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants