This was already accidentally the case for the main VE article
editor as it doesn't live inside #mw-content-text, however other
VE surfaces (e.g. DiscussionTools') don't live inside #mw-content-text.
Bug: T259889
Change-Id: I48bdee6f62af33cad7512128b5465d474a6dfebb
We found that Popups was overcounted approximately 2.5x relative to Cite.
This patch attempts to nearly match the circumstances under which Cite
(and its tracking) is loaded.
Bug: T214493
Change-Id: Ib31df3c33879f4ea63d9808ffd260861069a8977
The Popups pageviews were suspiciously high, about 2.5x higher than
Cite counts which should have reported the same number. This patch
attempts to deduplicate pageview events.
Bug: T214493
Change-Id: I51bf6d1909d65ecd9d3ef28eda285852897343ec
We want to know the side of this population in pageviews, in order to
produce proportional metrics later.
Bug: T214493
Change-Id: I940872870754e85d19688f3855d6857e65b982b1
We're getting a few dozen records per day at 1:10, so let's stop sampling.
We need enough data to get significance on events happening at 0.1% or less.
Bug: T214493
Change-Id: I8a913fe1ee1e5b72d84914e183ac2386ddb20d84
Notice how this actually reduces the size of the final, compiled index.js.
It's not much, but still.
One issue I noticed is that the coverage reports for the JS code stopped
working. I have no idea why.
Bug: T208951
Change-Id: I2fe92579574b3b1ba4d2dd064899eee944045a96
The signature mw.RegExp.escape() is deprecated since MediaWiki 1.34.
By the way, this is the 4th or even 5th time in my short career this
tiny, single line (!) helper function is moved around and I need to
update all my code. This felt already ridiculous when it happened the
2nd time.
Change-Id: I4d05a49120aff64ebc316d0af7736c62385d9307
We've logged zero events after 12hr of deployment, so it should be
safe to increase sampling by 100x.
Bug: T214493
Change-Id: Icd67aed448269f603dd4465f7e46eac9a64bd1ab
Logs events to the ReferencePreviewsPopups EventLogging schema, in
order to understand whether Reference Previews is helpful for
end-users.
This will be removed along with the older tracking, as soon as our analysis
phase is finished.
Incidentally disables a lint rule for the generated JS, it's about
readability so irrelevant to the minified code.
Bug: T214493
Change-Id: I2638611ba67b785338f7e98a1c4b08a5e829812d
The eventlogging `isEnabled` function determines when to sample,
this patch removes the `isAnon` conditional so that we can measure
reference reading habits for all users.
Also guards against a non-function navigator.sendBeacon, which was
previously intended but incorrectly tested.
Bug: T214493
Change-Id: I42cb3082fb85c7900426a2055dfa3c2f6ecfd968
I was able to track the issue down to the changes made in this patch:
https://gerrit.wikimedia.org/r/331563
This patch is mostly a simplification of ce8a2d4
(I9a73b3086fc8fb0edd897a347b5497d5362e20ef):
- Don't make wait#wait() abortable. This adds complexity and isn't
needed since the token is rechecked in the .then() of
actions#linkDwell(). The request is permitted to continue and fetch if
that token still matches and is never issued otherwise.
Once a request has been issued, that request is still abortable.
However, note that calling XHR.abort() is just a request to abort and
may not be granted. Whether or not XHR.catch() is invoked is what
dispatches the FETCH_ABORTED action (or doesn't if the request to
abort was denied).
- Remove the abort tests for wait#wait() since the functionality is no
longer provided.
- Pass the preview token in the FETCH_ABORTED action and reduce
FETCH_ABORTED the same way as ABANDON_COMPLETE in reducers/preview.
The follow-up patch, I3597b8025f7a12db0cf5d83cce5a77abace9bae3, adds
integration tests for the specific bug fix. Note that these Selenium
tests are incompatible with the content proxy, so it is probably best to
simply unset $wgPopupsRestGatewayEndpoint and $wgPopupsGateway and allow
the defaults to be used. Also note that the tests are incompatible with
recent versions of Node.js (so use NVM) and emit many deprecation
warnings (so set deprecationWarnings to false in
tests/selenium/wdio.conf.js) An example run of the tests looks something
like:
chromedriver --url-base=wd/hub --port=4444 &
# If any changes are made locally, also run `npm -s start &`.
MW_SERVER=http://localhost:8181 \
MEDIAWIKI_USER=foo \
MEDIAWIKI_PASSWORD=bar \
DISPLAY= \
npm -s run selenium-test
Live testing may be performed as well. Remember that RESTBase requests
are incompatible with MobileFrontend's content proxy hack so ensure to
comment it out if $wgPopupsGateway is configured for RESTBase (see
T218159).
1. Open the DevTools network tab.
2. Disable the browser cache. Chromium, at least, won't abort requests
coming form the cache.
3. Hover back and forth quickly over a preview. In Chromium, canceled
requests are labeled and appear red. This is a good scenario to test.
With the patch, a preview should always be shown when ultimately
resting on a link. Without the patch, it is possible to rest upon the
link with no preview showing. This may require several attempts.
Bug: T219434
Change-Id: I9da84b0296dd14e9ce69cb35f1ca475272fb249a
As mentioned in T205744, EventLogging schema ResourceLoader modules have
been deprecated. This removes those modules from loader calls.
Bug: T221281
Change-Id: I1b7355c69e09756f50ccd1c1955b45cae4a64b9e
Removing wrong ARIA `role=tooltip`. Those are meant only for static
text content on short `title` like dynamic tooltips.
Bug: T223827
Change-Id: Ib4f490c7ad421e516fb0cf47eff4335bcaf26c43
We did a strict === comparison before. This works fine when the page
zoom is either 100% or 200%. Other zoom factors produce fractional
numbers. Comparing them with === is doomed to fail because these numbers
are IEEE representations, not necessarily being identical in situations
where you would expect them to be identical.
This applies two fixes: Change the comparison to a >=, and always ignore
1 extra pixel. If we are so close to the bottom, the gradient is not
helpful any more.
Change-Id: Idf4c604142de8d2a803e1d94f3093cec8a8abb72
This fixes page previews appearing when there is self link to a page
that has a file extension in it's name.
Bug: T222869
Change-Id: I5caf610fa76986026948a5b7b55537723752b755
This is relevant in case the HTML looks like this:
<… class="reference"><a><span>[</span>2<span>]</span></a></…>
The additional <span> cause many additional mouseover and mouseout
events. The code already tries to filter these duplicate events that
are all triggered on the same link, but gets confused, especially when
the multiple chains of events overlap each other in unexpected ways.
It's a timing issue. This change does not fix the fundamental issue,
but does make it much less painfull.
This patch also removes the > from the selector. This is in case the
HTML looks like this:
<… class="reference"><span><a>[2]</a></span></…>
The additional inner <span> would prevent any reference preview from
being shown.
Bug: T214693
Change-Id: If2554ba78072245c27a1f85c46f33e3c58582c1d
Introducing the REFERENCE_CLICK action that will fetch and show the
preview for the clicked reference right away without any delay.
The main goal of the new chain of events introduced with the reference
click is showing the reference preview right away. The actions triggered
by the dwelling include delays in multiple parts of the process.
If there's a dwell action-chain in progress when the click action is
executed, the related promise ( that might still include steps with
delays ) and the reference preview is retrieved and shown right away
re-using the token.
In the case where there's no dwell action running ( e.g. when the click
was triggered via touch ) we create a new token and start from scratch.
In either case we want to avoid, that multiple clicks trigger multiple
actions and abort early when there's already a click action in progress.
Bug: T218765
Change-Id: I073a93be2d17a21178aebe12267765f60a2811b9
I actually tried to bring the animation for the "bottom" property back.
When finished, I realized this animation is done on *top* of the
horizontal scrollbar. This looks fancy, but is not what we want. We
need to keep animating the opacity.
This patch here contains a bit of refactoring that was left after I
went back animating the opacity.
Bug: T220200
Change-Id: Icf613f72f3baa3de86f8aa319667c8e8f16593fd
The changes in the patch check if a horizontal scrollbar is present
and move the absolute positioned fade overlay above that scrollbar.
Since we cannot change the CSS of the :after element via JS a new div
element was introduced.
Bug: T220200
Change-Id: Ia69c9be0facaf3ecebdb9f76d36f7cb3412c0816
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
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
We run into this issue already one time, see Ifb5fe89 (T214710). The
exact same applies here.
The effect of this bug is that for certain references (typically ones
with a colon character in their name) the "jump down" link will be
broken.
Change-Id: Ic6723bd910cb5e5e1e1872ce39f2e271012245de
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
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
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
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
As well as:
* Simplify the selectGatewayType() function a bit. Or was this
intentional?
* Remove a TODO we don't need any more, at least not in this file.
Change-Id: I5528f0012cbaf8b4e88e22c7e2a8d87bf027e8f1
We discovered a bunch of possible solutions (see previous patch sets),
including replacing the `$( document )` selector with a more specific
one. That idea does not pass the linter.
Very late I realized the original selector starts with
`#mw-content-text`. This heavily limits where popups are allowed to
appear: really only in the main text content area.
We should limit reference popups to the exact same scope.
This fixes the issue described in T215195. Before, the content of the
popup was covered by the selector. Reference links *inside* the popup
would trigger another popup, which makes the current popup disappear.
Now the popup itself is not covered by these event handlers any more.
Bug: T215195
Change-Id: I142aee68abbd57ca321873855fef9209e0db0bbf
There are a few cheap checks done before this regular expression is
even needed, most notably the check for a pretty URL (without query
parameters). Since the vast majority of links processed by this parser
are pretty, I believe this optimization is worth it.
Change-Id: I730b87dc010161e8bc3f311c517293c0ad553326
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
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
This CSS class is used by the regular wikitext content area, and used
by certain CSS selectors (e.g. the external web link arrow).
The DOM structure for each popup is (intentionally) created outside
of this scope, at the end of the current page's body. This works
great for page previews because the do *not* want to share any styles
with the rest of the page. But reference previews want to do this.
In this patch I also remove the inner <span>. It was misplaced (note
the name) and resulted in block elements nested in an inline element.
Bug: T214463
Change-Id: I740e37a2ed929edf971b348fbf20e5fb12012d37
This gets rid of a little bit of code duplication, and makes the
interfaces all conform to one standard again after I05ed4b8 left them
in a little inconsistent (but properly documented) state.
Bug: T214970
Change-Id: If8407c1a48aff1cb31fc2e74b3c2b846e79a3cb5
As discussed in Iaadcce9. This does have a few benefits:
* Less code in the already pretty big render.js file.
* The code setting the target attribute is much closer to where it
belongs: in the file that specifies how the content of a reference
popup should look and behave.
* The class name "mwe-popups-extract" is not mentioned in two different
files, but in the same.
Note this changes the signature of this src/ui/templates/… file to not
return an HTML string any more, but a jQuery object. The other templates
still return strings. I believe this is fine, and not that much of a
difference anyway. The signatures don't need to be identical. And the
jQuery object still represents the exact same HTML as before.
If it helps we could change all templates/… signatures accordingly.
Could be done in this or a separate patch.
Bug: T213908
Bug: T214970
Change-Id: I05ed4b886f79c5ae748f53ab9fed965dfd217620
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
This is mainly done to increase testabilty of this part. I am a bit
unsure if this should ( have been ) integrated in the former index.js
that's now the page.js. - See also the refactoring done before.
Bug: T214971
Change-Id: I90d0441510bc1ec0b4900a392afcbaff6a552377
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
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 tried hard to keep the CSS as small and robust as possible. The
icon will be align with the text by adding a negativ margin. With
that we also decided against using RTL and LTR specific icons that
are positioned at the edge of the canvas for now.
Bug: T213907
Change-Id: I98888114e1c50e249cf31e71749323bd4f69da3f
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
Special characters that have a meaning in one of the many different input
formats jQuery accepts must be escaped.
The real-world use-case are references like <ref name=":1"> with a colon.
But it's many more characters that need escaping. See
http://api.jquery.com/category/selectors/
Note this patch misses a test. I already uploaded I9ec57e0 to fix the
currently incomplete tests. But I can't make it work. How do I create an
element in the test environment so that jQuery finds it?
I suggest to merge this and continue working on the tests later, because
this is currently one of the most annoying issues that makes all testing
unreliable.
Bug: T214710
Change-Id: Ifb5fe896936078f799298ac803d019d9caa048c8
The need for this is more a sign for a broken specification than an
actual issue with this code. But better be sure than sorry. More
details at
https://mathiasbynens.github.io/rel-noopener/
Bug: T214776
Change-Id: Idbcfae6d146fbbe3bff730239329beeb3455e18c
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 adds support for preview popups on reference/footnotes from
the Cite extension. For that a new preview type was introduced and
integrated into the existing structures.
The essential starting points were this code comes into action are
added behind the feature flag introduced in the previous patches.
Bug: T213415
Change-Id: Ie0ccb03117bd654373d0f458b62cc52018361c67
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.
Bug: T213415
Change-Id: I4f4392057f6d3eff78409c8b6f49898c8be45d3e
This became an issue after the patch Ifa56d41 (part of T206323) removed
some hidden <span> and replaced them with title="…" tags. The CSS selector
in src/index.js get's active on all <a> elements that have a title="…"
attribute. This is now the case for all references that are used one time
and have their ↑ (or ^ on the English Wikipedia) linked.
I realized this is really only an issue for these ↑ links.
The more general issue described in T198652 still holds true, but becomes
less urgent with this bugfix.
Bug: T198652
Bug: T206323
Bug: T212419
Change-Id: I9287e8692d031f9d2ba50f967520bf327ed5c42f
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.
Bug: T213415
Change-Id: I00d46a716c0e6ada82ffc0034a7dd5582363c657
Copy learnings from MobileFrontend's Webpack configuration. If nothing
else, the files are more consistent and easier to diff. When the change
to rename source maps is excluded, the build products are identical.
The following changes were copied:
- DRY up the output directory and source map extension as variables. For
the latter, rename the source map from ".json" to ".map.json". Without
renaming, the build products are unchanged.
- Reduce verbosity. Only report warnings and errors.
- Fail to build when an error occurs.
- Update ordering and add comments for easier reasoning and diffing with
MobileFrontend.
Bug: T212527
Change-Id: Icf11dff91358ad021932aa209c65ed8aac77d12b
Due to loosely versioned dependencies, master's resources/dist is out of
date. This won't be an issue when package-lock is supported. Rebuild the
assets as a workaround.
Change-Id: I2bb8eab5b849616f5ae96a7915f8f1a9069109a6
Replace $.noop dependency with inline empty function.
The tests relied on a single function instance to pass. A toString()
comparison of the noop function cannot be used as they differ when
coverage is enabled.
Change-Id: I641801593beb240a8f7d06e388a0e41dc8a25bc6
redux | 3.6.0 | 4.0.1
redux-thunk | 2.2.0 | 2.3.0
Upgrading these two dependencies caused the build size to increase from
from 12.2 KB to 12.4 KB (gzip) and so the bundlesize was adjusted
accordingly in our package.json.
Additionally, our linters flagged two rules that were then turned off in
.eslintrc.es5.json.
Looks like the changes in Redux were mostly cosmetic with much work
dedicated to typescript definitiions and dropping support of private
imports [1].
The changes to redux-thunk only involved changing typescript typings and
should not affect us. [2]
The upgrade was tested locally and did not appear to break anything.
[1] https://github.com/reduxjs/redux/issues/1342#issue-130452197
[2] https://github.com/reduxjs/redux-thunk/releases/tag/v2.3.0
Bug: T209314
Change-Id: I35284793ca0c72914d9b9b2e7c28dd407bafd4d8
webpack | 4.1.1 | 4.27.1
webpack-cli | 2.0.12 | 3.1.2
The upgrade of webpack modified our build files and included these
side effects:
* Our linter flagged the usage of bitwise operators in the build files
so I turned that off in our build file lint config (.eslintrc.es5.json)
* Our build file's size increased slightly from 12.1 KB to 12.13 KB
(gzip) so the package.json's bundlesize was updated accordingly.
No deleterious effects were noticed to popups running locally, but the
jump in webpack version included these notable changes:
* Switch from uglify-es to terser minimizer [1]
[1] https://github.com/webpack/webpack/releases/tag/v4.26.0
The upgrade for webpack-cli was a major version jump, but the params
that we pass to it in our package.json file appear to work as before.
Bug: T209314
Change-Id: I1403f2c4d354cf54554a740f8c23176bf80fd3c6
FETCH_COMPLETE_TARGET_DELAY is used to introduce an artificial delay to
HTTP requests as needed. However, FETCH_START_DELAY is always accounted
for so it makes sense to define FETCH_COMPLETE_TARGET_DELAY with it. The
docs are updated to draw the distinction between total delay and API
response delay.
Change-Id: I4cddc89b8090d54db0dd85f270441cab17c54993
Currently the Popups schema is disabled but if we were to re-enable
it, the page tokens would now be consistent with the ReadingDepth
schema
The getToken function is kept to allow generation of the
pageInteractionToken's relating to a link interaction, for which
there is no centralised API.
Additional changes:
* I've updated various tests to use the central mw.user stub to make
clearer where tokens come from.
Bug: T203013
Change-Id: If746bea5aeed2b4c192a9b8a02feb1fe06480633
diffs now correctly output errors. This will pick up changes to
index.js and index.js.json that have not been committed
--exit-code is passed to ensure that if there are changes then an
error is thrown and the action is not allowed.
The dist folder became out of date as depending on what version
of node you run, the output is different.
To aid debugging, the script that checks the diff now outputs the
node version and the npm version. From now on, we will all have
to use the same node version to build assets.
This is annoying, but we will re-evaluate the approach we are taking
to build assets in T202743. We can easily work around this by all using
nvm and making consistent use of the same node version.
The assets have been rebuilt with the node version that CI uses.
Bug: T202748
Change-Id: I82aee879d4b04ca06447f95eb81230bfc24d20e9
jQuery.hidpi was deprecated by T127328
(https://gerrit.wikimedia.org/r/441614). This repo used the
"bracketedDevicePixelRatio" function from that plugin. Since browser
compatibility is good now for window.devicePixelRatio, this commit adds
a function which relies on that instead.
Bug: T198579
Change-Id: I56c234048d7741f12f35bfff5f7319c6e085c29f
This change made it impossible to open links in new tabs.
Reverting so we can try again.
This reverts commit ff5bfd1d04.
Bug: T200940
Change-Id: I10a387df8bdeb891f8d8be0eb9075f0d324646b6
Whe user moves mouse away and we abort the http request we shouldn't
count that request as a FETCH_FAILED. The reasoning behind is that
FETCH_FAILED state increments the counter.PagePreviewsApiFailure.
Our StatsD graph gets polluted with lots of aborted requests and it
becomes unsuable. It doesn't show only the failed requests.
Changes:
- introduced new state: FETCH_ABORTED
- switch to FETCH_ABORTED when browser aborts the request
Bug: T199482
Change-Id: I58047eb80f0700b78b2991daff9395ecc92553b8
Previous implementation did not pass the `result` variable
to the catch() statement. Because of that every execution that
ended with exception inside fetch() statement was threated as
not a network exception and tried to present the null preview.
Changes:
- properly handle data returned by rejected fetch promise
- chaged the big if (result && result....) into something easier
to read
- pass Error object instead of 'http' string
- Restbase can return exception, it doesn't have to handle the 404
errors by itself, it's already taken care in the catch() logic
- fixed unit tests to reflect new logic in restbase gateway
Bug: T199482
Change-Id: Ibb30fc58248623d9ad4c5388a5b2ff9b387e01de
Instead of mixing window.mediaWiki / mediaWiki and window.jQuery /
jQuery references, always refer to globals which exist whether code is
executed in browser or headless Node.js environments.
find src tests -iname \*.js|
xargs -rd\\n sed -ri 's%window.(mediaWiki|jQuery)%\1%gi'
Change-Id: I21d0a602dcbd2bc6774934bee6c487e443270fe0
Changes:
- added acceptLanguage as a config option passed to
both mwApi and restbaseApi, by default code will use
the language defined in `wgContentLanguage` config
variable. The `wgContentLanguage` is always defined
(see ResourceLoaderStartUpModule::getConfigSettings())
so there is no need for checking the variable existence.
The new logic was tested both on MediaWiki API and Restbase API
Bug: T198619
Change-Id: I1cb31f1999fd674a8b870b2b5effb92ed3dfaa1f
Whenever an HTTP request sequence is started, i.e. wait for the fetch
start time, issue a network request, and return the result, abort the
process if the results are known to no longer be needed. This occurs
when a user has dwelt upon one link and then abandoned it either during
the fetch start wait time or during the fetch network request itself.
This change is accomplished by preserving the pending promises in two
actions, LINK_DWELL and FETCH_START, and whenever the ABANDON_START
action is issued, it now aborts any previously pending XHR-like promise,
called a "AbortPromise" which is just a thenable with an abort() method.
There is a similar concept in Core:
ecc812f06e/resources/src/mediawiki.api/index.js.
Aborting pending requests has big implications for client and server
logging as requests are quickly canceled, especially on slower
connections. These differences can be observed on the network tab of
DevTools and the log in Redux DevTools.
Consider, for instance, the scenario of dwelling upon and quickly
abandoning a single link prior to this patch:
BOOT EVENT_LOGGED LINK_DWELL FETCH_START ABANDON_START FETCH_END STATSV_LOGGED ABANDON_END EVENT_LOGGED FETCH_COMPLETE
And after this patch when the fetch timer is canceled (prior to an
actual network request):
BOOT EVENT_LOGGED LINK_DWELL ABANDON_START ABANDON_END EVENT_LOGGED
In the above sequence, FETCH_* and STATSV_LOGGED actions never occur.
And after this patch when the network request itself is canceled:
BOOT EVENT_LOGGED LINK_DWELL FETCH_START ABANDON_START FETCH_FAILED STATSV_LOGGED FETCH_COMPLETE ABANDON_END EVENT_LOGGED
FETCH_FAILED occurs intentionally, STATSV_LOGGED and FETCH_COMPLETE
still happen even though the fetch didn't complete successfully, and
FETCH_END doesn't.
Additionally, since less data is transmitted, it's possible that the
timing and success rate of logging will improve on low bandwidth
connections.
Also, this patch tries to revise the JSDocs where possible to support
type checking and fix a call to the missing assert.fail() function in
changeListener.test.js.
Bug: T197700
Change-Id: I9a73b3086fc8fb0edd897a347b5497d5362e20ef
If a thumbnail is narrow, then the extract can expand to take
the available space. It does this via JavaScript taking the difference
between the normal space for a thumbnail minus the actual space needed
to display the thumbnail.
This removes unused whitespace in both the thumbnail and extract.
Bug: T192928
Change-Id: I59e87f9160e707fbce321a567c0a68e85f6d72ec
Prevents the source_url param in virtual page-views from getting
too long and causing an error because it exceeds varnish's max-url size.
Bug: T196904
Change-Id: Idf3667c4c2ad7e0436f013c70d5ff4ebea453d7a
Make it so the entire popup area is clickable.
Update the click handler to reflect the actual parameter
it receives (an Event not an Element) and do not pass it
in the action, given it is unusedt
Bug: T192773
Change-Id: If80969f4759b1675278d11caaf5cb093ce72031c
Since we use an SVG mask, we cannot use border-left to visually
separate the page preview thumbnail from the text. We can however
make use of a polyline and programatically work out it's start and
end.
Bug: T192928
Change-Id: I0f983a80e3210b2f7e9aa197d2a632680675973e
Prevents video files and other non-image files from being rendered as
popup thumbnails. Restricts thumbnail format to either jpg, png, or gif.
Bug: T193792
Change-Id: I7a9be5d1c8396c02ebf0893c960f65644acc9d99
- Adds an extra element in popups template to contain settings icon.
- Resizes the setting icon so that the hover/active state is a square.
- Updates the settings icon SVG file.
- Modifies margin rule for icons placed near top of popup.
Bug: T193058
Change-Id: Icc16a788bba8e2f0a82a27c2b5c7be6c2cccaa90
