Currently, there are different mixins that add keydown and keyup listeners.

We should change that so we only have those listeners in one place.
The idea is that there will be KeyboardMixin handling those.

It has to be a deduping mixin (same as DirectionMixin) to only add listeners once.

The mixin should toggle focused and focus-ring attribute.

Name is chosen to align with the :focus-visible proposal:

• Previously known and implemented in Firefox as :-moz-focus-ring
• There is a polyfill https://github.com/WICG/focus-visible
• Elix has own mixin https://github.com/elix/elix/blob/master/src/FocusVisibleMixin.js

This mixin needs to be integrated to ControlStateMixin and ItemMixin.

I am not too sure how to phrase this without sounding like a too demanding jerk.
I am well aware it is ~epic. Also, it is mentioned in different issues that it is not going to happen anytime soon.

But anyway, removing Polymer dependency would be (really) nice.

Not that I don't like Polymer, on the contrary, but it is currently not possible to use recent lit-element based Vaadin components in application running older version of Polymer (mostly because dom-module is registered twice, once for every Polymer version).

Is there any plan to replace themable-mixin (I think the only reason for Polymer Dependency) with approaches tested by @jouni here: https://jelements.netlify.app/stylable-mixin ?

Thanks,
C.

• Add helperText property
• Add slotchange for a corresponding slot
• Toggle has-helper-text attribute

See the example PRs where this is used.

• vaadin/vaadin-text-field#476
• vaadin/vaadin-custom-field#80
• vaadin/vaadin-radio-button#153
• vaadin/vaadin-checkbox#174

The current hack with updating private _styles property is a temporary solution only.

There is an issue related to adding new API in lit/lit-element#870
Once the getStyles() API is implemented, we should update ThemableElement to use that.

This means there will be a minimal lit-element version required by Vaadin components (2.3.0 or newer). We should keep that in mind when we add components to platform.

In vaadin-checkbox and vaadin-radio-button we have checked property.

checked: {
  type: Boolean,
  value: false,
  notify: true,
  observer: '_checkedChanged',
  reflectToAttribute: true
}

Then there is also aria-checked attribute and _checkedChanged observer.

We should extract that to mixin. In future it could be also used by toggle switch component.

We should extract code for track event from Polymer gestures.

The components that will need it:

• vaadin-split-layout
• vaadin-date-picker-overlay-content
• vaadin-grid (reordering, resizing)

Problem

In certain components we apply CSS conditionally based on media queries.
We don't have a unified approach for it, so components handle it differently.

1. JS API

Example: vaadin-date-picker with _fullscreenMediaQuery set from JS which toggles the fullscreen state attribute on vaadin-date-picker-overlay.

When using this approach, we currently hardcode media query also in CSS (example) which makes overriding it more complex (as it needs to be done in multiple places).

2. CSS API

Example: vaadin-app-layout with following CSS custom properties:

• --vaadin-app-layout-touch-optimized
• --vaadin-app-layout-drawer-overlay

Both of these are observed in window resize event handler (there is no platform way to observe changes to custom CSS properties so far, see also discussion about observedStyles).

Then those true / false are used to show / hide parts of the DOM and one of them is synced to overlay property (which reflects to attribute and is read-only).

While this approach lets the user to write their CSS and override the defaults, it still requires boilerplate work for every component and that doesn't sound like a good DX.

Solution proposal

Let's assume we have a mixin with the following API:

static get mediaQueries() {
  return {
    touchOptimized: '(max-width: 800px) and (min-height: 500px)'
  };
}

Then we declare property with noAccessor flag:

static get properties() {
  return {
    touchOptimized: {
      type: Boolean,
      reflect: true, // optional. set if we need to sync with an attribute
      attribute: 'touch-oprimized' // optional. set to have dash-case
    }
  };
}

For each of the properties declared like this, the mixin does the following:

• create window.matchMedia and add listener to the mediaQueries[prop]
• create custom accessors that make sure the property is read only
• toggle property value between true and false depending on media query
• when attribute is set manually, keep it and override media query based value
  • this is needed to allow forcing specific appearance e.g. touch-optimized
• when attribute is removed manually, watch to media query changes again

This approach (only allow to force the value that is not the default one) is inspired by the way how we handle dir attribute in DirMixin when set by the user to a different value).

The name is chosen based on Elix which has SelectionInViewMixin.

This mixin should contain logic from vaadin-list-mixin related to handling scroll and adjusting when item is selected.

The mixin should require to use SingleSelectionMixin.

Depends on #17

The mixin imports (that end up in JS code) are currently imported with extension.
For dumb classes / interfaces we don't really need it so it's generally omitted.

It would be nice to align these cases in order to be consistent.

This project is currently a monorepo managed by Lerna.
Whether we want to keep this is a question to decide on.

Monorepo

Pros

• It's clear what should be imported from where (single big package could become messy)
• Developers can choose what mixins to install in their components
• Bonus: we can investigate how to run tests for affected packages only (maybe to Rush)

Cons

• We can't cherry-pick to older feature branches.
• Many versions to publish and update in components

Summary

We should be very careful if we want to go with a monorepo. It could work if we do it this way:

• Every mixin only does exactly one thing it was designed to do.
• No new features to existing mixins - only bug fixes that can land in patch releases.
• New feature means there is a new mixin to be added for it.
• Changing a mixin in a way that requires adding another mixin is a breaking change.

Single npm package

Pros

• Everything in one package - easy to manage branches and releases

Cons

• Whenever new feature is added, all the components must update.

Summary

Easier in terms of cherry-picking (as soon as "next generation" becomes LTS with minors).
Potentially challenging in terms of release frequency before that (if we have many mixins).

As se will be dispatching a lot of "-changed" events, we should have some kind of a helper (in element mixin for example) that would reduce the boilerplate from:

this.dispatchEvent(
  new CustomEvent('opened-changed', {
    detail: { value: this.opened }
  })
);

To something likethis._dispatchChangedEvent('opened');

Or if there is a cleaner way, we could make it like that:

static get notifyingProperties {
 return ['opened'];
}

Example for items-changed:

We need to figure out how to specify event detail type, and use this for other mixins.

There is a part of duplicating logic in different components:

• vaadin-accordion 
• vadin-list-mixin

This logic should be extracted to a separate mixin so we can reuse it.

We might include _vertical as a protected property, and decouple it from orientation.
So technically it would allow us to have vertical accordion, alongside tabs / list box.

The mixin will be shared by vaadin-list-box and vaadin-tabs.

It should provide selected property, logic to set it on click and keydown.

The mixin should require to apply SlottedItemsMixin and KeyboardNavigationMixin

• Extract from https://github.com/vaadin/vaadin-item/blob/master/src/vaadin-item-mixin.html
• Exclude the code related to value that we need in vaadin-item but not in vaadin-tabs

Depends on #9
Depends on #10
Depends on #11

1. Extract the corresponding logic from vaadin-item-mixin to a separate mixin

2. Use that mixin in ControlStateMixin instead of having that code duplicated

The mixin would be needed to create vaadin-drawer-toggle do not depend on vaadin-button.
The part which can be extracted is related to active state and corresponding event listeners.

The code related to mousedown / touchstart event handling can be adapted from the Polymer gestures in order to not depend on GestureEventListeners mixin anymore.

Prototype of the mixin (not in TypeScript): open-wc/open-wc#1265

We should consider this option. Currently we use items property in SlottedItemsMixin and it is read-only. We also have @mediaProperty decorator that creates read-onlly properties.

We need to test components against Chromium Edge.

This needs to be added to karma config here, as well as in vaadin-component-dev-dependencies

Originally mentioned here: vaadin/vaadin-tabs#151 (comment)

We assume that SingleSelectionMixin only accept items that implement SeleectedStateMixin but we don't filter out the items here.

It resulted with an issue that I fixed in vaadin/vaadin-tabs#151

This should be fixed in a more elegant way by implementing _filterItems in SingleSelectionMixin.

Change ControlStateMixin to use the new mixins:

• DisabledStateMixin - #10
• FocusVisibleMixin - #11

Note: this would need 0.2.0 version bump.

The active state attribute set on mousedown / touchstart and removed on mouseup / touchend is currently handled differently in vaadin-button and vaadin-item-mixin.

We should have a unified implementation in ActiveStateMixin that would be used by:

• ItemMixin (#5)
• ButtonMixin (#6)

None of the code has been reviewed. This is an issue to track this.
There will be a PR only containing packages code linked to it.

The following code is going to be needed in multiple places:

type Constructor<T = object> = new (...args: any[]) => T;

const appliedMixins = new Set();

function getPrototypeChain<T>(obj: T) {
  const chain = [];
  let proto = obj;
  while (proto) {
    chain.push(proto);
    proto = Object.getPrototypeOf(proto);
  }
  return chain;
}

function wasApplied(superClass: Constructor<HTMLElement>): boolean {
  const classes = getPrototypeChain(superClass);
  return classes.reduce((res: boolean, klass) => res || appliedMixins.has(klass), false);
}

getPrototypeChain and wasApplied are used in DirectionMixin and will be also needed to implement KeyboardMixin (#46) as well as others deduping mixins.

And the Constructor type is already copy-pasted pretty much everywhere.

Time to create a common utils package.

We need a package where to place detectIosNavBar helper used by various components.
Currently it is duplicated at least in the following components:

• vaadin-app-layout
• vaadin-overlay

Other potential helpers that can be also added later.

• touch device support: context-menu, grid (see also vaadin/vaadin-grid#1222)
• IE, Safari and Firefox detection: grid, lot of tests

These should be moved to the separate package (could include a mixin, e.g. DeviceDetectorMixin or DeviceAwareMixin).

It is better to also do these checks once, in the form of constants, and then export these.

One more piece to extract from vaadin-list-mixin is code related to orientation property.

We only need it for vaadin-tabs but I still think it is worth extracting.
So it would be theoretically easier to implement one of the following:

• horizontal orientation for vaadin-accordion
• vertical orientation for vaadin-menu-bar

Someone could also extend vaadin-custom-field and apply that mixin.

The alternative would be to keep that code in vaadin-tabs only.
But IMO it's worth splitting to another mixin right away.

We should replace IronResizableBehavior with the native Resize Observer.
This API is not supported by older Safari versions, so we need to load a polyfill.

The mixin should create an observer instance in the connectedCallback in order to observe element size change, and then call disconnect() inside of disconnectedCallback.

This mixin should be used in vaadin-tabs, vaadin-form-layout etc.

In certain components, namely vaadin-checkbox and vaadin-radio-button we have this logic:

ready() {
  super.ready();

  this.shadowRoot.querySelector('[part~="label"]').querySelector('slot')
    .addEventListener('slotchange', this._updateLabelAttribute.bind(this));

  this._updateLabelAttribute();
}

_updateLabelAttribute() {
  const label = this.shadowRoot.querySelector('[part~="label"]');
  const assignedNodes = label.firstElementChild.assignedNodes();
  if (this._isAssignedNodesEmpty(assignedNodes)) {
    label.setAttribute('empty', '');
  } else {
    label.removeAttribute('empty');
  }
}

We should extract it to a mixin, e.g. @vaadin/slotted-label-mixin and use that.

Currently the _items property is protected but the components that implement mixin have to check for it in updated(props) where they test props.has('_items') which is an anti-pattern.

We should update the mixin to create custom accessors for public items property.

From #23 (comment)

Just started thinking that why do we have this kind of event handlers (with very generic name) as protected API?

I think this can cause problems if the user/dev provides a base class that already has an event handler with similar generic name and this can also soon become a problem/conflict in our own classes.

Currently these implementations don't check for possible super method, but even if they would, I think it's somehow weird that we have these generic event name based methods as part of an abstract class that should define the API for this mixin behaviour, but these method names don't seem to be any way related to the mixin behaviour in my opinion.

Should we handle these in some other way?

Current implementation with the dumb classes workarounds microsoft/TypeScript#17744.

While it works, having generic names like _onKeyDown and _onClick is questionable.
We need to agree on the API we use, to make sure it is convenient and extensible.

Originally mentioned at vaadin/vaadin-radio-button#141 (comment)

So we have wasApplied used for deduping mixins: DirectionMixin, KeyboardMixin.
And then we also have another approach in non-deduping (public) mixins:

The idea is to always make mixins deduping and use wasApplied.

That might be slightly less performant in terms of prototype chain lookup.

The corresponding fix was merged to vaadin-list-mixin: vaadin/vaadin-list-mixin@2fd88cf

So we should apply it here, too.

We already have logic for event listeners (touchstart, touchend, mousedown, mouseup):

It would be nice to extract that to a mixin, and then use it:

• refactor ActiveStateMixin to use the new mixin,
• vaadin-split-layout

We could consider providing methods like _onDown & _onUp as well.