The behavior of the enter key in the new template dialog sidebar
is somewhat inconsistent. When pressing enter on the name of a
template it sometimes just doesn't work, but focuses something
else.
I realized this is because the message "The … template doesn't
yet exist." does not have a link. There is nothing to focus in
this element. The code just gives up and the selection returns
to whatever was selected before.
It works when there is a link in the template header. But this
is not even that useful.
Let's try to always focus the first parameter instead. The user
can still press Shift + Tab to focus the link to the template
page.
Bug: T289043
Change-Id: Id314ee8ebf47d387df08c7fb432094b6d8f7a3d2
The previous patch I15aa2c0 (approved by UX) was incomplete. The
required indicator was still shown, depending on the skin.
This patch also reduces the amount of generated HTML when it
doesn't have an effect anyway. At the moment an empty <span></span>
is generated for _every_ parameter in the dialog. That's potentially
hundreds. But the element is only needed for deprecated and
(in the old UI) required parameters.
A missing space is added while we touch this code anyway. The
missing whitespace between label and indicator icon is confirmed to
be a bug by UX.
Styles that are the same on all skins are moved to the .css file
that's loaded for all skins. Missing word-wrapping for overly long
template parameter names (on the right side of the dialog) is added.
The position of the indicator icon was broken on Minerva the moment
a parameter name is a bit longer. Fixed by replacing `inline-block`
with `inline`.
Bug: T290492
Change-Id: Ie346d88969cec2effaf90d328d08567ab7b7bf75
This applies in several situations. A trivial one is a parameter
that's already in use, but you uncheck it while the relevant
error message is shown. Vice versa.
Bug: T290977
Change-Id: Ia4114194a2efe34a7d51e633c776ce892cc9cb18
This patch improves the error handling for when a user tries to add
a parameter which is either an alias of a existing parameter, the
primary name of a existing aliased parameter, or a name/alias of an
existing parameter which is shown with an override label.
The error message was modified to always refer to the conflicting
parameter using the same name that is has in the sidebar.
Example: A parameter named "Parameter B" is already present in the
sidebar under its alias "B". When a user tries to add "Parameter B",
the new error message will inform the user that the parameter they
are trying to add already exists as "B".
Bug: T285869
Change-Id: I762b72b6cf14eb8ff5fcef63b4dcb70e297050de
It's impossible to use the delete button from the
OO.ui.OutlineControlsWidget to delete a parameter when the new
sidebar is active.
This partly fixes one of the issues mentioned in I97d77f4. The
delete button might stil become active for a moment (not fixed
yet), but doesn't stay active forever any more with this fix
in place.
Apparently this also makes another workaround obsolete.
Change-Id: I0bca310772c26149170af23ff8e5505c3ce4adf4
There is no point in firing this event when noting changed.
This should reduce flickering and some of the issues described
in I97d77f4.
Change-Id: I7c387889a4a33dac5053cec11a0641d358020b56
This just copies the colors from the old sidebar.
* When hovering with the mouse (without click/press) the background
is gray, and the text black. Relevant for readability via WCAG
AAA.
* On click/press the background is blue (slightly darker than a
selection), and the text is dark blue as well.
As noted in
https://docs.google.com/document/d/1V0rXMPr6upNjHF9AkROx4R8IF1LDZUzrG4K6oWT08sU
Change-Id: I443045b55826ef390688b32616dfdcfdc6555eb3
As a reminder (not part of this patch): Pressing enter on the name of
a template should select it, and jump to the content area on the right.
Pressing space (that's what this patch is about) should select as well,
but not move the focus.
The best way to test the behavior is with a multi-part template.
Bug: T285323
Bug: T289043
Change-Id: I97d77f43b231696f92ba6758a6b8feac34e02e6d
I tried to review all of them. Some of the changes I did:
* Make sure the `config` parameter is not marked as optional
when it is not.
* Make sure default values are mentioned.
* List individual `@cfg` options when it makes sense.
Note I don't list all options a class could accept (e.g. via all
its parent classes and mixins). That's too much. Instead I checked
how a class is actually used and list only these options.
Even then I don't list everything, e.g. unspecific options
like "classes" that can be used pretty much everywhere.
Change-Id: Idf4fbe1dc3608ace277df9e385f2f140df3a2f50
We don't need to distinguish between these any more. Both are
"active", i.e. both focus the widget on the right side of the
dialog. Sometimes the "choose" event is fired to actually add
or remove a parameter. Sometimes it's fired, but the state of
the parameter doesn't change (for whatever reason, i.e.
because the parameter name was clicked instead of the
checkbox). There is nothing to do in this case, except for the
focus change.
Change-Id: I3c7c0c81a075ccff76eda0a4fb2aa1ac7be3cec5
* The template model fires an "add" event. Listeners don't
automatically steal the focus any more.
* Instead there is a separate "focusTemplateParameterById" event
fired from all relevant places that add a parameter.
* The "remove" doesn't steal the focus any more.
Bug: T285323
Change-Id: I93f17727524bfbcf6f11647a6c2441781337c4cc
The 'classes' property is a OOUI interface. Personally, I like
this code style better.
However. It appears like the code style in this codebase is
somewhat mixed. It looks like the top-level .$element always
uses .addClass(), while other code uses the 'classes' property.
Should we unify this?
Change-Id: I9ecd75e22d00f06ffd707f766dc9e8d748ff9a37
In JavaScript .split() behaves different, compared to PHP. In
PHP the last element contains the rest of the string. In
JavaScript the rest of the string is discarded. The limit acts
as if the array is truncated. That's why we can reduce the
number in
'foo/bar'.split( '/', 1 )[ 0 ]
to 1, as we are only interested in the element "foo".
The same code in the other class is currently not covered by a
test. But changing it accordingly should be obviously fine now.
Change-Id: I20c27d480ddb1799df9eb1e5bc119b724e80653d
When I try to click the input field, the expand/collapse button
is focused instead.
This also fixes a copy paste mistake in this class.
Change-Id: If9ab340711fbe7d88845c008360fde5df7059df0
Before, the content pane (the right half of the dialog) was moved
to the right, outside of the visible viewport. But it was still
active and could i.e. be navigated to via the tab key. Only truly
hiding it solves this issue.
Bug: T274554
Change-Id: I8925a9cca0099528aca8e98452816b5f9dd23a76
The original idea was to make the interface as narrow as
possible. However, it turns out it's better to model the
"templateParameterClick" event more closely after the "choose"
event.
This is split off to make reviewing the following patches
easier.
Change-Id: I271f576c6cd756cecfc6cb1fd64810f8da5c3575
Pure cleanup, doesn't change behavior.
Change If8da5ae85dff63c34 included in OOUI v0.42.0 tracks invisible
controls, so it's no longer necessary to maintain persistent class
variables pointing to the buttons.
Also simplify repeated logic to make it clearly exclusive.
Bug: T290554
Change-Id: If9b6404d7061999540515645fa8e50b9a21f5a21
The list of parameters should remove itself from the list of
possible tab navigation targets when it's empty.
Note there is no way to remove elements from the parameter
list. That's why we don't care about "remove" or "clear"
actions.
Change-Id: I8b1215117e0ddc94f787d173e9bea6f7567d9671
Previously, when the tranclusion dialog was being resized, there was
a specific width at which both the sidebar was collapsed and the dialog
was downsized from "larger/large" to "medium". This resulted in the
dialog switching from fullscreen to floating, since the dialog's width
breakpoint for "medium" doesn't match our width breakpoint for mobile.
If the user continued to downsize past this breakpoint, then the dialog
would eventually switch back to fullscreen resulting in this weird
behavior.
A simple way to prevent this is to avoid changing the dialog's size
at all. Since we don't support collapsing the new sidebar (unless
already in fullscreen), we can just leave the dialog at the
"larger/large" size.
Bug: T274554
Change-Id: I5460cdfb1a7ed73fe7957745ba37055c5f66dce1
This is split from patch Iebb982e to make it easier to review.
The name is rather ambiguous. Does "input" refer to the input
element? Is it triggered for every key press, i.e. when the
input changes? Or when it's submitted?
Change-Id: Iddbe3bfb9faf3561d8d71b96ffae507799827a95
Any of these characters results in bad wikitext, when we accept
it in a template parameter name.
Instead of displaying an error message we simply block the
button, as long as the input is not a valid parameter name.
Coming up with a message is not really worth it, I would
argue. Users typically don't have a reason to use any of these
characters. This is super rare. And even if, the behavior of
the widget is not hard to understand, I believe.
The same is done in ve.ui.MWParameterSearchWidget, a little
hidden in the .addResults() method.
Not yet approved by UX. Can be done in demo time.
Bug: T285869
Change-Id: I5576cdfb90411e5fdec93749f72939d31ecd9c56
E.g. avoid calling the rather expensive method multiple times
in a row, if only 1 of the results is needed.
Change-Id: Iff1d2c0892367e927303f6f45d3231e04c045cab
* Use a more specific …-top property, as this is the only thing
we need to overwrite.
* Bring some selectors in a hierarchical order that makes more
sense.
Change-Id: If36db87d83f699fe0a43ac67d439cac42cbb1fa3
* New help text for the case where TemplateData is present, whether
or not it includes a description.
* Remove help text when TemplateData is missing.
Bug: T288465
Change-Id: I0668ccae8eeb5ffffc626e3b7d24c1d7ed99bbed
I tried hard to come up with the best possible names. Some of the
criteria I used:
* Longer and more unique is better. This makes it much easier to
e.g. search for the event name.
* The term "part" should only be used for top-level parts. While
template parameters have a unique id, they are not a subclass
of …TransclusionPartModel and therefor not "parts".
* BookletLayout manages "pages" via "page names".
* The page names of top-level parts are identical with the part
id, see ve.ui.MWTemplateDialog.getPageFromPart.
* The page names of parameters are identical with the parameter
model id, see ve.ui.MWTemplateDialog.onAddParameter.
Some code knows parameter ids, but not what pages are. Other code
knows page names, but not what parameters are. The transition
currently happens in the …OutlineContainerWidget. We might want
to move this point up to the …TemplateDialog. But I would argue
this is good enough for now and can be changed later, if needed.
Bug: T285323
Change-Id: Iab2805b3203988db400b67c8d00e48905fdc53dc
I tried to memorize the cursor position in the list of
parameters. This way you could leave the list with tab, and
return to the original position with shift+tab. Unfortunately
this is not how the SelectWidget works. The moment the
highlight is gone it's gone. There is nothing that remembers
a position. We could introduce code to do this. But I feel
like this is a lot of effort for not much benefit. Also not
listed as a requirement anywhere, at the moment.
Bug: T285323
Change-Id: I8d44ba4539ec4b5535bc031accfeacd87e1886eb
This got lost in patch I20dbd2b.
Both events come from the same sidebar class. The difference
between the two is:
* selectPart is when the button representing a top-level part
is clicked.
* focusPart is when a parameter name is clicked while the
parameter is already checked.
Yes, this is confusing at the moment. Following patches will
rename, merge and split a lot of these events to be much more
self-explaining.
Bug: T285323
Change-Id: I0c6b53c93c712ff5e47c1beb5199d590cba7ab1a
We forgot to remove this in I319896a. The individual
…TransclusionOutlineParameterWidgets don't fire this event any
more. Instead this is done by the …SelectWidget.
Bug: T285323
Change-Id: I2c29e45127464785ffdc32d73b52188fcbefb7bf
Note there are currently two different code paths utilizing two
different events. The existing event handler actually changes
the selection of the top-level part in the sidebar (the
corresponding template name turns blue). The new event handler
highlights a parameter (it turns gray). This is currently
intentional (partly because of a bug in OOUI). I will try to
merge these code paths, if possible.
Please test, and if it works fine from the user's perspective,
please merge it as it is for now.
Bug: T285323
Bug: T289043
Change-Id: I8fafee68b8b7ff225c7b3c327f483f3426d8129c
Proof of concept: while flex is – well – flexible, it feels like
this should be possible with some good old block containers and
margins. It's pixel-perfect in my test.
Bug: T288465
Change-Id: I1458900fff197e08ce318398524a3cf2b6b9ee2a
- Change description text according to ticket
- Make sure link to template page opens in new tab
- Add missing placeholder text
Bug: T272487
Change-Id: Ie8189e9cb9db5908e8fc5fc8bf7ff20df5595094
This fixes a few style issues:
* The buttons that represent top-level elements have a proper
2px focus rectangle again. Back to the OOUI default.
* The list of parameters does have a 1px focus rectangle all
around. Intentionally thin because there is a 2nd level of
keyboard navigation (via cursor keys) in this element.
* All these focus rectangles look the same in Firefox. Before,
it was a thin dotted line on the parameter list.
* Parameters with long names don't wrap on a 2nd line any more.
I believe this was working before but got lost in I92e8fd2.
Bug: T285323
Change-Id: I0229b6395a64a9903335bf96349af70fb20ad047
When I press the button to expand the input field for
undocumented parameters, it needs to be focused. Otherwise I
have to click it manually all the time.
We probably forgot to list this as an acceptance criteria when
working on Ic5dcd36.
This also replaced a bit of JavaScript with CSS. I do this
mainly because I found the mixture before (one piece was
hidden via JavaScript, another via CSS) a bit confusing.
Bug: T272487
Change-Id: I0cbee63c65a37f2f1860bde007c1e5c8408ba006
This makes sure the corresponding top-leve part is selected in
the list on the left when navigating the main area on the
right.
Bug: T289043
Change-Id: Id1b398e1786c4099d5b14fe88dd21a106269096b
This comes with a few significant changes:
* A whole bunch of places in the code that focus and highlight
an element in the old sidebar consider the new sidebar now.
* Same when e.g. the toolbar at the bottom needs to know which
part is selected. This is read from the new sidebar now.
* To make this possible I had to merge the small helper class
we introduced in I7bc73cc back into the dialog.
It's helpful to understand how the event flow works:
* You click a template name. This does nothing (does not select
the element). It only triggers an event.
* The event is catched by the outer container that manages
all parts. From there all elements are unselected, and one
selected. This call is internal and should not trigger
another event.
Bug: T285323
Bug: T288827
Bug: T289043
Change-Id: I4a2d2b83cf2691423d4b0e6f4487228fa3c7b56d
This is mostly, if not exclusively visual, at the moment. The
actual state is still managed by the old sidebar.
I made the element OptionWidgets for convenience. This gives us
all the functionality we need (primarily setSelected and
isSelected), without to much clutter. However, I didn't made
the container a SelectWidget. This comes with to much stuff we
don't need at this level, e.g. cursor key navigation.
Bug: T285323
Bug: T289043
Change-Id: I20dbd2ba23ceaa9125947b25e037c0bb3c91a471
Most notably:
* Move some code snippets from the outer …TemplateWidget to
the inner …SelectWidget, without introducing new
dependencies.
* Move all knowledge about the item class
…OutlineParameterWidget class into …SelectWidget.
* Some more self-documenting method names for event handlers.
* Avoid the somewhat ambiguous variable name "checkbox" in
favor of "item". That's how it's named in the upstream OOUI
…SelectWidget.
This is extracted from the following patch Ibd94c39. The
difference is that the following patch adds a new dependency:
The …SelectWidget gets to know the template model. This patch
here contains all changes that are possible without this new
dependency.
Bug: T288827
Change-Id: I187f313c84424b28005d9276cb1356029f9ebb75
There are 2 situations:
1. Either the template name is used in a [[…]] link. In this case
we must provide the namespace. MWTemplateModel.getTitle() does
this. However, it's not a mw.Title object and therefor not really
guaranteed to be a valid title. This is fine. The worst thing
that can happen is that the link points to an error message.
But this should be entirely unreachable anyway.
2. Some messages want to display the name of the template.
Ideally without the namespace. That's what
MWTemplateSpecModel.getLabel() is for. Again this is not
guaranteed to be a valid mw.Title. But it doesn't need to. It's
only used as a label.
Change-Id: I03d0481201620a2f5c444ee32b656bcaade98aac
We should only need that label for the link. The other mechanic
would fail when editing wikitext like this:
{{{{echo|<}}|param=foo}}
Bug: T272487
Change-Id: If8d228b40bf1589181e83e8f68f3c33b4c7759c7
This also fixes a mistake in the class where we forgot to
disconnect event handlers when an element is removed from
the list. This doesn't have much of a consequence, as the
event flow is only in one direction, from the destroyed
element up. This is not possible any more.
Bug: T289560
Change-Id: I0bcc1d68c50b8cbdb033ef6692b34e2fc94e8d85
This replaces I8cf9ecd.
Significant changes:
* The …OutlineContainerWidget doesn't need to know the
BookletLayout any more. The only remaining resason to have
this dependency was some focus management. This is now done
via an event.
* Renamed an existing event to match the new one. The two
really mean and do the exact same, even if they are
triggered from two different places.
* Simplified some existing code.
* Updated documentation.
Bug: T288827
Change-Id: Ifcf2cadabf7fa4b8ecb72e3937003fab3b00d9bb
Names like "fetch" or "resolve" are heavily ambiguous and
continue to confuse me. I hope these new names reflect better
what's going on.
Bug: T288827
Depends-On: I587a203a9370e4742f87586b4f1867b37459c375
Change-Id: I8fa47ed313e7d7b2c114a5638a67c4f3c8b830f1
This merges all code-paths that re-select a part (i.e. an input
widget on the right side of the template dialog).
Note there is an edge-case that actually changes with this
patch. When a page is removed, and creating a new page fails,
there is an `if ( page )` check. Before, the behavior was that
nothing gets selected in this case. After this change the
behavior is the same as if a page was removed: the closest one
gets selected. Not only does this make more sense. The `if` is
only a fail-safe anyway and should not result in different
behavior.
Bug: T288827
Change-Id: Ibb0260587588fb51a876658b16a81c5a73371dc4
What this changes:
* The moment the user selects anything in the parameter search
widget, the input is cleared, no matter what happens next.
Even in case of an error. We know the input was bad in this
case. Let's get rid of it.
* The method makes sure it does not even try to add a
duplicate parameter. This should be unreachable, but better
be safe than sorry.
This is split from I5eeb973. I run into this while playing
around with different approaches related to hiding deprecated
parameters. Typically there should be no way the parameter
search widget offers a duplicate. Still I believe it's a good
idea to have this extra safety-net.
Bug: T272487
Bug: T288827
Change-Id: I04e76d73b4a3f6467d0ccf3ccff5d2f6b4114bd9
* getPartId() is unused.
* Use this.data instead of a custom this.partId.
* No need to store this.header as a property.
* Rename the event to "headerClick". That's enough when the
event comes from a widget that does have the word "part" in
it's name.
Bug: T274544
Bug: T288827
Change-Id: I8c70425403c6cd6a19e3a1cacb2b085e5c8b2e46
The base widgets we are going to use (notably OO.ui.OptionWidget
and OO.ui.SelectWidget) also have events, and some of them use
the same names. Such conflicts are really hard to track down.
This is meant to be temporary. The goal is to use the events
from the base classes and get rid of the custom ones, if
possible.
Bug: T285323
Change-Id: I0f103a5bbb8fb800e57009e3bf709f00a651fdda
The problem here is that the OO.ui.OptionWidget base class
(changed in the previous patch) also contains a .setSelected()
method, but with slightly different behavior. This results
in crazy behavior when I try to make the outer widget an
OO.ui.SelectWidget.
Renaming the method to be a custom, private helper method
avoids this problem.
The plan is to actually use the default setSelected()
behavior and get rid of the helper method. This will be done
in later patches.
Bug: T285323
Change-Id: I84e752f20a4d07007fd4e61989f9b34983410950
