This is split off from the unfinished patch I039d6f6. This only moves
existing code around, makes use of chaining and such to make code
more compact and hopefully more readable. Methods are renamed to
reflect better what they actually do. No behavior should change.
Change-Id: I3ba538c8c77ad4455bf0f0aa821ca14feadef7cc
Note this implementation introduces some technical debt: It adds a
little bit of knowledge about what "part widgets" and the toolbar are
to the parameter SelectWidget. I think this is acceptable.
A "cleaner" implementation is probably so complicated that we don't
want it in the code, for such a minor benefit. However, alternative
patches are very much welcome.
Bug: T313703
Change-Id: I957698d58a7622cbe54bcc2ba454388ba9f09537
The edit tab should be active as soon as the editor starts loading
(to indicate it doesn't need to be clicked again, and that the read
tab can now be clicked).
Change-Id: I450c53eef64c25e9520d3868b4ecc95204644138
The .selected class does nothing on mobile, nor are the tabs visible
under the full screen overlay.
Change-Id: I14a6747f4a3274d71b7aa16b2c9b76b62a5253c2
Generally the default button margin on the parts is 24px. The only
exception are the placeholder and wikitext when they are the last
parts in the outline.
Bug: T312644
Change-Id: Ie513bf1c022b2696cc92aacbbca59ddf6e55043e
In Iebfe2e2 we already tweaked this by 1px. Turns out this was not
enough in all relevant situations. I still get random scroll events
just because I move the mouse around. Setting: Firefox, 120% zoom,
multi-part transclusion.
Bug: T312926
Change-Id: I475c1ef029e9721cc663881e40547730389cd26d
* Rename method because it turns out it is not only about the sticky
header, but also relevant when there is no header.
* Move some code to more appropriate places.
* Use 0 as documented in OOUI, not null.
* Set the padding back to 0 when the sticky header is not visible.
As of now this is an unreachable state because the filters never go
away after they have been made visible. Still this code was always
written with this possibility in mind to make it future-proof.
* Performance optimization for the boolean "show filters?" check.
Bug: T312926
Change-Id: Iaba08ccd8bf00360fd26f9268d5be43df4f4fbd8
This is a partial revert of Ide45141. Now the scrolling always
happens (again), but properly considers the presence of the sticky
header. It was also not correctly initialized on construction time.
This is a candidate for a backport. The patch is intentionally as
small as possible because of this. Code cleanup will be done in a
later patch.
Bug: T312926
Change-Id: I06425b42566bfb2087846636055ee75e98a05029
The message was also shown when a documented template appears as
part of a multi-part transclusion but with zero parameters being
used. You see the filters in this case and can click "show all".
The message is just wrong in this situation.
Bug: T312926
Change-Id: I8d26ceec483e05fd1f69013e506fa1eb4e4c29ed
Currently the sticky size is exactly 114px. With 115px you also can
see an auto scroll effect on multi part transclusions when you hit
that 1px sweat spot at the top of the list.
Bug: T312926
Change-Id: Iebfe2e2c6360c650755cd985157949a26a5287a4
Introduced via Ibc56abf. But the OOUI SelectWidget does have some
methods for this already.
This patch also updates some @mixins documentation.
Bug: T302965
Change-Id: Iffbb44d41586786a2165f8d7916f94a52ad19122
The rename in I5a16ab4 was done the exact same time a new usage was
introduced via Ibb717ca. Neither patch shows a conflict. Still there
is one.
I remove the extra `|| null` line because it is just not needed. We
don't need to set the property to null when it was already null.
Bug: T312213
Change-Id: Id3f025786c9412e8c1946434113c41356da08098
Previously we could log a warning if the user had the editor enabled,
but the page did not support being edited as wikitext.
Bug: T312632
Bug: T314171
Change-Id: I647cb057ed44c155b4eba1b728d6908f8a7abb69
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
This is send as an associative array. Instead of adding elements that
are null we can just not add them.
Bug: T291729
Change-Id: I28d847941eec865cb255779534eca14ec88f588f
Log a client error when VisualEditor cannot load because of an
incompatible skin (which is not supposed to happen in practice).
Bug: T312632
Change-Id: I2ccfaa584d7a05e10170a66d446bd4e476dcc775
Depending on the order in which this code executed, sometimes the
dialog initialization would overwrite the readonly state set by item
initialization.
They should simply all check both conditions.
Change-Id: I6a18f1e074f118423438c017b3e4e34e75579e5d
This code was for when the dialog had a trash can icon for every
parameter, and parameters could actually be deleted. It's unreachable
now.
We missed this when removing the old workflow.
Change-Id: Ic94df506ea84009a1e1863a4e9847a70498df448
OOUI support for multiple modal window managers is hacky, and only
works correctly when the managers are attached directly to <body>.
Remove the wrapper that doesn't seem to be necessary.
Bug: T313690
Change-Id: I4134c0f50d28a364dcf15b426bd9b59a4f7a985d
The dialog is unusable when there is no outline. See T313489 for a
longer explanation.
Bug: T313489
Change-Id: Ib2cc9c363d3596a16f6f1c4aef03ca216abf6b1f
Apparently this can be undefined when Esc is pressed. Note this code
cleanup related to but does not fix T313690.
Bug: T313690
Change-Id: Ia4658f8e00a68ed4cc3a6ddb0a932b3218b813dc
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
Same issue as in the previous patch, but less intrusive. It was always
possible to add a new part, but it was often inserted at the wrong
position. It worked only as intended when a top-level part was
selected. When a parameter was selected, the new part was always
appended to the very end of the transclusion, not after the selected
template.
This is now a little bit of duplicate code. We might extract this to
a method in a later patch.
Bug: T313388
Change-Id: I1327222969d1d315bdacf3998f366d88c4c26bd5
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
This should make zero difference in most situations. Except you
navigate a list of parameters with the keyboard. In this case the
SelectWidget gets a dark blue outline which overlaps with the light
blue selection bar, but the outline disappears behind the bar. This
looks odd. Making the color transparent fixes this without the need
to fiddle with z-index or such.
Bug: T311204
Change-Id: I7049eb60dc0ea72c2c4620f4351525fe447e0f46
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
I hope this makes it a little more readable. The two steps done in
the loop are mostly independent:
a) Find pages that should be removed.
b) Find next best top-level page when the current one is removed.
Change-Id: I600253fb206a31ef5851865e733b66c336d5014d
This does not have much of an effect, but can cause visual glitches
in rare situations. One goes like this: Use the keyboard and tab key
to navigate to a list of parameters in the template dialog. Press
space to enable the checkbox. The parameter gets a blue background
(= it's now the active a.k.a. "set" item). Press space again. Blue
disappears, as it should. Press space again. Blue is now missing.
Bug: T312213
Change-Id: I3071ec4d0a05e3505ec5216acc5a97b8eaf6f5d5
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
There are many errors that are temporary in some way, and treating
them as unrecoverable is a poor experience.
Even for errors that really are unrecoverable, our interface works
poorly, because you need to hide the error message first to do
anything else, and you need to close the dialog to see it again.
This distinction is not really helpful, let's get rid of it.
Bug: T307330
Change-Id: I9680cc416da5b27881aeb3502f506dcb5d4bb71f
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
These comments just repeat what the code says. Most of them are
copied from OOUI. Some talk about BookletLayout – a class name we
don't use any more.
Change-Id: I7ce354dfe059657395103ffef767eb8f6d37bfb7
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
The optimization removed in this patch is done twice. The effect is
that the loop does not end when it finds a match but the match happens
to be the current page.
This doesn't change anything. It's really only about performance.
Change-Id: I5c2de101eb2f14f814f00cf7eacf46b70346f4c8
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 line was added as part of I0229b63. While what the comment says
is still true to some degree, the line stopped being helpful when
we introduced sticky headers. When scrolling the sidebar for a
multi-part template with many parameters the sticky header jumps up
and down by this 1 pixel.
TL;DR: This is one of these hacks where it's better to remove it and
look for a better solution when we notice the original issue again.
Bug: T312768
Change-Id: I2fedea4e1d4d6c95c74a63c522821a6ebc2ee2b2
250ms gaps are pretty common when typing at a normal speed, resulting
in a lot of (often expensive) API requests (e.g. T312319). For most
use cases this level of responsiveness in the preview is not necessary.
Change-Id: I6504567d4da02a66171ee35a9c4fd35c85136909
This patch contains a small cleanup that moves the scrollIntoView into onSidebarItemSelected.
Since all other places already have a focus call, scrollIntoView is not needed there.
Bug: T312850
Change-Id: I1a3cb3905faea2bd8a6bf8dc4cfd748813e1c875
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
Using Parsoid HTML in the 2017WTE has enabled us to iron
out lots of rendering bugs over the past few years.
In that time Parsoid has been moved into PHP, and at some point
we also become the default parser.
Also more extensions have started to use content transform hooks,
which are only supported by the action API.
As a result it now seems like a good time to migrate back to the
content API instead of building the preview from Parsoid HTML.
Bug: T154844
Change-Id: I90d775dd71d5f5a61d651b63d946ab60a27e2ca3
Fixes a bug that crashes the interface when unsetting the focus after
another input was focused, because `page` is undefined in this case.
We were already calling `setPage` to unset focus, so document it.
Bug: T312017
Change-Id: I2926beffe23e0ee822f5690d4791534a5413886a
This can only happen when deleting the last part of a transclusion,
then we want to set the previous page as selected. This should never
be a parameter though.
Bug: T312221
Change-Id: I06700dcf37f6d4f7dd073f8f2bc1b8b0a17a62c4
Just putting this eslint auto fix out there. No hard feelings, but
a warning that pops up everytime.
Change-Id: I157e0338e5f5a6f27dbbd9ae116da0f922468586
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
It is no longer necessary to prepend a colon in Parsoid HTML
to ensure they are interpreted as links rather than an image
inclusion or categorization.
Instead, the colon causes Parsoid to generate piped links
when they could be unpiped, so remove it.
This code was added in 1e62e9f64c (2012),
the Parsoid bug was fixed in b62b93c678 (2013).
Bug: T312700
Change-Id: I3d71fd658b5dd627445e60b850f647081ef842e7
This prevents an invisible and unactionable focus target when the
parameter search field doesn't match anything.
Bug: T312547
Change-Id: Ib29913a547cd68649d29e28d921c4c1358bad7b8
The last checkbox in the entire sidebar should get the extra bottom
margin, but not the last checkbox in each template group.
Follow-up on I1edc5db98d16a4c0de8abd7f705776fb9eb65b97
Change-Id: I4ffade9c053191ce202340edadbd032c67bb39a4
Causes problems when we remove a parameter using the spacebar, for
example.
This partially reverts Id3843b2c7ad6.
Bug: T312524
Bug: T312205
Change-Id: I65d834bc0d2cc649803b536ecc65bdd1fa166a32
Using a certain amount of depth to make sure to override OOUI
specificity.
More can be done in follow ups.
Change-Id: I1edc5db98d16a4c0de8abd7f705776fb9eb65b97
Should be a noop. Also moving one rule further up to the set of
similar selectors.
Removing one rule that was disabled for some time now.
Optimizations follow.
Change-Id: I8da70a52c13afd8ac1c3ff43bae63a203c3bf86a
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
Gets rid of some unused behaviors that we've already disconnected.
Brings the remaining styling into VE files.
Bug: T312524
Change-Id: Ie94472019ba41124831621c45713861297219594
Both parts are not relevant anymore:
- The first part is about focusing parameter placeholder pages.
These are added after load only when using the keyboard shortcut.
Focus will be done separately in that case.
- The second part is dead code since some time now. Also it is not
needed here. Scrolling to newly added parameters is done via the
setPage() method in the layout.
Bug: T289043
Change-Id: I928afef01b9e62a9da91db10340d9df78717e125
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
... except on mobile because the keyboard will expand.
Need to disable a spurious focus when the element isn't visible,
because this was making it impossible to focus later once it becomes
visible.
Bug: T290975
Change-Id: I0e6057d1cfbef24324a287e50e4988f935720c61
Instead of using a potentially weird fractional value like 3.2 lets
use the floor(ed) value, e.g. 3. Reasoning: These fractions are not
from text lines but optional margins between some of the lines. These
margins should not be counted.
Also increase the cut point from >=3 to >3. This is much closer to
the original specification that talked about "2 lines plus x
characters". See T273426. These "x characters" are the 3rd line.
Bug: T310775
Change-Id: Ib5d1642d7967593dd5f13e556c6244fc44677af4
When blurring out of keyboard parameter navigation, the
last-selected parameter will be highlighted again.
Bug: T312017
Change-Id: I8b0fb667b44b324529d4c45c39bf21573517f989
`OO.ui.PageLayout.prototype.setActive` explains that the "active"
state only makes sense for a case we don't support, when a booklet
layout is used in non-continuous mode.
Change-Id: Ib5ddd605f29e82ebc85b833ac96b70ff4c95e502
Most of setPage's side-effects are redundant and can be skipped when
reselecting an item which already the current selection—except for
scrolling, which is useful to perform every time, for example when
clicking on the already-selected sidebar item.
Bug: T294348
Change-Id: I90ba14817e5536ac018eb70102f657f29a29644d
This was using the currentPage from the StackLayout. It will not
be updated there anymore.
Also removing the line about focus after toggle. We do not want
that anymore afaik.
Bug: T312015
Bug: T289043
Change-Id: I8b6eedd580d49604014118171c6da62849752d53
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
Only works with changes in OOUI core. See https://gerrit.wikimedia.org/r/c/oojs/ui/+/753987
Depends-On: Ie822dc87bc5748985de5637cb35f1940837f64d3
Bug: T299036
Change-Id: Idaccc7c95d67abe14621eabb7725ba07e449ab1e
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
