Add focus attribute support

[manuel-guilbault]

I'm submitting a feature request

• Library Version:
  0.10.0

Current behavior:
Trying to use the built-in focus attribute on UX components (e.g.: ux-input or ux-textarea) doesn't work.

Expected/desired behavior:
Since focus is a built-in attribute, one would expect it to work with UX focusable components.

Analysis

I see two ways to implement this.

A first solution would be to expose a two-way bindable focus property, which would in turn be bound on the underlying input/select/textarea (this is already what's done for ux-textarea):

export class UxTextArea implements UxComponent {
  // rest of class details omitted
  @bindable({ defaultBindingMode: bindingMode.twoWay }) public focus = false;
}

<template role="textbox">
  <textarea focus.bind="focus"></textarea>
</template>

However, after a couple of tests with ux-textarea, it doesn't seem to work. It looks like Aurelia gives precedence to custom attributes over a custom element's bindable properties when there's a name conflict. This means that when aurelia-templating-resources is loaded, any property named focus on a custom element will be ignored and the focus custom attribute will be used instead. So, implementing this solution would require to change how Aurelia resolves attributes naming conflicts.

A second solution would be to add the focus and blur methods to the component's HTML element (through the custom prototype), so the built-in focus attribute can work properly (it works by calling those methods when the value bound to it changes):

export interface UxInputElement extends HTMLElement {
  value: any;
  focus(): void;
  blur(): void;
}

// Rest of code omitted

const uxInputElementProto = Object.assign(
  Object.create(HTMLElement.prototype, {
    value: {
      get() {
        return getVm<UxInput>(this).getValue();
      },
      set(value: any) {
        getVm<UxInput>(this).setValue(value);
      }
    }
  }), {
    focus() {
      getVm<UxInput>(this).focused = true;
    },
    blur() {
      getVm<UxInput>(this).focused = false;
    }
  }
});

[bigopon]

Adding focus and blur to prototype helps align the custom element closer to the native behavior, would be nice if you could create a PR for it @manuel-guilbault

[bigopon]

Also I think all inputable custom elements need to expose methods blur and focus sooner / later, so it's will be a conflict if focus is made bindable property.

@ZHollingshead @EisenbergEffect thoughts ?

[EisenbergEffect]

I agree that focus/blur APIs on relevant components is an important part of what we need to do.

[manuel-guilbault]

Other than ux-input and ux-textarea, what other components should implement this API?
I'd say ux-button, ux-checkbox, ux-radio, ux-switch, ux-chip-input, ux-select and ux-datepicker.

[manuel-guilbault]

I've also noticed the following issue with ux-checkbox, ux-radio and ux-switch: they don't get the focus when you click them.

[ben-girardet]

• I've started to tackle this issue for the textarea component. platform5@a37b2df
• I've also added support for material design labels (similar to what has been done with the ux-input component). platform5@1a6582b
• And also fix the styling (so that it also happen at binding, and not only when the theme bindable changes). platform5@b3f6b0f

Unfortunately I'm yet unable to build properly the library so I don't feel like doing any PR yet with my work.

So here is a call for help. If I can start building properly the library on my machine I would be more than happy to continue contributing.

In order to test locally I managed to build the textarea component on its own, but not the whole aurelia/ux library using lerna. (see my issue here: https://discourse.aurelia.io/t/contributing-to-aurelia-ux/3023). Once the single component is built, I copy/paste the textarea/dist folder on a local project and can test... but it's not very practical...

I'm working on Mac OS X (Mojave), with Node 10.14.2, NPM 6.6.0, Typescript 3.6.4.

I say it again, I would really appreciate help so that I can properly build the library (with lerna) and start making PRs. Many thanks!

[bigopon]

@ben-girardet thanks for this, I'll setup something so folks can easily verify their changes/local setup & contribute

[khuongduybui]

something so folks can easily verify their changes
Storybook?

[bigopon]

Storybook?

For now, it should be a simpel local app that alias Ux dependencies to local packages, so you get faster feedback cycle via simple npm run dev script.
I think then folks with more storybook knowledge/experience can take over from there

[khuongduybui]

Storybook?

I think then folks with more storybook knowledge/experience can take over from there

No promises but I'll try.

[bigopon]

@khuongduybui glad to hear that :)

And i've finished the setup locally. Now you can simply do:

npm ci
cd app # local app development
npm ci
npm run dev

-> start editing ux packages and get live reload

Will push up the code later tonight

[bigopon]

@ben-girardet an small update:
I've pushed the changes here to my branch at https://github.com/bigopon/aurelia-ux/blob/dev-test-app but it will be a bit more work before a PR. Though it's already in good working condition, as you can see from this combo:

https://github.com/bigopon/aurelia-ux/blob/dev-test-app/app/src/app.html
https://github.com/bigopon/aurelia-ux/blob/dev-test-app/app/src/app.ts

[ben-girardet]

Wouaw thank you so much @bigopon for the great support on this issue and aurelia/dependency-injection#187

Using you dev-test-app branch I can now

• successfully build aurelia-ux project using lerna
• successfully build a package
• and the test app with link to dependencies is fantastic to work with

Now, what do you recommend for me to do? Should I wait for a PR in main repo, or would it be better to fork your fork ? My concern is that if I fork your repo, I'm not quite sure how to properly do PRs.

[bigopon]

@ben-girardet glad to hear you were able to verify it. I also prepared an easy way to build directly into a folder in different path, making it friendlier for testing with existing apps.
You can do that with

cd packages/textarea
npm run build:dev:watch -- --environment target_dir: relative/path/to/my/app

Besides that, before we proceed with adding focus behavior on the ux inputable element, we need to define how they will behave, aka some specs. Would be nice if you could have some short notes for it to discuss easier.

And finally, would be nice if you (and others) could help contributing further to UX, we are quite short in contribution here 😙

[khuongduybui]

I think we should wait a little bit for @bigopon changes to get merged back into main repo. At least that's what I'll be doing.

[bigopon]

@ben-girardet @khuongduybui I've completed refacotring all the component builds, and updated my branch. https://github.com/bigopon/aurelia-ux/blob/dev-test-app/app/src/app.html

A few final touches and it should be ready to go

[ben-girardet]

@ben-girardet glad to hear you were able to verify it. I also prepared an easy way to build directly into a folder in different path, making it friendlier for testing with existing apps.
You can do that with

cd packages/textarea
npm run build:dev:watch -- --environment target_dir: relative/path/to/my/app

Besides that, before we proceed with adding focus behavior on the ux inputable element, we need to define how they will behave, aka some specs. Would be nice if you could have some short notes for it to discuss easier.

And finally, would be nice if you (and others) could help contributing further to UX, we are quite short in contribution here 😙

Can't wait to work with this new tooling. It definitely motivates me to contribute. I'm very new to the open source world and therefore might need some guiding along the way.

As I'm using Aurelia UX in various projects since nearly the beginning I really would like to help the project move forward. At the moment I'm planning to contribute towards:

• fix this focus issue because it's very important in so many use cases. For exemple I'm using Aurelia UX in Cordova applications and need to manage focus related to the soft keyboard
• fix quite a few mistakes in CSS variables. When trying to use the components together, there are noticeable differences in look and feel between them. I would like to have better alignment between components and also clearly defines the themable properties. There are quite a few properties in themes (exemple UxTextaraTheme) that are not used.
• talking about alignment, I think the ux-textarea component should behave like the ux-input. When providing a label to ux-input, it behaves in a material design way with the floating label. Textarea would benefit from having the same approach.

Hopefully by helping out with good-first-issue kind of issues I will gain confidence and be able to contribute more :-)

Back to the focus attribute discussion: from my understanding of this thread:

• we need to have a public focus() and blur() api on the components
• we must not have a focus bindable, in order to avoid name conflict. I suggest using focused (as it's already the case in ux-input)
• we need to extend the prototype of the custom element, adding the focus() and blur() methods (so that it works with the Aurelia focus custom attribute)
• we need to define the list of ux-components that should implement this api - and make sure the api is consistent across all of these.
• ux-input, ux-textarea, ux-checkbox, ux-radio and ux-switch have been listed on this thread

[bigopon]

@ben-girardet it's ready at #216

About the list of focus/blur behavior, it sounds like a good start, we can iterate based on that more, just need to have it somewhere, so if you are going to create a PR for that, lets put the list in a design-decisions.md or something

[ben-girardet]

@bigopon thank you !

When you say it's ready, I still need to wait for it to be merged before I can fork and start making a PR for the focus attribute. Right ?

[ben-girardet]

Started to tackle the focus attribute issue here:

https://github.com/platform5/ux/tree/focus-attribute

When writing the design decision I went to the conclusion that components that don't actually have a keyboard input feature don't need the focus() and blur() api if they work correctly with TAB. But maybe I'm missing something and they should also implement focus() and blur(). Any thoughts ?

Regarding ux-select I'm facing two questions:

1. Because it can be opened (to display the list of options), I think it should also implement the focus() api and allow this focus() method to open the list. Moreover, as a future feature, we could add a search input on top of the list (for selects containing 10+ or 20+ options) so that user can filter out options to quickly find the one to select. Therefore this component might need keyboard input in this scenario. What do you think ?

2. I'm struggling to correctly blur the component when clicking outside. The blur works correctly if the user has clicked on the option list and then click outside, but it doesn't work if the component has been focused by focus() or by the focus custom attributes. I don't know what I'm missing... seems like the focus mechanism is not properly implemented. Any idea?

Related commit for ux-select : platform5@20f4281

[bigopon]

A general direction we should align ourselves to is standard.it couls be difficult to implement because we have to keep track od many things.

About the component with typing you mentioned, we may need to do it ina different component instead, called combobox maybe?

[ben-girardet]

What is standard.it ?

Good idea to separate ux-select component with another one with such capabilities. Therefore I’m thinking that we can keep ux-select as is without implementing the focus() and blur() API.

[bigopon]

Standard. It
I was typing on the phone. About the component, yes leta make it that way. So folks can think about ux as standard like components, and there' extra components like combobox