In T275914#7156632, @DannyS712 wrote:

In T275914#7156516, @Catrope wrote:

For performance reasons, we'd like to avoid exposing the entire set of icons, because it's so large. Instead, I think we'll want to offer two options:

• Using a build step, tree-shake just the icons that are needed, and include those in the built bundle
• Allow RL modules to list the icons they need in their module definition, and build some ResourceLoaderImageModule-style magic to automatically add wvuiIcons.iconName = '...svg string...'; to those modules

Neither one of those would work for onwiki scripts though, right?

I wrote a sketch of a dialog component, before abandoning it and working on other things: https://gerrit.wikimedia.org/r/c/wvui/+/699788 . It's very incomplete, but here are some of the things I had in mind while writing this.

In T281527#7158412, @sbassett wrote:

In T281527#7158003, @Catrope wrote:

There are only release candidates for version 1, which means code is constantly changing and hard to ensure project stability. It would be ideal to make sure once the 1.0 release is available, another follow up security review should be done to ensure there aren't any security issues.

We can probably just re-open this task with an updated description and a new comment pinging the Security-Team.

Thank you!

I like the idea of reducing compatibility problems by only exposing exports to package modules. That should be pretty safe, because AFAICT the scenarios are:

• Code uses exports as an implicit global: this shouldn't be happening, eslint would catch it
• Code uses exports as a declared variable or as a function parameter: this is fine, it'd just shadow the exports parameter from the wrapper function that we'd provide
• Code uses typeof exports or similar to detect its presence: I think it's safe to assume that only UMD code would do this, and the wrapper file would need to be changed in the way you describe

For performance reasons, we'd like to avoid exposing the entire set of icons, because it's so large. Instead, I think we'll want to offer two options:

• Using a build step, tree-shake just the icons that are needed, and include those in the built bundle
• Allow RL modules to list the icons they need in their module definition, and build some ResourceLoaderImageModule-style magic to automatically add wvuiIcons.iconName = '...svg string...'; to those modules

Point 1: agreed.

Hello security people (@sbassett @Mstyles), just a heads up that I'll be on vacation from right after all-hands until pretty much the end of the quarter (June 18 - 29, returning on June 30). If you have questions about this task during that time, please ping @egardner.

T280828: Create a temporary entry for all WVUI components is also relevant for how these two bundles came to be

I found this example in the ARIA docs that seemed to more closely track how our dropdown works, so I implemented its behavior instead. The significant difference is that I'm not setting role="combobox" (because that's only supposed to be set on input[type="text"] elements) and that I'm setting aria-activedescendant on the listbox instead of on (what used to be) the combobox. I found it strange that this example doesn't link the head (button element in their case) of the dropdown to the list, so I used aria-owns the way it's used in the combobox example.

In T280712#7134553, @AnneT wrote:

Regarding the OptionsMenu's slot's default implementation...

Yeah, it makes sense to try to store as much as we can in the OptionsMenu component so we don't have to re-implement the default slot (or anything else) in every component that uses it.

If we did this, we would also be able to get rid of wrapper files for UMD distributions of external libraries, like moment-module.js. UMD does assign to module.exports rather than to exports, but it uses typeof exports !== 'object' && typeof module !== 'undefined' to detect whether it's going to assign to module.exports or try something else. In our environment, UMD ends up assigning to a global variable.

Re Vue versions, with our current plans as I understand them, everything's going to have to be Vue 2 for the time being. But we should definitely develop guidelines for things to do and things to avoid that will make a future Vue 3 migration easier.

In T272879#7135122, @egardner wrote:

In T272879#7134971, @Jdlrobson wrote:

However, doing this will make it more complicated to use Storybook as a tool during component development. It would still be useful for public documentation purposes (display all the merged components and how to use them, etc). Are folks relying on Storybook to help with the initial development of new components (similar to how we use unit tests)? Or do stories usually get written after the fact. @AnneT and @Catrope would be curious as to your thoughts here especially.

Yes, I definitely use Storybook a lot, both during initial development and when making changes to components.

In T280712#7133348, @AnneT wrote:

I think these are my main issues:

1. In other libraries, "select" usually means a custom-styled element that replaces <select>, but we call that thing a dropdown
2. When I think "menu" in terms of web design, I think navigation (even though it could mean a menu of options, which is exactly the language MDN uses to describe <select>)
3. I can't always guess what a widget does, or which one I need, based on the widget names (ButtonMenuSelectWidget vs. ButtonSelectWidget, MenuOptionWidget vs. MenuSectionOptionWidget vs. MenuSelectWidget vs. MenuTagMultiselectWidget, MultioptionWidget vs. MultiselectWidget...) They make sense now that I understand OOUI better, but I don't find them intuitive.

Reasons this might not matter much:

1. We can probably get rid of some of these widgets since we're no longer relying on a model of inheritance
2. We'll have a nice demo page to show developer users what each component does (I have relied heavily on the OOUI demos page during my time here...)
3. Maybe it's just me (although we could consider chatting with some new-to-OOUI designers and developers to see what they think)

It's not just you, this is legitimately confusing, and I don't know what all of these widgets are either. I think we'll suffer from this problem much less in WVUI because 1) we'll be more careful to choose non-confusing names now that we've been burned by this issue, and 2) we'll need fewer components because a) we're not using inheritance and b) we're using data rather than individual components for options.

@Jdforrester-WMF tells me this is blocked on upgrading the version of Node used in our CI images, which is in turn blocked on SRE approving a newer version of Node (at least for pipeline jobs like the one used for WVUI, which uses a production image)

Here are some other things I thought about and/or would like reviewers of my patch to provide input on:

• What do you think of my use of slots? Should OptionsMenu have a default implementation for its slot, or can it afford not to have one, since it's an internal component? What do you think of the use of named slots with default behavior in Dropdown?
• Right now I have only one type for options (OptionMenuItem). Should there instead be a base type (OptionMenuItem) that only has what it needs (ID and disabled; label is not needed if there is no default rendering, or if the default rendering is the ID), and a specific type (DropdownItem) that extends that and adds dropdown-specific things (e.g. group header, icon)? Or are those things not really dropdown-specific and should they be supported in all components that use OptionMenu?
• The MediaSearch implementation accepts various shorthand formats for the items prop. Should we do that / add that here too, and normalize to the longhand format internally?
• What do you think of my use of v-model with custom prop and event names? Should I rename to modelValue/input so that I can use the modelWrapper composable? Or should the composable support alternative prop/event names? Should the composable use generics so that it can be used with typed props?
• Should there be styling for the default label that is shown when nothing is selected (e.g. italics)? (There's already a --value-selected CSS class that could be used for this, following MediaSearch's example.)
• How should focus be handled? The Wikit and WVUI implementations allow the dropdown gain focus when clicked, which makes it easy to hide the menu on blur and to capture key events, but because they don't prevent mousedown the text in the dropdown can be selected. The OOUI implementation prevents mousedown, so there are no selection issues, but that prevents the dropdown from gaining focus (except through the keyboard), so key events and click-away events have to be captured on the document. I chose to go the former route, but maybe we should prevent the text from being selected and prevent the dropdown from visually appearing as if it's focused unless it was focused via the keyboard?
• Which ARIA attributes are needed that I missed? I haven't added the ones from MediaSearch that rely on IDs. MediaSearch uses the name prop to build IDs, but I don't like the idea of making name a required prop (form field names also don't have to be document-unique, only unique within a form). But we could also generate random IDs at mount time and use those, if the ID-based ARIA attributes are useful.

In T280712#7022388, @AnneT wrote:

Some comments:

• Select vs. dropdown: Are we open to changing what's represented in the style guide as "dropdown" to "select" to better match the definition of these words in other libraries and places?

I agree, and my proposed patch calls the specific UI component a "Dropdown", while calling the more generic menu an "OptionsMenu".

So sorry for the late response, @Mstyles! I missed your comment and only saw it today because I was working on this task for other reasons. I've added it to our team's workboard so that we can keep a closer eye on it in the future.

In T281527#7108133, @Mstyles wrote:

@Catrope What version of the composition API do you plan to use? From the release page, it seems that v1.0.0 only has release candidates and betas. Is there a particular commit you are planning to pin to?

Another trick I learned yesterday: if you access a ref, like this.$refs.foo, TypeScript doesn't know exactly what type it is, and infers its type as Vue | Element | Vue[] | Element[]. That's not a very useful type if you want to call methods or access properties, but you can tell TypeScript what the right type is.

One thing I've noticed while doing development in WVUI is that the TypeScript integration with Vue 2 behaves strangely sometimes. For example, it only works if you explicitly specify the return type for every computed property function. If you don't specify a return type on any one computed property function that accesses this.something, then type inference breaks for all props and computed properties in all computed property functions.

With this patch:

In T282625#7087847, @RHo wrote:

Hi @Catrope - I recently captured some ways we would like to ideally extend the icons set usage on T280666:

Thanks!

@egardner and I made a proof of concept that integrates Vite with ResourceLoader to provide hot module reloading for Vue code. It turned out to be simpler to achieve than I expected. The extension implementation is here, and the associated core patch is here.

In T282625#7081261, @santhosh wrote:

1. The strong binding of available icons to javascript helps you to use quickly discover and pick an icon through autocompletion features of popular IDEs.
2. A build step or even real time checks by IDE make sure you are not refering an icon that does not exist. This is better than a loose dependency like CSS classnames

Needing to import icons in JS and pass them through to the template via data() is annoying

I would say this is positive. It is kind of "strong typing" for icons :-).

This is a good point, and I had forgotten to say: the little extra annoyance is probably worth it for performance reasons (so that tree shaking works), and I also like your framing here as "strong typing for icons", that's exactly what it is. It's also worth noting that, in cases where logic is needed to choose which icon is displayed, you need a computed function anyway, and at that point returning an Icon object instead of a string isn't any extra work.

I'd be curious to hear what other people's experiences are with these icon systems or others, what suggestions they have for what our icon system should look like, and what other factors they think we should consider in the architecture of our icon system.

Here are some pros/cons and trade-offs with WVUI's current icon system as I see them:

In T282183#7071763, @suffusion_of_yellow wrote:

Could it be this?

	if ( $user->isAnon() ) {
		$conds[] = 'actor_name<>' . $dbr->addQuotes( $user->getName() );
	} else {
		$conds[] = 'actor_user<>' . $dbr->addQuotes( $user->getId() );
	}

But actor_user is NULL for unregistered users.

FYI you can already do this without a startIcon prop, because a button (unlike an input) can contain arbitrary content:

developers will benefit from better IDE tooling and will be able to write code in a more standardized way

What do you mean by this exactly? If you mean import and export, then I think there would be additional challenges in getting those to work/coexist with ResourceLoader. Are there other language features that are only available when type="module" is used?

The deployment target date is nuanced: we plan to introduce the plugin in WVUI in the coming weeks, but that doesn't immediately result in production deployment. That would only occur once we cut a new release of WVUI and put it in MediaWiki core. We may well be ready to do that some time in May or early June, but if the security review isn't complete by then, we could hold off (or put it behind a feature flag) until we have real code in an extension using it that is ready to deploy. And we don't think we would get to _that_ stage until July (or perhaps June if things go well/fast).

See T281527: Security Readiness Review For Vue composition API plugin for more context on how this task came about.

Thanks @sbassett! I asked the other engineers on the Vue team, and it sounds like we do plan on using the composition API plugin soon (which is a plugin for Vue 2.6 that essentially adds a subset of the anticipated 2.7 features). I've opened T281527: Security Readiness Review For Vue composition API plugin to request a review of that plugin. I'll link this task there too for context. We'll plan to reopen this task when Vue 2.7 comes out.

Since it looks like Vue is never going to support IE11 in Vue 3, and instead backport some Vue 3 features to a new Vue 2.7 release (that's what's currently being proposed, and it seems likely to happen), we don't think that any WMF code will likely migrate to Vue 3 for at least a year or more. I do think we'll likely upgrade to 2.7 once it's out, and we may want to use the composition API plugin before then, to get some of those 2.7 features early (the proposal calls for this plugin to be made part of Vue itself in 2.7). So I think a security readiness review of Vue 3 can be postponed for now, and we'll probably ask for a review of Vue 2.7 when it comes out. We may also ask for a review of the composition API plugin for 2.6, I'll ask the other Vue migration team members about that.

Everything you asked about is correct, as far as I know

The attached patch proposes an implementation using approach #2 from the list above. I'm not 100% confident that this is the right approach, but it seemed like the one with the fewest downsides to me. Eager to get feedback on this.

In T278509#7016587, @gerritbot wrote:

Change 681188 had a related patch set uploaded (by Catrope; author: Catrope):

[wvui@master] Fix appearance of progressive/destructive buttons

https://gerrit.wikimedia.org/r/681188

This color is @background-color-quiet--active from wikimedia-ui-base. On a white background, this color looks the same as Base80, which is what the style guide prescribes.

In T279714#6985950, @AnneT wrote:

• I prefer to pass in the label as the main slot since it's content, it's the simplest implementation for a required element, and it might take a form other than a string (like an i18n message object). I also like the named description slot in the Wikit component, although this isn't included in the design style guide at the moment.

I like the idea of putting the label in the main slot. Speaking of labels, what are your thoughts on how we should render those? Wikit generates a random ID, then uses <input type="checkbox" id="randomValue" /><label for="randomValue">Label text</label>. MediaSearch puts the input tag inside of the label tag (<label><input type="checkbox" />Label text</label>), which avoids the need for an ID. For that reason, I lean towards the latter.

Per T278509#6986197, do you think it makes sense to add toggle switches to the scope of this task? It feels to me like they're more like checkboxes than anything else.

We should also think about how to handle toggle buttons and toggle switches. I don't think we have Vue implementations of these yet, but OOUI has them. Toggle buttons look exactly like normal buttons, except that they stay pressed after you click them (and become unpressed again when you click them again). We could consider adding a prop to the regular button component that enables this mode. Toggle switches are a bit more akin to checkboxes (see T279714), and I think it would make sense for them to be a separate component.

Another issue is that the Vite loader assumes ES6 modules are used. It uses dynamic import (await import( 'url' )) to load new versions of modules, which is an ES2020 feature. The latter is fine, we can assume that developers use recent browser versions. The former is a problem, not necessarily because of browser requirements (basic import/export is in ES6, although we probably wouldn't want to limit HMR to ES6-only modules) but because ResourceLoader doesn't support ES6 modules and would require fundamental changes to support them (for example, import/export can only be used on the top level, so wrapping module contents in functions breaks them).

Some not-fully-organized notes from digging into how HMR works in Vite, and how we might integrate it into RL:

1. Risky Patch! 🚂🔥

As other commenters have pointed out, async/await is part of ES2017, whereas the minifier only supports ES6 (ES2015) currently. Adding support for async and other post-ES6 language features to the minifier should be pretty easy. However, making those available for MediaWiki code also involves a trade-off: are the additional features worth excluding more browsers? This depends on how useful the new features are, and how much usage the non-supporting browsers have. I've started a task for exploring this here: T277675: Explore native support for ES2016 (ES7) or higher versions

I haven't yet looked at which browsers support these features and how well-used they are. That information should be added to this task next. I don't have time to do that right now; anyone else should feel free to pick that up.