Note this patch is somewhat incomplete. The feature fully works and
I would like to see this patch merged as it is. But whenever you
press one of the keys the focus is stolen by some element on the
right side of the dialog. This makes it impossible to e.g. press
Ctrl+Shift+Down multiple times. The idea is to work on this in the
next patch.
Bug: T290262
Change-Id: Ic67f2a696c94f1e5c71134d681161221aecbfdf6
Reasserts scroll and highlighting when toggling the sidebar, in the
case that the other panel is hidden (narrow-view mode).
FIXME: Doesn't reassert focus because the page.focus() and
bookletLayout.focus() methods don't seem to work, maybe the bug is
especially prominent when the item to focus was already marked
active.
FIXME: Stopped working for right-to-left sync on wikitext elements.
This is a less common use case and can be addressed in follow-up.
Bug: T290975
Change-Id: I94f5709e810c63ee5fd7729a192ac7b92686b88f
… obviously only to methods that are meant to be private, i.e.
only called from within the class (and possibly tests).
Change-Id: I581558078dc7210abac5f5724f71316ac45745e6
The .onUpdateOutlineControlButtons() method doesn't describe what it
actually does. This issue was introduced in I9c5478a. (Intentionally,
to not make the patch to complicated.) Let's continue to rename
things to be a) unique and b) honest about what they do.
This is an alternative to I8d98e61.
Bug: T289043
Change-Id: I4d52ffa6e9e5df2025a0c33031c1517bcb421279
I can't really tell what insight we get from the word "container".
Every widget is a "container" in some sense, isn't it?
This widget is just _the_ outline, I would argue.
Other suggestions?
Change-Id: I1fb27ee58c1a3dd790022504e978198dadf7ea02
This prevents multiple highlighting, which was possible to achieve by clicking
sequentially in the input fields of two parameters from different templates.
Change-Id: I404936f1569ab544b693a9bc6921381636ea8f40
This is bad for multi-template transclusions, where we focus the
first parameter of each template ending with the last. It's also
inconsistent, we don't do the same for wikitext chunks.
Change-Id: I720ce1a380a6f4a8618c3608b63557df5fb50393
This was damaging the UX by causing the first parameter to be marked
as selected, but without reliably focusing it. For example, loading
a wikitext-template-wikitext multi-part transclusion would cause the
initial focus be given to the documentation link in the template
content header. After this patch, the focus will be at the top of
the page and tab will run down window functions and then through the
sidebar.
Change-Id: I84131870ae3887dcae74d91d68c5984d1dbffd85
The weird auto-scroll feature is described in T289043#7297679.
This also fixes T291381 different than I393a2b1. Only one of the
patches should be merged.
Bug: T289043
Bug: T291381
Change-Id: I70d87f12fd68001e880510fb6c38d7c419d64b15
AddPart in the resetDialog also triggers replace part
so this call could be removed there.
Bug: T291365
Change-Id: Id6c0f5bf3aaece45da37ffab75a4d99c113944f6
This is mostly re-arranging existing code. Actual changes made:
* Remove the message that claims a template can't exist. We can't
really know this.
* Instead show the message about "modifiers" in cases where curly
braces and other wikitext syntax is involved.
Bug: T290140
Change-Id: I713d7f54cad2510f9a02c113600980cba8c3e58b
The titles in the link cache do not include subst: anymore so to see
if these pages exist we need to use the same link title used in the
query.
Bug: T290140
Change-Id: I18de81e0bf46212c2199a948f7ca89182aa19eff
This code was introduced in I8fafee6. I can't tell any more what
the "bug" mentioned in the commit message was. Let's get rid of the
duplicate code path, see if we run into regressions and deal with
them one by one. That's much easier to handle than keeping this
confusing code path around.
Note this "focusin" event handler was actually re-implementing
parts of the upstream BookletLayout, namely
OO.ui.BookletLayout.onStackLayoutFocus().
Bug: T289043
Bug: T291381
Change-Id: Ib386ae6efec08465122f0e8ee81cd6dc9a2d337a
Note there is still an issue with the upstream
OO.ui.BookletLayout.selectFirstSelectablePage() method stealing
the focus in some situations when you press space. Still this patch
already improves the situation. Pressing space on both top-level
template elements as well as parameters should scroll the thing into
view, but keep the focus in the sidebar. This was just not happening
at all.
Make sure to use a very long multi-part template to test this.
Bug: T289043
Change-Id: I9c5478a04b14b94ccd5d00480d48a7d59b4e0c37
We broke something with the change I166b971. When we renamed that
method it started to override (and therefor disable) the method
with the same name from the base class.
I decided to move all code in the subclass for the moment. It might
be misplaced there (note how almost all code related to the new
sidebar is in the base class). But this is cleanup work for later.
Bug: T289043
Bug: T291151
Change-Id: Id255585e78967eee0f72c27727cd23211674923c
The most basic fix. The "outlined" flag is set to false for the
cite dialog, in contrast to the transclusion dialog. It's always
true for the transclusion dialog. Note it doesn't mean the sidebar
is visible, but specifies if a sidebar is created in the first
place.
Bug: T291241
Change-Id: I5a8b538949e9fd0b8e85a6a91ca2420ef72e4612
We removed this line of code in a recent patch, but it turns out
it's still necessary in at least one situation:
* Make sure you have a multi-part template where the first part is
a wikitext snippet.
* Edit the template.
* Click the very first item in the sidebar.
Nothing happens. But the text cursor should be in the wikitext
field.
Another situation:
* Put the text cursor in the first wikitext field.
* Press shift + tab. Now a button in the bottom toolbar should have
the focus.
* Click the 1st element in the sidebar. Again, nothing happens.
The extra .focus() call is redundant in many situations. But it also
doesn't hurt to repeat it. It will just re-focus the element that's
already focused.
Bug: T289043
Change-Id: Iccbe376b98a1b1e5469cd17e1c95d5d8869442d3
It was called like this because the event is literally called "set".
But it doesn't explain _what_ is set.
The new name .onBookletLayoutSetPage() already appears somewhere
else in the codebase.
Change-Id: I166b971c08f5d0fae97fc9d6244117a680f84b7c
The previous patch Id314ee8 was incomplete. The event changed.
The id in the event is not guaranteed to be a top-level partId any
more, but can be a template parameter's id.
Note: "Parameter id" and "pageName" is the same. The fact that
these ids match is how the left and the right side of the dialog
communicate.
Bug: T289043
Bug: T291151
Change-Id: I391f0f8edb96398fd33a2e0b01003013c52776da
I realised these are vital information to make the buttons at
the bottom of the template dialog behave sane. It's still
possible to focus this page, even if it doesn't have a visible
item in any of the old/new sidebars. This is when these flags
are used to decide if the up/down/remove buttons should be
enabled.
Bug: T291151
Change-Id: I6ab709b856d110bfb37daa1592c0b6a99714aa25
The OO.ui.OutlineOptionWidget class does have an .isRemovable()
state. This is how OO.ui.OutlineControlsWidget decides if the
remove button can be used.
It appears like ve.ui.MWParameterPage forgot to mark required
parameters as not removable.
This makes some oddly specific code in ve.ui.MWTransclusionDialog
obsolete. Note how this class does not contain any other code
about "required" or other flags specific to parameters.
Note that one aspect of this patch will be visible in the old
sidebar: The trashcan will be visible on required parameters, but
disabled. It was hidden before. However, this actually improves
the UX. Hiding the trashcan made the up/down buttons jump around.
This makes it unnecessary hard to hit them. It also causes visual
distraction when navigating the list of parameters.
Let's stick to the upstream OOUI behavior.
The remove button still disappears when the only element in the
dialog is the template search widget. This is clearly an
entirely different state. (Don't ask why the up/down buttons are
not hidden. It was like this before.)
Bug: T289039
Change-Id: If78881e503f19f497f1993da4e5b9b09ee538307
The separate setup method was introduced in 2014 via I7c3c133.
It appears like most of the code here was written before this
method existed. Let's update it.
* this.outlineItem is guaranteed to be set. No need for the `if`.
* The parent method is effectively abstract. There is no point in
calling it, I would argue.
* The return value is never used. I.e. this method is never
chained, and probably shouldn't.
Change-Id: Ida26ebdf09be74958936c3950ebdf6def9a69bc0
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
As preparation to introduce then new UI to add unknown parameters.
This is a few things:
- Merge the code paths when adding a MWTemplateModel
- Put code adding parameters to the dialog next to each other so
that preventing reselection happens around that block
- Reduce duplicated code when re-focusing after addition
- Move adding the placeholder page to the end
- Add and clean up inline documentation
Bug: T272487
Change-Id: Ic700edd42027a928a236ed11f2c257fffe994257
This removes the paramter placeholder page from all places where it's
not usefull anymore under the new sidebar.
The new UI will be re-added in follow up patches.
Bug: T272487
Change-Id: Ifc6f6f64fed1a1b23c92282e2a1bb40a7d401d72
The plan is to change the outer …TemplateWidget (which contains
a list of template parameter checkboxes) into a SelectWidget.
But this requires the elements in the list to be a subclass of
OptionWidget.
Note this change does not have any effect, as of this patch. But
this makes the following patches smaller and easier to follow.
Additionally:
* The OptionWidget class is already a LabelElement. No need to
initialize this twice. This happens via the parent constructor
now.
* Remove CSS that is not needed any more after Idc5e048. This is
not a FieldLayout any more.
* Update some related code documentation.
Bug: T285323
Change-Id: I92e8fd2bbece9e6c55083cdfe6ed7ad16a64d688
The icons have a padding of 6px around the icon image itself. To
get to the required 16px/8px space the margin was adjusted
accordingly. Note that there's also a 2px padding around the menu.
Bug: T272482
Change-Id: I3df9f355dfd5c4e6366432555b96bf788e784280
This is what actually happens:
* We call `addParameter()`.
* This triggers an `add` event.
* This calls an `MWTemplateDialog.onAddParameter` event handler.
* This code doesn't check if a parameter already exists (because
it shouldn't). It detroys the page in the content pane on the
right and recreates it from scratch.
The only reason we do this is to focus the input field on the
right. This patch introduces a dedicated event to do this.
Bug: T288827
Change-Id: I47effe05427cfabfcf534920edee79521eaa033f
I found this while working on T274551, which is all about the
definition of "empty".
In the old sidebar a parameter's name is dimmed (gray) as long as
the parameter's value is empty. This stops working entirely when
there is a default value.
My first impulse was "this is a bug". When there is a default
value, both the empty string and the default value (when the user
enters it exactly) typically trigger the same behavior: The
template uses the default value, just as if the user entered it.
But this code is correct because of the way it is used. Only
parameters that are "truly" empty should be visually marked as
such. The moment there is a default value it is either impossible
to change this back to an empty string – meaning the parameter
can never be truly empty – or the empty string is meaningful user
input.
Bug: T274551
Change-Id: I90657bfe83e56afd3942428c0dd8a47b444e39c9
This should not have any effect on how the thing looks and
behaves.
* All elements in the sidebar should be reachable with the tab
key, including disabled elements.
* Enter jumps to the corresponding paremeter in the content
area on the right. But enter doesn't change the checkbox
state.
* Space canges the checkbox.
The class will be renamed in the next patch.
Bug: T285323
Change-Id: Idc5e04828ece0ba77a65e4c839cd3ffccc3b6733
As discussed in Ia44da16. This change avoids possibly hundreds
of events (when a template does have hundreds of parameters),
and replaces them with a single one.
Bug: T288202
Change-Id: Ic819e8c93e872b653c238f396f1f327b6a8759d2
There is still a lot to do, but this implements some basic
behavior that was missing before.
* You can now use the tab key to navigate all checkboxes,
including disabled ones (required parameters).
* Enter key now works on both the checkbox as well as when
the entire line is highlighted. Enter forces the checkbox
to be checked and moves the focus to the content area.
* Mouse clicks now work on the entire line. Before, only the
text label was clickable.
Open issues (not to be resolved in this patch):
* Clicking the text label and the empty space after the text
label does different things. Probably shouldn't.
* Should a click on the label check the checkbox?
* Space key should probably not move the focus to the content
area.
* Focus rectangle is different on disabled rows. Is this ok?
* Background color when a line is focussed is missing.
Change-Id: I22ccd1bea92e4f098d4b25a9e38cddde5c103423
The checkbox is the first parameter in the parent constructor.
The parent is the FieldLayout class. The checkbox becomes the
this.fieldWidget in the parent class. Just use this instead of
storing a duplicate reference.
Bug: T274543
Change-Id: I4ae7d467334f88f2be93a62660145a025089401f
I get a scrollbar at the bottom of the sidebar. The reason is
that the container's width is 100% + 1px. The extra pixel is
from the border, which is not needed in this mode.
Bug: T274554
Change-Id: I4f749be6b9a7f89f9a7a195dc66c5c18253b1327
This is really only about the methods name, but doesn't change
any behavior.
I realized we work with several different definitions of what
"empty" means. There are at least two significant definitions:
1. When a parameter's value is the empty string or identical
to the default value, the behavior of the template is the same.
It will use the default value just as if the user entered it.
The auto-value is a meaningful value in this scenario and can't
be considered equal to the empty string.
2. The context here is when the user presses the back button.
This will destroy all user input. But an auto-value is not user
input. It will appear again when the user realizes they made a
mistake. Nothing is lost.
Personally, I would not use the word "empty" to describe this
concept. Things like "containsUserProvidedValue",
"isCustomValue", "isMeaningfulValue", … come to mind. These are
all still a big vague. A "user provided" value can be identical
to the default or auto-value. "Custom" how? I went for
"containsValuableData" for now.
Bug: T274551
Change-Id: I2912a35556795c867a6b2396cbad291e947f0ed6
This is a direct follow-up to I6ebd020.
Steps to reproduce the bug:
* Make sure you have a template with a deprecated parameter.
The position doesn't matter.
* Add the template. The deprecated parameter is hidden.
* Add an undocumented parameter, e.g. "b". This is added to the
end, as it should.
* Add an undocumented parameter "a". This should appear before
"b", but doesn't. The reason is because the invisible
deprecated parameter is in the list that is used to calculate
the index, shifting it by 1 (or more when there are more
hidden parameters).
This patch includes a few closely related changes:
* We can loop the list of checkboxes directly instead of
indirectly via the list of parameter names.
* I made it so that an active filter only resets if it would
hide the new parameter. The original problem we had to solve
was that the new parameter would always be visible, even if
it doesn't match the filter. This awkward mismatch is still
guaranteed to not happen.
Bug: T274551
Change-Id: I1b0480ae836cc19b77b159d3fb30ff32e8c59df4
I came up with a new event to do this. This event is triggered
individually for each parameter. An alternative is a single
event that gets a list of visible parameters. Is this better?
What do you think?
Bug: T288202
Change-Id: Ia44da16917c28171a01aef0f1c613dcd5d3266ba
This is – for now – intentionally done in a way that can be
undone. This will still be helpful for debugging for a while.
But we need to get rid of the duplication to be able to make
this new functionality visible on the beta cluster.
Actual removal will hapen the moment we actually remove the
old toolbar. There are already tickets for this.
Bug: T286765
Change-Id: I842c3c39a55a273af20643fa8a602d2e57fb6b8c
This affects only the new sidebar. Deprecated parameters don't
get a checkbox, except they are used already. "Used" includes
parameters that are present, but empty.
Bug: T274551
Change-Id: I6ebd020d02650c19060345d13495373acab363df
Disambiguation pages are rarely the page users intend to link to,
especially with newbies. By moving the disambig page(s) as the last
result, the user is more likely to pick the page they actually intended
to link to.
Bug: T285510
Depends-On: I2b8545f6dd4849629037f81f48a540748e60da83
Change-Id: Id55a19e7665d8f88559c471de36e5447fb2babb0
With this users can also filter undocumented parameters
that they added to an empty starting template.
Bug: T272481
Change-Id: I99adb38b0ae4d4ade91fcb506f10c0222b9bb5e8
Note that this patch alone probably does not make that much
sense. The code executed is pretty much the same. The only
difference is that the empty (!) …ContainerWidget is kept
and re-filled with what might be a completely different
template.
This is not much of a difference to before when the
container was recreated.
This change will make more sense when the container has to
manage more state, e.g. focus states. This state will
survive then.
Change-Id: Ic336d10a595e3e222741a3dc57c1d54639166b7a
This ellipsis was there before we started working on this code,
but was never working properly.
We understand that the CSS was intentionally done like this (as
the comment explains). However:
* We changed the width of the dialog. The old value doesn't
match any more.
* The width is different when the sidebar is expanded vs. when
it is collapsed. Even if we update the number, it won't
always work.
* The 100% work fine in current browsers. I can only assume
this was different back in 2014 when this CSS was written
(see Ia8259e9).
Bug: T285044
Change-Id: I3de2b0ed0b6a05d2b9fa0b325a2b12277564b271
Notably:
* Don't require the model in the new sidebar via dependency
injection, but connect the event handlers later. This is
relevant because we currently create the new sidebar in the
wrong spot. Removing the hard dependency allows us to split
the code and utilize initialize() and getSetupProcess()
correctly. This will be done in a following patch.
* The change event now includes the new position. This makes
it very easy to add this missing feature to the new sidebar.
Also:
* Stop triggering change events when nothing changed. These
events are expensive. They bubble all the way up to the
TransclusionModel, and to all linked
onTransclusionModelChange() handlers.
* Update event documentation to make this more visible.
Bug: T274544
Change-Id: Iafe29f18a6fed14d9c3124c9756aa840886afbbc
Notably:
* Include parameter aliases, labels and descriptions in the
search.
* Don't use a possibly outdated search index, but live data.
* Clear filter when a new checkbox is added.
Bug: T272481
Change-Id: Ie90a803af6178a8bb6de370a0f8e079800d9f8a2
In detail:
* Allow clicks on all elements in the new sidebar. This should
focus the corresponding element on the right.
* Make all elements in the new sidebar tabbable.
* Fix MWTransclusionOutlineTemplateWidget.createCheckbox() to
not need a temporary param object any more.
* Rewrite more code in MWTransclusionOutlineTemplateWidget to
be shorter and easier to read.
* Fix MWTemplateModel.addParameter() to not do way to much
stuff when a parameter already exists.
* Update code documentation.
* Use more specific, less ambiguous variable and method names.
Bug: T274544
Change-Id: Iaf6f7d1b0f7bf0e9b03eb86d01f3eceadece6fe4
Clicks on the left side now focus elements on the right
side.
This patch also simplifies the …ContainerWidget constructor.
The config parameter should only be used for "OOUI things"
that are needed by subclasses and mixins. But the parameters
we have here are not "UI things".
Passing them as config passes them to classes where we don't
know what they do with it. What probably happens is that
some class keeps a reference to the entire config object,
which doesn't have a benefit and possibly blocks garbage
collection.
Bug: T274544
Change-Id: I0c0e4a1ba59dcb43141338ffe939c9c6783e000d
Actually reusing this OOUI mixin gives us a lot of well
developed functionality we need anyway. Most notably proper
event management, e.g. click events.
The number of CSS properties we need to override is managable,
I would argue. Let's see:
* Our buttons are not inline-elements, but should use the full
width.
* No focus-border left and right for the same reason.
* We want much more inner padding.
* We want a stronger hover effect.
* We need to fine-tune the position of the icon. This is
because of the inner padding.
* Need to get rid of a negative margin that's only relevant
for inline-buttons.
I currently feel like the benefits are worth living with
slightly more brittle code. Note that we can undo this change
any time because all this is well encapsulated in this new
class.
Bug: T274544
Change-Id: I33f275a958964d49e803e56bf74a6fa961093da1
This introduces another generic "button-like" class that can
be reused in multiple places in the new sidebar. The main
change in this patch is the "add more information" button
which is now an instance of this new class as well.
This patch also simplifies over-complicated setup code in
related widgets.
Bug: T274544
Change-Id: I0cfe7675d02fdd5c5dc8d9198bb3f4aec9abf397
Before, the new sidebar was hacked in a place where it confused
the BookletLayout logic. This became visible when using the
up/down buttons to move elements in the sidebar.
This new container wraps the new and the old sidebar. It also
uses a temporary color to make it easier to see where one ends
and the other starts.
Bug: T274544
Change-Id: I4e5b40b1d1556886fc85cff9e926a02e4888f032
The two new widgets are pretty trivial now, thanks to the base
class.
Note there is still no code to delete the widgets. That's also
why you will always see a placeholder widget at the top. This
will be fixed with the next patches.
This patch also renames most of the "…TemplateOutline…" classes
to "…TransclusionOutline…" The reason is that these widgets are
not for a single template, but part of the container widget for
a more complex transclusion (i.e. a sequence of multiple
templates and wikitext snippets).
Bug: T274544
Change-Id: If4219b0b8ad4d1969ab1ec5ec4db0728811bab35
The icon and the name of the template are now created by the
base class. This is meant to be reused for other elements
that are not templates.
Bug: T274544
Change-Id: I76bbc0e8c0420e9c6357d093d5f5e1651a0c2719
Reasoning:
* format=json must be the default. Nothing else makes sense in
the context of this code. This should not be a surprise.
* formatversion=2 is only a default when the custom
getContentApi() is used, but not when mw.Api is used. One
might argue that it's safer to always specify formatversion=2.
However, this is not done in other places in this codebase.
It should never be done or always.
* I find it confusing when the action=… is missing. Let's not
rely on this default.
Change-Id: I6ca29f76bffc0849103c5bcff4aaf28fcaaa4c52
This code doesn't do anything but adding an empty <div> to
something that is already a <div>. It doesn't even have a
class name, i.e. it's not referenced from anywhere. We can
add such containers back any time when it turns out we
actually need them.
Bug: T274544
Change-Id: I62546cc7939364db31f37b9de0c035974554544b
This base class will be used to style the 3 types of top-level
items in the sidebar the same way, without the need to
duplicate code or styles.
Bug: T274544
Change-Id: I1a62ff610728d7150dea1717316ef20f6882783a
This matches the existing naming scheme better. I also plan to
re-use this class for other types that are not templates.
That's why the name is the more generic "transclusion" now.
This patch also removes a `padding: 2px` that's not that
helpful. We will need paddings later, but need to choose them
much more carfully.
Bug: T274544
Change-Id: I6f0f630da2230b023b3fb065e5ad86d8211bb7b3
Because the API uses a generator, the search results are ordered
alphabetically. The actual search result ranking is in a .index
field. This code accidentially deleted the alphabetically lowest
template instead of the least relevant one.
Change-Id: I79de024feb569e9f06bedab908a6509a4d4fa99b
We do this additional prefixsearch anyway. What we did before
was ignoring the result when it was not a 100% exact match.
Instead we can always add this 1 prefixsearch result when it
was not already part of the CirrusSearch result set.
This won't happen often. Usually the 1st prefixsearch result
was already part of the CirrusSearch result set anyway. But
if it wasn't, that's a serious issue for expert users that
expect the search to behave similar to the suggester at the
top of the MediaWiki interface (which is also a prefixsearch).
Change-Id: I959d2b058a3d64596a8cfbe5476ab351e40f8760
Some details:
* The config is not optional in these cases.
* This patch continues to remove some comments that don't add
any information but just repeat what the code already says.
Change-Id: I5c27cd01ad80709bb583256821d65c6b65b74b05
For example, checking if a parameter is required works just fine
for unknown parameters. They are never required. Since I16708b0
we don't need to guard the spec related methods any more.
Change-Id: Id90e4cb810dc9faca3b26f122a534f276ee31709
It's good practice to make transparent elements transparent
for mouse clicks as well, i.e. make it possible to select text
behind the fade effect.
Bug: T283943
Bug: T286235
Change-Id: Ib5022a74c70e4b7cb5e2a0faad20bd9abcc0da36
Introduced in 2 separate patches by the same author. This
patch removes the line that was introduced last.
Change-Id: I77575f7afe0f9276c7b54ee44d828e7ccb87c978
These methods are special in so far that they create *minimal*
wikitext where optional whitespace is not preserved. I tried
to rename the methods to reflect this, but could not find a
caller. What's used instead are the .serialize() methods.
Bug: T284895
Change-Id: Iedaa5b7efa9675151cc0553854d8aef3f9a46cbb
If a known parameter is present using one of it's aliases, then
only the aliased name should be shown to the user. This patch,
therefore, resolves the issue of the same parameter being added
to the sidebar twice.
When adding a parameter that is aliased, it will receive the same
position as the non-aliased parameter it is replacing.
Bug: T274545
Change-Id: If4e58c941fd0f0e690d3603935f5a5d3f9938163
It's allowed in values, but not in parameter names. The moment
a parameter name contains an `=` the parameter name will be cut
off at this point, and what's behind the `=` will become part
of the value.
You can test this on any live wiki. Open VisualEditor. Edit any
template. Add a parameter with a name like `a=` and some value.
Switch to wikitext mode and back. Edit the template. The `=` is
now part of the value.
Bug: T98065
Change-Id: I5e00e8fac987471243605816b041d3638927ac3b
When what you type is a partial match, you can't add it as an
unknown parameter, even if that would be the correct action. The
reason for this unexpected edge-case is a mistake in the code
where a variable called "exactMatch" is set when a *partial*
"nameMatch" was found.
Bug: T285940
Change-Id: I6d12e2d7251a19d7d5f8be544c3c32a3ac14fcf0
The so called "spec" class keeps track of parameters that have
been used before, no matter if documented via TemplateData or
not. Removed parameters are still "known" (i.e. have been seen
before).
This feature allows to easily find previously used parameters
names when an undocumented parameter was removed and the user
tries to add it again.
Bug: T285483
Change-Id: Ia1555eea87cd99e7a3f386f4279ec5a80fb98a79
I rearranged this piece of code like a dozen times before I
finally understood what it actually does. This should be much
more obvious now.
The idea is:
* If no edit was made the button is always disabled.
* You can save pretty much everything, except when the
transclusion still starts with a placeholder.
* You can also click the done button when the dialog is empty.
This feels a bit odd, but was like this before. I think this
codepath is unreachable. But it probably doesn't hurt to
keep it.
Bug: T284895
Change-Id: Ic483201b64fd64f414c5b1ec4c44198b8eadb9f2
These tags don't do much, if anything. But they provide a hint
in which scope a method might be used.
Bug: T284895
Change-Id: I0b4bdd416ee89d26961c4ded4d8bbace8c57da76
While the term "canonical" is not wrong, I find it still
somewhat ambiguous.
1. "Canonical" could mean different things. E.g. is the order
of parameters as they appear in the article's wikitext the
"canonical" one? It's possible to argue like this, esp. if a
template doesn't have TemplateData documentation. In this case
the only order known is the one from the wikitext.
2. "Canonical" sounds like the parameters must be reordered.
But this should never happen. Not having dirty diffs is more
important than having the parameters in a specific order.
Bug: T285483
Change-Id: I23658d37fea50b727667677ac6a49066673b2135
This reverts commit 950a5300dc.
Reason for revert: This broke several workflows. The reason is
that MWParameterPlaceholderPage & MWParameterSearchWidget both
hold references to the MWTemplateModel. This model is not
always the same. The dialog might be the same when a template
is edited multiple times. But the model might be a new one.
From this point on the MWParameterSearchWidget pulls data from
an outdated model.
Bug: T284636
Bug: T285571
Change-Id: I7b9ea8cab8f17705ec8020f07e3732da6ba0e73c
