According to the docs:
The Element.append() method inserts a set of Node objects or string
objects after the last child of the Element.
String objects are inserted as equivalent Text nodes.
When given a string we want to treat these as HTML
Also fix the storybook which hinted at this being a problem
(incorrectly fixed in 504eefcc1)
Bug: T341318
Change-Id: I415c9821a6a37b45cf0f8a65f58a41bc7d0a042d
The footer link shows when a page preview has been disabled
by an anonymous user via the settings cog.
Change-Id: I236a620322716f42443fc3d90a0405352132a99f
Previously we did a try/catch when closest didn't exist. Now that's
gone we're flagging this issue.
Follow up to Ia45c03d1fd6949bf83ebed6d40075e453e42cdd7
Bug: T336650
Change-Id: I7a4f453ae1a4a8222863a8342a27d615883d339a
For fetch and AbortController we provide native polyfills (see
resources/src/skip-web2017-polyfills.js) so safe to use this here.
This will be empty for modern browsers.
Change-Id: Ic0f55eb0a0276be3587a4b866834bddff1124ad2
Rather than obscurely loading instrumentation code as a side
effect of loading UI code, run it explicitly inside the index.js
initalization code. Instrumentation is moved to its own file
and Popups modules now support an init function.
Change-Id: I9d2643ec8fb4e1dedc7ab9534b2965272f12335f
eventlogging reducer was removed in
I640ab367cd235ab8da7dd70dbef7ae9076712e84 so this is syncing
a non-existent value.
Change-Id: I58386bf9a0219f6344c08f757af3aaafbae92751
Certain browsers have the closest method but do not
support not selectors with multiple arguments. This
variant caters for both.
Bug: T325113
Change-Id: Ib5fc912bfe0f831fea4c9882c25b27541d83b66f
Follow up to
Iefe98c1f0422dbf034e385b1a41a859d030a2cf4 where we switched from the
jquery event delegation pattern to native methods. One thing that we
overlooked was that we also need to consider the case where the selector
matches the parent of an element, for example a span nested inside an
eligible link. I've rethought this logic, to first find the closest
eligible element to normalize the element passed to model methods
before running matchesSelector.
Bug: T325007
Change-Id: I4133751dc900a51829173e9c0d965cbb18e6a33e
Adjust the logic of elementMatchesSelector to fallback
to jQuery when the native matches fails.
Follow up to
Iefe98c1f0422dbf034e385b1a41a859d030a2cf4
Bug: T324514
Change-Id: I0e9e894e77e2eb29fee65853aa98b141bc2a11a3
Allow extensions to register new types of previews via
extension attributes.
Changes:
- The check for reference previews doesn't make sense
as $('a[ href*="#" ]' ) will match any elements with a hash
fragment, so the additional check to Title.getFragment
will not provide a different result. This was introduced in
I9ec57e0fbb0d21beaaa7b359c1c2bef64d2c14f5
- Links that point to themselves are marked with mw-selflink
in MediaWiki so this can use the not selector we already have.
- The new API is used internally and only available via extension
Attributes
- An example is provided in SkinJSON
(https://github.com/jdlrobson/mediawiki-skins-skinjson/pull/14)
Bug: T233099
Change-Id: Iefe98c1f0422dbf034e385b1a41a859d030a2cf4
This was done while working on T277639 where we introduced
multiple "enabled" flags for individual popup types. This
change listener is one of very few places that work with
this flag. This patch is meant to make this code more robust
and easier to change.
A few unrelated but trivial changes are included that make
code shorter and hopefully easier to read.
Also fixes a bug with tooltips overlapping previews.
Bug: T287119
Change-Id: I7fb0a8d4bb9f5e78fe62cfca524cc157ea89a233
The requirement for landscape images is to be at least 320px
wide. The requirement for portrait images is more relaxed,
only 250px high. Images that fall between these two
requirements currently don't show a thumbnail, even if they
could.
This change affects a very specific group of images:
* Square images from 250 x 250 to 319 x 319.
* Landscape images from 251 x 250 to 319 x 250.
* Landscape images from 319 x 250 to 319 x 318.
The most extreme ratio is 319 x 250. This will be cut to
203 x 250. I.e. the absolute extreme are 58px missing left
and right, but never more.
Requested at https://www.mediawiki.org/wiki/Topic:Vwl97pm6as9fuf6k
Additional stories for testing more extreme small images:
* Small Tall - 300x1000px
* Small Short - 300x200px
Update tests accordingly.
Bug: T268999
Change-Id: I811f1c0e7e9b0c30280b36a61cc7831a5b9e58c8
Turns out this was only in place because the test was
(intentionally) incomplete. But it's dead code in
production. Let's get rid of it.
Change-Id: Ieeb145b6972dceb7eeda3a167d907b680d5c3ce4
For example: We know the HTML element we are dealing with is
always an <a> element.
I believe the small changes in this patch are all obvious and
non-controversial. Please let me know if you disagree.
Change-Id: I9fe15845affffdd0f5f0fd6ef7d6b607cb567ac7
This is not really an error. It does not really have
consequences for the user, except:
* It might waste bandwidth because the module is loaded, but
unused.
* It might waste a tiny bit of CPU time.
However, this can only happen for registered users, and only
with very specific combinations of settings (notably: a
conflicting gadget must be activated).
The reason it's marked as an error is that previously we could
give more guarantees about this. This changed recently. We will
check if the log message is still helpful and either remove it,
or restore the previous guarantees.
Bug: T271206
Change-Id: I218726c9c4879a405acef68710e79c6ac8d070fe
Open questions:
* It is possible to make the patch smaller in case this makes
it easier to backport it. Is this worth it?
* This code is not covered by any test. I think it's worth
writing tests. Most of this code will stay, even after beta.
Bug: T281352
Change-Id: I5f30054f1664643b427909f7fa189b4ea5e11879
Use mw.track instead of dedicated tracker for VirtualPageViews.
This way we can migrate it to the new Event Platform client.
The new client does not observe DNT, which was the reason this
instrument was moved to a dedicated tracker on the first place.
Bug: T279382
Change-Id: I8bb515eab337ffed686ba7522bc6153cfdd8ca8d
This does not fully solve the ticket, as these are not the
actual OOUI styles. But it's already much better than the
unstyled checkboxes before.
Bug: T281227
Change-Id: I9a5023482774c09aa73845ca6dfd1c4926f088e1
This is the smallest possible patch for a backport.
While it seems there is zero technical reason to limit user
option keys to lowercase with dashes, it's best-practice.
Same as message keys.
Bug: T281235
Change-Id: Ia4a45cf4459543c81b23b757ae9c2cfaf9676894
We changed the handling of the popup settings. They now contain popup types (pages and references).
To not get confused with the old setting options (simple, advanced and off),
we would like to rename the local for the simple setting to page.
It should already contain the correct string.
Bug: T277639
Change-Id: I8847b890e9e31602277a92d82a188fcdd3eea855
* Change more places to not hard-code the popup types, but use
loops and such.
* Change many `function ()` headers to use the more streamlined
ES6 sytnax.
Bug: T277639
Bug: T277640
Change-Id: Ifece87d51012e0e069286453b27f5c9ae273710e
We added reference preview as a checkbox the the
anonymous user settings. To handle both popup types
(pages and references), we changed the usage of
preview.enabled. We pass on all types as a map
inside preview.enabled. The footer link to edit the
settings will appear for anonymous users if at least
one type is disabled.
Bug: T277639
Change-Id: I860a1b35ac7749d8d0884575f6acb7186ad8e4d0
We still have 2 different mechanisms in place, maybe even 3:
* We simplify the CSS selector when we know a popup type is
disabled, and it's impossible an anonymous user can enable it
at run-time.
* We create that "initiallyEnabled" map that allows anonymous
users to toggle the individual popup types at run-time.
* This map is also used to check if the footer link should be
shown.
* There is also a wgPopupsReferencePreviews global that acts as
a "kill switch". However, this is not a pure feature flag,
but incorporates the user setting for registered users. This
is currently partly redundant (checking
`mw.user.options.get( 'popupsreferencepreviews' )` does the
same) and can be removed later when the feature flag is not
needed any more.
The footer link currently acts odd because anonymous users are
unable to enable ReferencePreviews, but get the footer link.
This patch introduces a 3-state model:
* `true` acts as before.
* `false` means a popup type is disabled, but anonymous users
can enable it (i.e. this is the opt-out behavior for anonymous
users).
* `null` means a popup type is not available at run-time, for
nobody. Anonymous users can't do anything about this.
Registered users must leave the page and change a setting.
Bug: T277640
Change-Id: Id8d1396c09cf0f706034a66f9cd3c880a8b33df8
An "advanced" option was first introduced in 2014 via patch I374805e
(originally named "monitor-or-edit", renamed via patch I7b4f6d2).
The isNavPopupsEnabled() function was added in 2016 via patch
Ic660f48.
The code that disables the extension entirely the moment the NavPopups
gadget is enabled was added in 2017 via patch Ia474b1b (T151058) and
patch Ia837816 (T160081).
As of now, the "advanced" option can only be seen in an extreme edge
case:
* Only for anonymous users.
* Only if NavPopups is enabled by default for anonymous users.
* Only if the $wgPopupsConflictingNavPopupsGadgetName setting is
misconfigured.
* … or if NavPopups is not a gadget in the first place, but e.g.
loaded via Common.js.
In this situation the settings dialog opens with all *3* options. This
is broken for several reasons:
* The "simple" option enables the extension, but doesn't disable
NavPopups. Both trigger, resulting in both popups being displayed
the same time.
* Since "simple" is the default, this bogus behavior is the default
for anonymous users.
* The "off" option doesn't stick. Every time the settings dialog opens
"advanced" is checked instead.
* "Off" can't work anyway. There is no code to disable the gadget.
* Only the "advanced" option "works", but more by accident.
It's unclear how to fix this:
* There is no code that does anything with the "advanced" option. It's
not even stored. The behavior of the option is identical to "off".
* The code appears as if "advanced" was meant to be shown instead of
"off". I.e. anonymous users can only choose one of the popups, but
not disable both. But there is no code to hide the "off" option.
* The bug when both popups are displayed was fixed in 2017 via an
entirely different mechanism. Re-introducing "advanced" does not
only mean duplication, it's unclear how the 2 mechanisms are meant
to work together.
It really, really feels like this was just forgotten.
Bug: T278949
Change-Id: Iab21f3a649a5b2f19ebb0d0dbb45ce1450c65678
… as well as in one place in production code. The motivation
for this change is to make the code easier and faster to read.
There is a little bit of duplication in the test setup now.
But I would like to argue this is a good thing. The values are
rather trivial. The difference (or absense of a difference) is
much easier to see now.
Change-Id: I9aa95b59f0c45ea7c9257970e2fcdba3a000d234
This patch does nothing but rename a pair of variables:
"prevState/state" becomes "oldState/newState". Reasoning:
1. The abbreviated "prev" is confusing, especially because we
are in a codebase that is all about "previews".
2. We are in a context that is all about a state **change**.
Change listeners get notified about the change from one state
to another. While it would be possible to stick to the already
mentioned "previous/current" terminology, I find the word
"current" confusing. What is "current" in this context? Did
the state already change? Am I notified about a change that is
**going** to happen or already happened? Is this even relevant?
I don't think it is. Therefor "old/new".
Another possibility is "previous/next".
Change-Id: Id886e1a095967fe86fb9021f59e335c62da8994e
The nextState() function was not able to understand updates that
are deeper than a single level. Example:
nextState( state, { pagePreviews: { enabled: true } } )
Before, this would replace whatever was in the "pagePreviews"
property with { enabled: true }, but not merge it. In some places
a single level of recursion was done manually because of this.
This can be removed now.
This is done in preparation for splitting the "enabled" flag into
separate ones for each popup type.
Bug: T277639
Change-Id: I35911c18018ba7cd1633a4c882b978656c3fee36
Note how getIsEnabled() is documented: "if the user hasn't
previously enabled or disabled Page Previews […] then they
are treated as if they have enabled them."
In other words: The idea that the default should be true is
encoded twice in this code. This is just not necessary. We can
remove one without loosing anything.
Motivtion: Simplifying the code and reducing the package size.
Since the code fundamentally depends on this default value
anyway, we can clear the users localStorage when they decide
to go back to the default – instead of storing a "1" which
does the same as the default.
Change-Id: I2814a1e9269979918609162a508eeee6944d9e52
The main motivation here is to dramatically reduce the number
of places that use the same property name "enabled" for values
on different objects (e.g. "state", "actions", and "updates"
are all different things) with slightly different meanings. I
tried hard to come up with names that reflect better what each
meaning is.
Bug: T277639
Change-Id: Ie766259793f716262e3d4622ca55156d11f4842c
… instead of only 2 hard-coded nesting levels, as it was before.
This is done in preparation for splitting the "enabled" flag into
separate ones for each popup type. This patch here doesn't change
any behavior or internal representation of the states.
Bug: T277639
Change-Id: Icad669d1c9675ad6de22f478e254debe5d1936d7
