Instead of the template that's currently selected. That's confusing,
e.g. when you click one of the search fields, search for a parameter,
can't fine it, and press ctrl+shift+d to add an undocumented
parameter. Or you navigate the list of parameters with the cursor
keys, can't find the parameter you are looking for, and press
ctrl+shift+d to add it.
Still falls back to the selection when the focus is outside of the
sidebar.
We suggest to merge this before UX review, and review it later in
demo time. It's easy to undo if necessary.
Bug: T313388
Change-Id: I9848dd0af4fe821526dafc18bbd7cb1ab0e68cfc
The need for something like this was anticipated in
I2bf43c7e83283f43e047229eb53c244918fcbb0c.
As of version 2.5.0 of Parsoid's output, if alternate text is missing
for an image but a caption is present and image isn't displaying the
caption (ie. it isn't a thumb or frame), then the text content of the
caption will be set as the alt attribute. Parsoid will then drop the
alt attribute when serializing if it matches the caption text, since
it's unnecessary.
However, if the caption is modified and the alt text isn't, the alt will
be serialized. This is likely to be unexpected to editor. They may
have missed that the both the caption and alt are populated in VE and
only edited one place.
Since all of the above is happening only for images where the caption
isn't visible, it doesn't appear to be a much used feature since, at
least for inline images, the experience of caption editing was already
less than optimal.
However, because of a quirk in how galleries are rendered in Parsoid,
this affects gallery caption editing, which is visible and presumably
used more often. See T268250 for a discussion on an improved gallery
structure. But for now, gallery images are effectively inline and set
the alternate text, thus subject to the above.
Here we add a checkbox so that the default is to ignore the alt if it's
the same as the caption. And only make use of it if it differed
originally or was explicitly unchecked to modify.
Bug: T311677
Change-Id: Idf297d8a98995971c5835b0cea56c3317a3626e2
Turns out we have two concepts, now represented by two methods:
1. A top-level part can only be moved or removed when it is actually
selected. This is relevant for the toolbar buttons and for the
keyboard shortcuts/hotkeys. We intentionally block the buttons
and hotkeys when a parameter is selected.
2. Adding a new part or parameter is always possible, no matter if a
top-level part or parameter is selected. This is again relevant
for the toolbar buttons and hotkeys.
Bug: T313388
Change-Id: I17caf8fce9d8f1ebe21660cf8c6d91ace8423490
The hotkey was only working when a top-level part was selected, not
when a parameter in a template was selected.
Some outdated helper methods are now marked as deprecated. They will
be replaced and removed in later patches.
Bug: T313388
Change-Id: I5ffe45fd00c36b97ee36dc0ba6831db5a941c731
This is a partial revert of Iaf089f4. It restores the old behavior:
* In case there is already a highlight in the parameter list, just
keep that. Usually there is no highlight at this point, but better
have this check in place to be sure.
* Otherwise always start at the top.
Jumping to the selection is confusing, esp. for keyboard-only users.
The argument goes like this:
* Let's say I'm in the middle of editing values on the right side of
the dialog.
* I want to navigate to the sidebar. How do I do this with the
keyboard? I use the tab key.
* Pressing tab also implies I move the selection to the next
parameter. And the next. Until I reach the end of the parameter
list. Then the selection stays there.
* When I finally reach the sidebar and tab into the parameter list,
the last parameter is selected. But this was merely a side-effect
of me navigating the dialog.
Such a "selection becomes highlighting" behavior was not specified
in T311204.
This patch is requested and approved by PM.
Bug: T312647
Bug: T311204
Change-Id: Ie5b5dfd4fca132050815e6182845ca23adb5f805
The main motivation is to get rid of the vague method name
"setParameter" that was previously used for three different methods
in three different classes. Now the three methods have three
different names.
Change-Id: I938de30b368daf6ce3385b2ed2bca98f316593e1
We would love to name this state "selected", but that term is already
used for a template parameter that is checked/used. The idea of "set"
was to have a list of parameters where one is "set". But the word is
confusing. I suggest "active page" because the entire purpose of the
blue selection is to highlight the currently active page (i.e. the
one you currently interact with on the right side of the dialog) in
the sidebar.
Change-Id: I5a16ab4c193ea05c21bb3bf89ada2ef550d8d6bc
1. Before, removePages() was calling setPage() with null. This makes
sense for removed top-level parts because these are really removed
from both sides of the dialog. Template parameters are never removed.
Only unchecked (and removed from the right side of the dialog, but
this is not what this code is about). When I navigate a parameter
list and uncheck a parameter I need the focus and highlighting to
stay.
2. We have a dedicated method when a parameter is unchecked. This
can check if the removed parameter is also the selected one (called
"set" in this code) and can reset this state. Without losing the
highlighting or anything else.
Bug: T312213
Change-Id: Ibb717ca49cae805617ebee196937c79daa72f1c1
Optimization: Don't search for a checkbox that represents a not yet
named parameter placeholder. There is never one.
Fix: Store null, not undefined.
Bug: T312213
Change-Id: I395008f15d13133ad456d0a77571b7aa1c7a7fc9
Steps to reproduce:
* Set a breakpoint or debug.log() at the start of
TransclusionOutlineWidget.setSelectionByPageName()
* Edit a multi-part template.
* Use the keyboard to navigate to a template name in the sidebar.
* Press space.
This is currently triggered twice. Let's get rid of the more obscure
one. It was introduced as part of Ic4ee673. I don't really know why.
Bug: T313207
Change-Id: I3ddc072f5d42c17249abc82026e0bf1a4be1dc6e
This also means we have to move the declaration/documentation of the
event up one level into the generic "part" widget.
Change-Id: I1b803201f8955b58136ee7f37c04c01edcd47395
The code that receives this event does not need to know that the
source is a "parameter". It's just some "item" in the sidebar. The
idea is to reuse the event for both top-level parts as well as
parameters. This will be done in the next patch.
Change-Id: I858040f5adf8e156b6013caaa527b3237b7bac0f
What I found is a single `$container.animate( animations )` that's
responsible for the misbehavior described in T312768.
This method is called from a few places. I think none of these places
benefits from making this an animation. This is often more distracting
than helpful, especially when navigating with the keyboard.
Bug: T312768
Change-Id: I90a80d6ae8c1b47ee22297d2520255cad890b90e
Fix setParameter() and let it (un)highlight parameters independent
from a selection (called "set" here).
Bug: T312925
Change-Id: Ie4e9ba94659f4f70160193ca6bec804f8a4473e4
Note how the two-pane layout was already handling the two events
identical:
sidebarPartSelected: 'onSidebarItemSelected',
templateParameterSelected: 'onSidebarItemSelected'
We move this knowledge up one level into the sidebar. Both actions
now trigger the same event.
This implies that we are now using the event formerly known as
"sidebarPartSelected" to act on both top-level parts as well as
parameters. We reflect this by renaming it to "sidebarItemSelected".
Change-Id: I9a2c95f91f05de7312d38ec4c8b360141a0c447d
This is not only fired when a parameter is added. It's also fired
when pressing spacebar on a parameter that already exists.
Change-Id: I245aa9f5938eb38c3a3f224a4d642d57068cf23b
This is not only fired when a parameter is checked or unchecked.
It's also not only fired when another parameter in the parameter
SelectWidget is selected. It's always fired just because the
spacebar is pressed. This is so we can scroll that parameter into
view.
Change-Id: Id621405b7ca3116cd4a06f474e49776d0830dccc
This regression from Iaf089f4b271fd853b17c1aa7f5938510ea8f5431.
Not calling the parent here was is essential, because when clicking
the checkboxes this event will trigger the focus of the whole widget.
That's not what we want in this case though. Also it seems we don't
need any of the logic from the parent and can get rid of that
complexity.
Bug: T312855
Bug: T312856
Bug: T312924
Change-Id: Ib3a2eeeb6d519dfa1bb627049f6b3a4708017e86
Triggers scroll in all situations where highlighting changes.
Including keyboard navigation.
Bug: T312542
Change-Id: I849f64c5cefe0cac3fde6e848b23b7b0cfc489ce
Introducing a set method to have a different state for a set
parameter and a highlighted one in the selection.
Allows us to remove a lot of workarounds for missusing the
highlight state and fixes several issues with these workarounds.
Main implications:
- Keyboard navigation and mouse hover now sets the grey highlight
- If a parameter is set (blue highlight) keyboard navigation returns
when focusing the SelectWidget
- If nothing is set keyboard navigation starts at the top after focus
- Unchecking a parameter using space will not influence the keyboard
focus in the list
- Highlighting a parameter with the mouse lets keyboard navigation
continue from there.
Bug: T312647
Bug: T311204
Bug: T312213
Depends-On: I385dca1d95033961d3844e888521750443e49c95
Change-Id: Iaf089f4b271fd853b17c1aa7f5938510ea8f5431
This prevents an invisible and unactionable focus target when the
parameter search field doesn't match anything.
Bug: T312547
Change-Id: Ib29913a547cd68649d29e28d921c4c1358bad7b8
We don't want to restore selection. Instead, we'll remove all
parameter selections onFocus, in a later patch.
This reverts commit 787d44af66.
Bug: T312017
Change-Id: Ia1dc8061dfd1813a58befff5adc5c3882b54d8e2
This should be safe because selection is only set from use cases
in which we do want the sidebar scrolled to the matching item.
Bug: T290975
Change-Id: Ib0c66bb048768d633d0f638c775eba24cd652db8
When blurring out of keyboard parameter navigation, the
last-selected parameter will be highlighted again.
Bug: T312017
Change-Id: I8b0fb667b44b324529d4c45c39bf21573517f989
Tabbing into the parameter select list should highlight the first
parameter, but focusing a checkbox by clicking it to uncheck should
not trigger this highlighting.
Copied from the superclass method.
Bug: T312014
Change-Id: Ia1fd450bad4861eb2815ca21eae69ee31e40ac08
New sidebar events take a `soft` parameter allowing reuse between
"hard" and "soft" selection and parameter add. The "hard" variant
causes a content-pane focus.
Bug: T311987
Change-Id: Ic49718840ae56eb4cfab01ce964a2fbc2d8db63b
If nothing has been selected, the action buttons should be
initialized in a disabled state.
Bug: T311608
Change-Id: I32fee0d73f6e13e09dc421c944df73b79e0260bb
It seems that the controls widget was the only side-effect we still
needed for maintaining existing behavior.
Bug: T310866
Change-Id: I507dacef4e56946b836b0fca31effce611260aec
This patch does a few closely related things:
* Replace a direct dialog → sidebar access with dialog → layout →
sidebar.
* Move the misplaced removeButton.toggle() to a more appropriate
place.
Bug: T311069
Change-Id: I5a2802aab587a6f7de4681bce4e9961a064ef8ee
Value changes are triggerd and tracked from the parameter page and
change events will be forwared to the outline, if they are relevant.
Initial value is read from the model.
Bug: T308730
Change-Id: I0a3b0faf40aee44889404dcce31d850714360580
Removing a class name that's going to be deleted from places where
it's not strictly needed, e.g. comments.
Bug: T307188
Change-Id: Ifb14695d05510d2c0e25623afa99c4e84af3aaf9
This focuses on some scenarios that are
a) complex enough to be worth a test,
b) but simple enough so I don't need to spend hours on comming up
with a test setup. ;-)
This patch also simplifies the ARIA related code in
MWTransclusionOutlinePartWidget a bit.
* Check 1 of the 3 ARIA configs only. Only having one is already
helpful and should not be skipped.
* No need for the large conditional. setAriaDescribedBy() works fine
with undefined.
Bug: T291157
Change-Id: I142782ec9b96147de64497f4f6a373eae05b9c8e
Will be removed when parameters are added. Needs different margins
depending on beeing shown in the single transclusion mode without
header or on multiple transclusions.
Bug: T300710
Change-Id: Ieb95d7276aa4d4b0fcbb74f87ab734e4a393dc21
Before, the fallback algorithm was somewhat adaptive, trying harder and
harder the fewer CirrusSearch results have been found. This updated
algorithm guarantees that 10 results are shown.
Warning: You might see only 9 results. The reason is a bogus, unrelated
behavior in the mw.widgets.TitleWidget in core that's used as a base
class here. There is a "showMissing" option that's apparently enabled
by default, and prepends a non-existing title from the main namespace,
ignoring the "namespace" option. This extra result from the wrong
namespace is later dropped by the very same widget.
This code here sees 10 results before the bogus one is dropped.
Disabling "showMissing" causes other issues. We would need a series (?)
of custom hacks to work around all this, but this seems inappropriate.
Let's live with 9 for the moment.
Bug: T303524
Change-Id: I2c577c9ef2752b6c6cd360f4023e151e9272fcd5
The main advantage of this change is that it drops the assumption
that the index starts with 1. This is not necessarily the case when
we prepend extra search results. Dropping this assumption here allows
to simplify such code.
* The incoming list of pages is guaranteed to be an array.
* There is no point (any more) that could cause the array to become
sparse.
* Note we still make a copy of the `origPages` array at some point,
e.g. on `.filter()`.
Bug: T303524
Change-Id: Ifbd92bb052155c613d2ca21ab6d54a0b3ef28c0c
This option is not only buggy (it just doesn't work when the namespace
option is set the same time), it is not useful in this context even
if it would work. It doesn't make much sense to suggest non-existing
templates in the context of the template dialog. If adding a
non-existing template really is what the user wants, they can still do
this by simply typing the name of the template and submitting the form.
We never need this to show up in the suggester below the input field.
The main advantage of this change is that is saves 1 useless API
request that's potentially done every time a key is pressed.
Bug: T303524
Change-Id: I903340a06d6e6490bb58f628f41903aa044ccb21
This separates the two steps:
1. See if items in the list of `origPages` miss their `.index`
property, and add it if possible.
2. Later code doesn't need to care about redirects any more.
Note that `origPages` is not used for anything else. And even if,
it's not wrong to have the index for each search result on both the
redirect and the redirect target.
Change-Id: I12135f0430c944b4e33c49ece7779d7c3bb6c211
This is not a pageid, but a simple numeric index in an array. Luckily
it doesn't make a practical difference for this particluar way of
iterating something.
Change-Id: I7ec9ace00d4fba7adde17670058a0365b30f5617
The result is guaranteed to be in formatversion=2, where the list of
pages is an array, not an object.
Change-Id: Ic73a68c3e249a70108a6a19a89f4ff6c475794ed
This code is optimized for the 2 most relevant use cases:
1. When Cirrus finds 10 results, we still want to search for the top 1
prefix match. This is critical for templates like !!. This will appear
at the top. unshiftPages() makes sure the limit of 10 is enforced.
2. When Cirrus fails to find anything, we search for 10 prefix matches
and use these instead.
The code can also handle everything in between. For example, when
Cirrus finds 5 results, we search for 5 more prefix matches and add
them when Cirrus missed them. The total number in the end might be 5 to
10 depending on the number of duplicates. This is intentional. Why?
Let's say we always search for 10 prefix matches and add them to the
top when Cirrus missed them. This might remove _all_ Cirrus results.
This shouldn't happen. This extra code is only to fill in glaring gaps,
not to replace Cirrus. 5 results are fine.
Bug: T303524
Change-Id: Ib0471795124c0c7001b6901edaf8e7b380e426b1
There is already some kind of "fallback" to prefixsearch. We always
check if the top-1 prefixsearch result is part of the result set.
Because of this the current worst-case scenario is that only this
1 result is shown.
This patch implements a full fallback to prefixsearch. But only when
there are 0 CirrusSearch results. Further tuning might be done in
later patches.
Bug: T304925
Change-Id: I1927eedad60c9b9ac2021481a85376c08ccf6fdb
In detail:
* Just compare strings where possible. Sometimes the first character
alone is distinct enough.
* Add missing ^ anchor to avoid possibly running unbound pattern
searches. This is not strictly necessary when we know the input is
fixed-length. Still I find it more robust and more obvious what it's
meant to do. Not to "search for some match somewhere" but to check if
the input _is_ a match.
Change-Id: I9b7651d8ec1266521c22b38ccabb95a947903f72
.test() is the dedicated syntax for a boolean "does match? yes/no?"
check. .match() returns an array of matches, or null. This is just not
needed in these situations.
Change-Id: Ibb996ab843d1a6c7d7af98d6a112990665d543b2
We ended mixing two concepts in a single method:
* We need a method that allows us to create the parameter list widget
when it's needed, even if it's empty. This is relevant when a template
is entirely empty, and the first parameter is added. This wasn't
working. Instead the parameter list was created with all parameters.
* On initialization we either want all parameters to be shown, or only
used ones. But this code is only needed once, on initialization. I
ended inlining this code in this patch.
Bug: T300640
Bug: T304046
Change-Id: I6620a870e4420dcb8fecf522b3274458eeec891d
Due to the stickyness we need to scroll the header and the first
visible item from the parameter list to have the best result in all
cases.
We also only want to scroll when the user triggered hiding the
fields. Not when the template outline view is initialized.
Bug: T302965
Change-Id: I84d293888a7dbf13ec655c293c0fc3a79edca698
In a test case with 200 templates where all but a few parameters are
unused the loading time is cut in half.
Bug: T300974
Change-Id: Ice850cb9e5e95b9e3a19ff511b3a4f32117c7199
Same random finds while working on something else. I carefully
checked and made sure these methods are actually called without the
optional parameter.
Change-Id: Iab36fd130258322985b5d6e7f8e1f7b4ee235ba2
