The idea of this piece of code is to make sure both sides of the
dialog show the same element. But it doesn't make sense to force the
*header* of a template into view when I clicked on a *parameter*.
Bug: T292718
Change-Id: I9945f8e54c856152f05bf717e43468ab5ab24d2f
I run into this in some local test. There are two reasons this code
can be reached:
* When a wiki doesn't have the TemplateData extension, the
additional API call from line #154 will fail. But the original
search query succeeded. We have the `originalResponse` and can
return it. This makes the code behave as if the additional
TemplateData API call was never done.
* But what if the original search query failed? We still end in
line #183 – as we should. But this time it can't return anything
but undefined. This will be considered a valid, successful API
response. But it isn't.
There might be a better way to clean up this chain of promises.
This is the smallest fix I found.
Change-Id: I02d3d053156da222ee424382007621f314777015
Instead of using an object mapping namespace ids to if they have
subpages enabled or not, pass an array of the namespaces where
subpages are enabled, reducing the size of the configuration that
gets loaded on all requests. Only requires a minor update to the
JavaScript that uses the value (check for array index instead of
object value).
Bug: T291729
Change-Id: Ia0ecac71721eceed52cc90f39ecc560bdf1b7f9b
I moved some code around and found that quite a lot of code wants to
know:
* Is the length of this transclusion exactly 1?
* I need that 1st part.
There is more that can potentially moved from the dialog to the model.
But I don't want to make this patch to big.
Bug: T292371
Change-Id: Ia94ed0450d04dd97c4c41f5bf7c266f9a534e821
This includes some moving of code. These helper functions seem to make
more sense in the TemplateDialog class.
Bug: T292371
Change-Id: I004405bab60a569b084f9083fefa41f44f9a5561
I hope this code is a little cleaner. This patch doesn't change how
the dialog behaves.
Bug: T292210
Change-Id: I237812b3404437948eb76d8b36dcca2a4c688d6d
This method does not only select a "part", i.e. a top-level item like
a template. It also selects sub-items like template parameters. The
new name reflects this better.
Change-Id: I51a8ddbd05b283248afba5a623cc52da7b2434f5
The TemplateData extension is not limited to the Template: namespace.
And even if pages outside of the Template: namespace typically don't
have TemplateData information, the API is still able to automatically
extract parameter information from every page.
Or:
It's rare that a page outside of the Template: namespace is used as
a template. But if it is, this is not a mistake. The code here in
VisualEditor should not try to be "more clever" than the user is. If
this is what the user want's to do, let's not block them.
Bug: T291883
Change-Id: Iaf3fd5530b74fab7cedfc85ce04c8c40632df11f
The feature set is (almost) fully covered by other tests, notably
the tests for the …OutlinePartWidget base class and the other two
subclasses.
The only bit that's not covered any more is the
"visualeditor-dialog-transclusion-wikitext" message. But that's
super minor and not worth a separate QUnit test.
Bug: T291157
Change-Id: I574f9cff0baf3dff885094769c124a9e05a1d1c8
The code that uses it is commented out
Bug: T291729
Follow-up: I7af2bc91524e832555b66f090a671672cd14f294
Change-Id: I4cceb9ca83a2274fa93783af3608b9486b773522
It's not only used as an event handler, but called as an ordinary
method as well. Let the name reflect this better.
Change-Id: Ie5a0d9c4cd072063a164886f18d0859327b3f267
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
Most block-level things that can be interacted with by clicking have a
highlight. Categories don't, and that makes it harder to discover that
you can edit them.
Change-Id: I6be3824c34b36bd09bbae8cab9a9f36b6bcdb767
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
I was (again) wondering why the try-catch is there. But it's actually
needed. Now covered by a test.
Bug: T290140
Bug: T291062
Change-Id: Iccc7274ed9e7b81b8491dbca7d3d771706b0ed09
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
