Handle keyboard-dependent focus outlines in Vuex #4847

jonboiser · 2019-01-31T20:47:35Z

Summary

vendors and modifies modality.js helper to manipulate vuex theme module. manages a core.modality property in vuex
rewrites $coreOutline getter to conditionally apply outline styles depending on value of core.modality

Reviewer guidance

Interact with links, buttons, and all kinds of inputs (inside and outside of exercises) to see if keyboard focus works okay

References

Fixes #4822

Contributor Checklist

PR process:

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Testing:

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Gherkin stories have been updated
Unit tests have been updated

Reviewer Checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.rst

indirectlylit · 2019-01-31T21:01:26Z

kolibri/core/assets/src/core-app/vuexModality.js

+      }, 100);
+    },
+    true
+  );


I know this was probably just copied over during the port so fine to leave as-is. However heads-up that we have two throttling libraries already included:

https://github.com/pelotoncycle/frame-throttle for high FPS stuff

lodash throttle for other stuff

rtibbles · 2019-01-31T21:27:01Z

kolibri/core/assets/src/core-app/vuexModality.js

+    console.error("Couldn't find any matchesSelector method on document.body.");
+  })();
+
+  const disableFocusRingByDefault = function() {


Do we need this function here? Presumably the Keen-UI helper is still doing this part?

rtibbles · 2019-01-31T21:28:35Z

kolibri/core/assets/src/core-app/vuexModality.js

+    'focus',
+    e => {
+      if (hadKeyboardEvent || focusTriggersKeyboardModality(e.target)) {
+        kolibriGlobal.coreVue.vuex.store.commit('SET_MODALITY', 'keyboard');


Any reason we can't just import the store here, rather than referencing the global?
Should be able to do: import store from 'kolibri.coreVue.vuex.store'; and then just reference that, no?

rtibbles · 2019-01-31T21:28:48Z

kolibri/core/assets/src/core-app/vuexModality.js

+  document.body.addEventListener(
+    'blur',
+    () => {
+      kolibriGlobal.coreVue.vuex.store.commit('SET_MODALITY', null);


rtibbles

Can do a little bit of cleanup in the vendored code, but looks like the right fix!

indirectlylit · 2019-01-31T23:18:04Z

tested and it seems to work well

note there are broken snapshot tests

codecov · 2019-01-31T23:24:21Z

Codecov Report

Merging #4847 into develop will increase coverage by 0.15%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4847      +/-   ##
===========================================
+ Coverage    74.96%   75.11%   +0.15%     
===========================================
  Files          261      261              
  Lines        12282    12282              
  Branches      1484     1484              
===========================================
+ Hits          9207     9226      +19     
+ Misses        2744     2732      -12     
+ Partials       331      324       -7

Impacted Files	Coverage Δ
kolibri/core/content/utils/channel_import.py	`90.93% <0%> (+0.87%)`	⬆️
kolibri/core/content/models.py	`94.14% <0%> (+0.97%)`	⬆️
kolibri/core/content/utils/annotation.py	`92.94% <0%> (+1.17%)`	⬆️
kolibri/core/device/api.py	`97.18% <0%> (+1.4%)`	⬆️
kolibri/core/notifications/models.py	`91.66% <0%> (+4.16%)`	⬆️
kolibri/core/deviceadmin/utils.py	`98.71% <0%> (+5.12%)`	⬆️
kolibri/core/content/test/sqlalchemytesting.py	`92% <0%> (+8%)`	⬆️
kolibri/core/apps.py	`100% <0%> (+8.69%)`	⬆️

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 783a511...e5e0520. Read the comment docs.

codecov · 2019-01-31T23:24:21Z

Codecov Report

Merging #4847 into develop will decrease coverage by 23.75%.
The diff coverage is 3.92%.

@@             Coverage Diff             @@
##           develop   #4847       +/-   ##
===========================================
- Coverage    74.96%   51.2%   -23.76%     
===========================================
  Files          261     864      +603     
  Lines        12282   26668    +14386     
  Branches      1484    3552     +2068     
===========================================
+ Hits          9207   13656     +4449     
- Misses        2744   12268     +9524     
- Partials       331     744      +413

Impacted Files	Coverage Δ
kolibri/core/assets/src/views/ImmersiveToolbar.vue	`100% <ø> (ø)`
...facility_management/assets/src/views/UserTable.vue	`0% <ø> (ø)`
kolibri/core/assets/src/views/KNavbar/index.vue	`69.69% <ø> (ø)`
kolibri/core/assets/src/core-app/index.js	`0% <0%> (ø)`
kolibri/core/assets/src/core-app/vuexModality.js	`0% <0%> (ø)`
kolibri/core/assets/src/state/modules/theme.js	`71.42% <50%> (ø)`
...sets/src/views/InteractionList/InteractionItem.vue	`11.11% <0%> (ø)`
kolibri/core/assets/src/timer.js	`24.13% <0%> (ø)`
kolibri/core/assets/src/heartbeat.js	`77.89% <0%> (ø)`
...s/style_guide/assets/src/views/StyleGuideIndex.vue	`0% <0%> (ø)`
... and 607 more

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 783a511...e5e0520. Read the comment docs.

indirectlylit

merging in - created follow up issue at #4859

jonboiser added 3 commits January 31, 2019 12:38

Vendor modality.js helper and modify for vuex

5e50756

Use checkbox col class in user tale

0fedb10

Add $coreOutlineAnyModality for one non-focus usage

13fcf3e

jonboiser added the TODO: needs review Waiting for review label Jan 31, 2019

jonboiser added this to the 0.12.0 milestone Jan 31, 2019

jonboiser requested a review from rtibbles January 31, 2019 20:47

indirectlylit reviewed Jan 31, 2019

View reviewed changes

rtibbles reviewed Jan 31, 2019

View reviewed changes

Update snaps

e5e0520

indirectlylit mentioned this pull request Feb 1, 2019

follow-up from outline bug fixing work #4859

Closed

indirectlylit approved these changes Feb 1, 2019

View reviewed changes

indirectlylit merged commit bf034ee into learningequality:develop Feb 1, 2019

jonboiser deleted the focus-styles branch February 5, 2019 18:41

jonboiser removed the TODO: needs review Waiting for review label Sep 9, 2019

indirectlylit mentioned this pull request May 20, 2022

Kdropdown keyboard navigation learningequality/kolibri-design-system#185

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle keyboard-dependent focus outlines in Vuex #4847

Handle keyboard-dependent focus outlines in Vuex #4847

jonboiser commented Jan 31, 2019

indirectlylit Jan 31, 2019

rtibbles Jan 31, 2019

rtibbles Jan 31, 2019

rtibbles Jan 31, 2019

rtibbles left a comment

indirectlylit commented Jan 31, 2019

codecov bot commented Jan 31, 2019

codecov bot commented Jan 31, 2019 •

edited

Loading

indirectlylit left a comment

Handle keyboard-dependent focus outlines in Vuex #4847

Handle keyboard-dependent focus outlines in Vuex #4847

Conversation

jonboiser commented Jan 31, 2019

Summary

Reviewer guidance

References

Fixes #4822

Contributor Checklist

Reviewer Checklist

indirectlylit Jan 31, 2019

Choose a reason for hiding this comment

rtibbles Jan 31, 2019

Choose a reason for hiding this comment

rtibbles Jan 31, 2019

Choose a reason for hiding this comment

rtibbles Jan 31, 2019

Choose a reason for hiding this comment

rtibbles left a comment

Choose a reason for hiding this comment

indirectlylit commented Jan 31, 2019

codecov bot commented Jan 31, 2019

Codecov Report

codecov bot commented Jan 31, 2019 • edited Loading

Codecov Report

indirectlylit left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 31, 2019 •

edited

Loading