Pretty much all usages of this function do *not* use the second
parameter to pass a page name prefixed with a namespace, but pass the
page name only.
This patch here removes the redundancy. The namespace prefix is now a
generated one, saying "Namespace <number>:…". It turns out no test
relies on this so far.
Bug: T220097
Change-Id: Ibd45d49c91e86a2647afe676a5e3bb07dfeab6ed
config.get( 'wgTitle' ) returns the unnormalized title of the current
page while title.getName() gets the normalized title (e.g. with underscores ).
On pages with spaces in the check failed before this patch.
Bug: T220097
Change-Id: I58a532627bb27be030cbc553f1181a89109edd80
Named references may include non-ascii characters, so we decode the fragment before matching against reference IDs.
Bug: T220196
Change-Id: I63bba59fa8f0f6aa95aeadbb1f85745d480988bd
It is set based on the same conditional that loads the code,
thus checking it inside the loaded code is a no-op and adds
extra HTML to the <head> that blocks text/layout rendering and
delays fetching of Popups JS.
Bug: T219342
Change-Id: I9c1f4b3861ce2cecb654eb0a78469a616730a40b
I had to disable ESLint to be allowed to upload this patch. It starts
complaining about something in code I did not even touched. The error
message does not make any sense to me (something about globals being
forbidden in code where I can not spot anything that would be remotely
global).
Change-Id: I6d4b178a65126c4b81b87d99142a6cdc845ae5ee
Two big chunks of code (the "click" and "scroll" event handlers) have
not been covered with QUnit tests before. I found this was not that
complicated and worth the effort.
Note we already have browser tests in place for these features. These
are still required because the scroll feature can not fully be tested
when the popup is not actually rendered on screen.
Bug: T214971
Change-Id: I58111489fe6c4bed65efec59f9fc4184c828b2b3
With this I want to pull apart testing two things at one, checking if the
URL is escaped and if it is put to the right position.
So this adds an explicit test, that makes sure that urls in the popup are
escaped safely and lets the big test become more general in that regard.
Bug: T214971
Change-Id: I09b5225a8370e8b1337b2cf6ca03ccb79b3a64aa
When a thumbnail in portrait-mode is narrower than the 200px
expected width, the SVG clip-path should be shifted on the x-axis
in order to align with the thumbnail image.
Adds extra test-cases to validate this logic.
Bug: T204627
Change-Id: I9359c9fb335e5fad3f7d5ba33ee89d2a1f26b8b2
During story time on 2019-03-12 it was decided to consistently talk
about "References" in all messages. Main motivation is that this is
the term the community is most familiar with, and it is also the term
that is used in reference section headings most of the time.
Bug: T215063
Change-Id: Iaab8d2c0da1546a3c9d27bc8e2e1c784050ed135
The method itself has not much to do with gateways as such, it's
more about the general preview type selection. Since the preview
types "live" in the model, I thought it might good to move it there.
Doing that the "original" getPreviewType method in the model was renamed
to avoid conflicts. If I get this right, that method is quite specific to
page previews, since it processes the output from the TextExtracts API-
request. - Therefore I also removed the TYPE_REFERENCE there, because this
code path will never be reached with that type afaik.
Inspired by the comments in Id1fa7dad59d8fe80bc60c1e2d7c3fb4087e52d1f and
as preparation for that patch.
Bug: T215420
Change-Id: Ic9e24a73e945c7d56435c656ecfdb42b65601d22
Rename selectInitialGateway() to selectGatewayType() in
gateway/index.test.js. This test's import name was out of sync with the
declaration which made it more difficult to grep and reduced
consistency.
Change-Id: I60ae359043fdf76015d75b6438992ef6061a4d72
This happened one time before and was fixed via T212419. It now came
back after I2670331, a patch for T214861 that made the title parser
accept more links than before.
This patch implements a decision tree that goes as follows:
1. If it's a link to another page, it can't be anything but a page
preview, as having a peek into another page is effectively the
definition of a page preview.
2. Otherwise it's probably a reference preview, but only if:
* It's a link with a jump mark.
* The link is actually marked as being a "reference".
* Reference previews are enabled.
3. If neither of these definitions fit, do nothing. Note this is not
"hiding errors" we would be able to fix in this code. There are many
ways to manually arrange wikitext in a way that it looks like one or
the other, without fulfilling all requirements. Unfortunately the
user who would see error message or dysfunctional popups would not
be the same as the user who wrote such wikitext.
Bug: T216683
Change-Id: I8d021f19ddc73a261e6a0c62959ddd0cb1d3182d
These are annotations for ESLint as described at:
https://eslint.org/docs/user-guide/configuring#specifying-globals
I'm not sure where the `…: false` comes from. I assume it is a mistake
and does not have an effect.
I tried to move these annotations closer to the line they are about in
case there is only one line. And move it to the top when there is more
than one line using the global.
Change-Id: I4bd112c5fddd8a97d829a9b91707b8eb7cd7a332
While reviewing the code of the reference endpoint I found it encodes
two CSS class names, "mw-reference-text" and "reference-text". So far
our scraping gateway only implemented one.
https://phabricator.wikimedia.org/diffusion/GMOA/repository/master/
This patch also extracts a piece of code to a named function. This makes
it much easier to read, I feel.
Bug: T214908
Change-Id: I9d1bb1f4c21eb9d57a6b763ca1f756e6cf7049e0
These icons are currently unused because the gateway does not deliver
the necessary information. This will be used starting with I6223cbb.
This patch aims to introduce all resources needed by the later.
Bug: T214908
Change-Id: Ie0c3c059222700169f2605c3123554c74d974256
The definition of "self-link" in this context is an <a href="…">
element that points to the exact same URL as the current document's
location, excluding a possibly different #… fragment. This is typically
the case when the <a> element does not contain a full URL, but something
like href="#fragment". JavaScript's HTMLAnchorElement.href getter
automatically expands this to be a full URL.
Example:
var a = document.createElement( 'A' );
a.href = '#test';
console.log( a.href );
Notes:
* This new code assumes the wgPageName setting properly reflects the
page name requested via the current document's location. Core does
give us this guarantee.
* The only URL element that is intentionally not compared is the
protocol.
* This accidentially fixes T215899 as well, because the namespace check
is now bypassed for self-links (as it should).
Bug: T214861
Bug: T215899
Change-Id: I2670331cbbdebf7dc9fc70d7342724534f9f54ec
This patch also removes misplaced empty lines at the beginning of a
scope. In PHP code we even have a sniff for these. In JavaScript we
don't, but I suggest to be consistent about this.
Change-Id: Ic104ae8fe176da1dafa9bc783402adecb71de1f0
According to the standard.
Note: In a previous patch I removed the <?xml …?> line. This conflicts
with what https://www.mediawiki.org/wiki/Manual:Coding_conventions/SVG
suggests. However, I think the removal is ok in this particular case
because this file will never be shown as an image, and never be shown
standalone. Instead, it will be inserted in the documents DOM. The XML
header doesn't matter anyway then.
Change-Id: If23ad54985abb30f8c92500546bd04eeca44fab3
I decided to keep the comments because they are sooo helpful, but
tried to shorten them a bit. The biggest change is the indention with
tabs and the much more compact <path> elements. The shapes are the
exact same. I manually confirmed this for all four.
Change-Id: I2d1294c9ae7e398dcbe2d111c42848d17be8a67e
I tested this with all 16 possible combinations:
* The pointer can show up in all 4 corners.
* The popup can contain a thumbnail or not. The code for the pointer
is very different then, because the SVG masks are only relevant in
this scenario.
* The thumbnail can be tall or not.
* I even tested tall popups without a thumbnail. This is a combination
that is impossible in production scenarios.
I found 3 issues. This patch fixes 2 of them:
* The pointer is misplaced in the bottom-right corner when the popup
does not have a thumbnail (as reported in T215194).
* The pointer is misplaced in the upper-right corner when the popup
shows a thumbnail.
* The pointer in the upper-right corner is gray instead of white when
the popup is tall, but does not show a thumbnail. As this is not
relevant in production, I did not fixed it.
It seems both misplacements are because of the same reason: For some
reason, calculations are done based on the assumption the popup would
be 300px wide, but it is 320px wide.
Note I did not just added 20 everywhere, but manually alligned the
pointer triangles so they are placed exactly the same distance from
the corner as in the three other corners.
Note I did not tested this (yet) in RTL scenarios.
Bug: T215194
Change-Id: If0ca63d4d4b6e8083c7de1517fe32f49671a40e6
This is now possible since the render functions return jQuery objects.
All this code is exclusively used in the pagePreview.js file, and
doesn't need to make the already very big renderer.js file even
bigger.
Note the tests for all renderers have always been collected in a
single file. That's why the test case does not move.
Change-Id: I0c24638751c5f0e93d2bc0f3f4bb61fa0cf50d15
This just adds a simple test if passing the id of the clicked
reference source footnote works.
Bug: T213905
Change-Id: Ifc6549aa0203f19a5b24fa854b0aaf0cfb25674d
To make sure that we enable the link highlighting in the Cite extension we want
to trigger the click handler on the original footnote link. This is done by
passing the id of the source element to the model and the renderer.
Bug: T213905
Change-Id: I0bd59ac326269f3c0850946851fb79b611dc2a57
As discussed, the $( '<a>', { id: 'foo' } ) syntax is bogus because
plugins are able to *change* it. It's not just a list of attributes,
but whenever there is a method with the same name, that method will be
called instead. This means the result of this feature is unpredictable.
This patch also streamlines a few other jQuery calls that can be
shortened.
Bug: T214970
Change-Id: Ib58b8673c7ce41139f926c845c1b3adfbfde1b26
This solves the (I believe) only regression we introduced: A bad fake
reference like <span class="reference">[[Other article#Section]]</span>
showed a page preview for the "Other article" before we introduced
reference previews, but would have shown nothing after I9ec57e0.
Checking if the link is a self-link solves this and possibly more related
issues. Only self-links can point to a footnote on the same page.
Manually created fake-references like
<span class="reference">[[#Section]]</span> still have a chance to show
nothing in case the manually created HTML does not strictly follow the
expectations in the gateway. There is not much we can do about this. We
should not accept any arbitrary HTML but need to make at least *some*
assumptions.
Bug: T214970
Change-Id: I86e91bf45c3ae4c6a4086f7f1c7b1280fd400d17
I do find these very confusing and would like to remove them:
* The test setup looks like these popup types are going to use
these properties. But they don't. They are not even trying to
access these properties.
* There are no assertions that make sure these properties are
*not* used. It would be possible to add something like this,
but I honestly think this is not worth it.
We might need to reflect this in the PreviewModel documentation
in src/preview/model.js. I would like to do this in a separate
patch.
Bug: T214970
Change-Id: I136112bfea7f732d2673bcb8c69aba9defe6ba85
This tests the newly introduced code that decides if page or reference
previews should be used in the handling of a dwel event.
Bug: T214971
Change-Id: Ib20d00b7b9ee9b1ed82763137ec62e468e8f05f9
This is as preparation to introduce a gateway switch that decides if the
page or reference gatway should be selected. Moving that code to it's own
realm makes that path better testable.
Bug: T214971
Change-Id: I5efa9fb8f63f1487c627eb9a3f1fe47f43c611cc
This installs a series of safety nets:
* The selector [href*="#"] skips links without a fragment.
* It's still possible that a fragment exists, but is empty.
mwTitle.getFragment() checks this.
* The gateway does not assume the element exists, but checks this first.
If there is no such element, the gateway aborts the request in a way
that no error popup is shown. This is currently only possible with the
`{ textStatus: 'abort', xhr: { readyState: 0 } }` response as seen in
this patch. We might need to introduce a new, more clean way to silently
quit a fetchPreviewForTitle() call.
* The test for the reference gateway finally covers the scraping code.
Bug: T214970
Bug: T214971
Change-Id: I9ec57e0fbb0d21beaaa7b359c1c2bef64d2c14f5
Including tests for all situations.
I believe it is impossible or extremely hard to actually abuse any of
these places. All these data are not extracted from the current page, but
delivered either by MediaWiki's api.php or a RESTful endpoint, as
configured via $wgPopupsGateway and $wgPopupsRestGatewayEndpoint. A
possible attacker would need to write it's own endpoint (which must either
run on the same server or somehow ignore the CSRF token), and set the
value of mw.config.values.wgPopupsRestGatewayEndpoint on the client to
this endpoint – which requires just *another* attack vector to be able to
do this.
It's "the right thing"(tm) to escape all this anyway.
I found two possibly relevant security reviews of this extension, T88171
and T129177, resolved in 2015 and 2016.
Bug: T88171
Bug: T129177
Bug: T214754
Bug: T214971
Change-Id: I1d118c9ccaea434a253a772d18139b9b077118ab
Instead of maintaining a list of named constants (which must be updated
every time we want to add a new test with a new message), the mock now
behaves like MediaWiki's build-in qqx dummy language code and returns
the message key in brackets. The additional benefit of using the
HTML-like <…> characters is that this will automatically test if the
messages are properly HTML escaped.
Bug: T214970
Change-Id: Id7911036a7b582aff21acf911a826b5421a55938
This will affect all links, including [[Other page#Fragment]] for
example. But it will not have much of an effect there. The mw.Title
class is able to understand strings like "Other page#Fragment". All
old code calls title.getPrefixedDb() on the result. This will *not*
include the fragment. Only the new code will use title.getFragment().
I made sure this does not affect regular page previews, even when the
link is something like [[Other page#Fragment]].
Bug: T213415
Change-Id: I15611a44aa0477cc5e48ee4b12aae3cd981d977c
I guess both is fine: either having the default in the gateway (as it
was before), or in the renderer (as this patch proposes). I, personally,
feel better with having it closer to where it is needed. This way it's
not possible to accidentially deliver a model object with an empty title.
The renderer will catch this.
At the moment we don't know exactly how we will fetch other titles (e.g.
"Book").
This change is split from I15611a4 where it was a little misplaced.
It also includes a test for the default fallback title.
Bug: T213907
Change-Id: I8ec3ddc21a417da7f95feff7b080cbd60d5472e7
Including tests. I also changed the title to include quotes as well,
even if not critical in that case.
Bug: T214754
Change-Id: I2f92a5714f7adc229a003f9167bcc9afdbc55583
This is documented at http://usejsdoc.org/tags-type.html, but not in many
other places, especially not in the JSDuck documentation.
The {!…} syntax means "can not be null". This is the default anyway.
The {?…} syntax means nullable. In a few situation is was used when a
parameter can be undefined. I decided to remove it everywhere and replace
it with {…|null} when appropriate, because this is much more explicit. Less
syntax to remember.
Note I'm intentionally not using the […] syntax when a parameter is followed
by non-optional parameters. Actually skipping a parameter in such a situation
would mess the parameter order up. Having optional parameters not at the end
is sometimes used as a feature in JavaScript code, but not in this codebase,
as far as I can see.
Change-Id: Ie370cfe08c32d1af5b0341951bed044fc3511c57
I finally found the issue. It was an incomplete mock for the
mw.html.escape() function that would return the string unescaped.
Bug: T213415
Bug: T213908
Change-Id: I198393b3c72771e4018f79913ddb9f4cb2c0d4de
Excluding tests for the renderer which keeps failing. This will be
readded in a later patch.
Bug: T213415
Bug: T213908
Change-Id: If79fa3d0a7a20f121b1ceda6e0e33ad691b1ad30
It's not exclusively about page summaries any more.
We had a few suggestions in mind:
* get, fetch, request, or issueRequest. But I feel these are all to
generic and don't describe well what the method does. As a reminder:
It expects a Title object and returns a promise, which returns a
PreviewModel object, which contains an HTML "extract".
* fetchPreview? I feel this can still mean to many things.
* fetchPreviewModel? But we don't really need to repeat that it will
return a model object.
So I went for fetchPreviewForTitle. What do you think?
Bug: T213415
Change-Id: Icb32c63cec82f72453dc1507c9f8b8d461fd4f4c
This is split from the current draft patch Ie0ccb03. This is part of a
series of very small patches that prepare the code for new types of popups.
We decided to not add code for other types of popups to the existing
createGateway() function, but introduce new files and functions instead.
Renaming, for example, the existing `gateway` variable name will make it
much more obvious which of the future gateways does what.
Bug: T213415
Change-Id: Ifcbc3ba53d0ab9ef67adf1f314defc76b4f89e89
