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.

Sign up for GitHub

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 tab focus & keyboard enter event to DrillDown & ViewDrd #802

Merged
merged 17 commits into from
Dec 7, 2023
Merged

Add tab focus & keyboard enter event to DrillDown & ViewDrd #802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
693530c
ViewDrdComp: Add tabIndex and include focus outline styling, DrillDo…
BrianJVarley Nov 16, 2023
fa70258
DrillDown: remove tabIndex as overlay button will receive focus natively
BrianJVarley Nov 16, 2023
f349730
ViewDrdComp: remove tabindex attr. since button natively receives foc…
BrianJVarley Nov 17, 2023
05e9dea
view-drd-button: remove custom focus style, now provided by button el…
BrianJVarley Nov 17, 2023
5ea5a0d
clean up unused code in PR
BrianJVarley Nov 23, 2023
3d772d3
fix: add type=button
barmac Nov 28, 2023
146a9c9
DrillDown: Add blue background & white color styling to button elemen…
BrianJVarley Nov 28, 2023
53af083
drilldown: remove unused css class
BrianJVarley Nov 28, 2023
c41fde6
dmn-js-drd.css: remove background color on drilldown button so Browse…
BrianJVarley Dec 4, 2023
7ee24ff
dmn-js-drd.css: remove background color on drilldown button so Browse…
BrianJVarley Dec 4, 2023
77c13cc
chore: improve colors
barmac Dec 6, 2023
d7522bb
fix: show outline for view-drd button
barmac Dec 6, 2023
990a8d2
feat: display span for non-interactive
barmac Dec 6, 2023
8937133
test: remove exclusive test
barmac Dec 6, 2023
ecc4f26
test: revert unnecessary change
barmac Dec 6, 2023
35100ba
increase outline offset
barmac Dec 7, 2023
539ad76
lol
barmac Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/dmn-js-decision-table/assets/css/dmn-js-decision-table.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,6 @@
color: var(--view-drd-button-color);
font-weight: bold;
cursor: pointer;
outline: none;
}

.dmn-decision-table-container .view-drd .view-drd-button:hover {
Expand Down
16 changes: 13 additions & 3 deletions packages/dmn-js-drd/assets/css/dmn-js-drd.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,21 @@

.djs-overlay .drill-down-overlay {
font-size: 16px;
background: var(--drill-down-overlay-background-color);
color: var(--drill-down-overlay-color);
text-align: left;
padding: 1px;
}

.djs-overlay .drill-down-overlay > button,
.djs-overlay .drill-down-overlay > span {
display: block;
padding: 2px 4px;
margin: 0;
border: none;
border-radius: 1px;
padding: 0 2px;
background: var(--drill-down-overlay-background-color);
color: var(--drill-down-overlay-color);
font-size: inherit;
outline-offset: 2px;
}

.dmn-definitions {
Expand Down
8 changes: 5 additions & 3 deletions packages/dmn-js-drd/src/features/drill-down/DrillDown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,12 @@ export default class DrillDown {
* CSS class that will be added to overlay in order to display icon.
*/
addOverlay(element, className) {
const enabled = this._config.enabled !== false;

const html = domify(`
<div class="drill-down-overlay">
<span class="${className}"></span>
${ enabled ? `<button type="button" class="${className}"></button>` :
`<span class="${className}"></span>`}
</div>
`);

Expand All @@ -84,9 +87,8 @@ export default class DrillDown {
});

// TODO(nikku): can we remove renamed to drillDown.enabled
if (this._config.enabled !== false) {
if (enabled) {
domClasses(html).add('interactive');

this.bindEventListener(element, html, overlayId);
}
}
Expand Down
28 changes: 27 additions & 1 deletion packages/dmn-js-drd/test/spec/features/drill-down/DrillDownSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ import {
} from 'min-dom';

import {
triggerClick
triggerClick,
triggerFocusIn,
triggerKeyEvent
} from 'dmn-js-shared/test/util/EventUtil';

var diagramXML = require('./DrillDown.dmn');
Expand Down Expand Up @@ -215,6 +217,7 @@ describe('features - drilldown', function() {

// then
expect(matches(overlayEl, '.interactive')).to.be.false;
expect(query('button', overlayEl)).to.be.null;
expect(iconEl).to.exist;
});

Expand Down Expand Up @@ -252,6 +255,29 @@ describe('features - drilldown', function() {
}
));

// TODO(@barmac): browser emits 'click' event on ENTER key press
// but not for synthetic events
it.skip('on Enter key pressed', inject(
function(eventBus, drillDown, elementRegistry) {

// given
var drillSpy = sinon.spy(drillDown, 'drillDown');

var overlayEl = queryOverlay('Decision_Table');
console.log(overlayEl.innerHTML);

// when
// press ENTER
triggerFocusIn(overlayEl);
triggerKeyEvent(overlayEl, 'keydown', 13);

// then
expect(drillSpy).to.have.been.calledOnce;

expect(drillSpy).to.have.been.calledWith(overlayEl);
Copy link
Contributor Author

@BrianJVarley BrianJVarley Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Working on fixing this test, at the moment overlayEl does return the button element, but the drillSpy is not called in test. After the ENTER key event has been triggered.

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't able to fix this test too. It looks like in Chrome the synthetic keyboard event for enter doesn't trigger "click" handler, but a trusted keyboard event does. So this won't work:

const event = new KeyboardEvent('keydown', {
  key: 'Enter',
  code: 'Enter',
  which: 13,
  keyCode: 13,
});
mybutton.dispatchEvent(event);

We could add an event handler for Enter (and also Space), or just trust in browser behavior, and leave it untested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah seems fine to skip this test, as the browser will trigger the click event on a button element by default on ENTER key event.

}
));

});


Expand Down
1 change: 0 additions & 1 deletion packages/dmn-js-literal-expression/assets/css/dmn-js-literal-expression.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,6 @@
color: var(--view-drd-button-color);
font-weight: bold;
cursor: pointer;
outline: none;
}

.dmn-literal-expression-container .view-drd .view-drd-button:hover {
Expand Down