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
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
As far as I can see this is not an integration test, because linkTitle.js
does not interact with the document (in contrast to footerLink.js, which
does).
Change-Id: I8b611263020fe597efb63d8a0080b996ffc7dde4
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
The following upgrade was made:
eslint-config-wikimedia | 0.8.1 | 0.9.0
The upgrade of eslint-config-wikimedia removes the need of
eslint-plugin-qunit as a peerDependency because it is now a hard
dependency [1] so it was removed in our package.json.
It appears the biggest change in the upgrade was the use of separate
profiles [2]. Given this, our root .eslintrc.json was updated to extend
from 'wikimedia/client'.
Several test files were flagged by the upgraded linter and were fixed in
this patch. Additionally, our build file was flagged for having too many
statements on one line. This rule was turned off in .eslintrc.es5.json
[1] https://github.com/wikimedia/eslint-config-wikimedia/blob/master/package.json#L48
[2] https://github.com/wikimedia/eslint-config-wikimedia/blob/master/CHANGELOG.md#090--2018-11-19
Bug: T209314
Change-Id: I29db72e77f04a327bc9c2b558c6d53849287bb80
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
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
Let's improve our documentation by linting it and ensuring it
is complete and matches guidelines
This fixes offenders
Change-Id: I7c829b375705e763085cf731e9a77cc14339af67
Although Popups only uses JSDocs at this time which seemingly doesn't
care about casing[1], we should endeavor to use the proper return types.
This patch lowercases typing to indicate primitive / boxed type as
appropriate.[2] As a special case, function types are uppercased for
compatibility with TypeScript type checking.
Lastly, JQuery types are of type "JQuery". The global JQuery object's
identifier is "jQuery". This patch uppercases J's where appropriate.
[0] https://github.com/jsdoc3/jsdoc/issues/1046#issuecomment-126477791
[1] find src tests -iname \*.js|
xargs -rd\\n sed -ri '
s%\{\s*([?!])?(number|string|boolean|null|undefined)%{\1\L\2%gi;
s%\{\s*([?!])?(function|object)%{\1\u\2%gi;
s%\{\s*([?!])?jquery%{\1JQuery%gi
'
Change-Id: I771bdbb69dc978796a331998c0657622ac39c449
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
Update the mw-node-qunit require to @wikimedia/mw-node-qunit. 2d150f0
missed this and it caused tests in CI to silently succeed.
Bug: T197251
Change-Id: I9de597b0e9afc747c47bddc6debcbe5b87bcd793
* Force arrow-parens
* Disable no-prototype-builtins for time being
* Drop unnecessary maxlen rule
Change-Id: Iceb0fe47354a5753202d2c6ad9e1a9c76791f744
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
Prevent outdated ESLint error waivers from littering the code by
enabling `--report-unused-disable-directives`.
Change-Id: I3b9c39131f030cf2c4113ecd947c3f4a8679bdfe
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
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
Via jscodeshift:
jscodeshift \
-t jscodeshift-recipes/src/qunit-assert-equal-to-strictEqual.js \
Popups/tests
Also, some very minor manual clean up.
https://github.com/niedzielski/jscodeshift-recipes/blob/5944e50/src/qunit-assert-equal-to-strictEqual.js
Additional change:
* Drop redundant clipPath parameter from createThumbnailElement - this
parameter does not exist in the function signature.
Change-Id: I209ecf2d54b6f5c17767aa2041d8f11cb368a9b5
Replace all test assertions for calledOnce / Twice with callCount.
assert.ok( calledOnce / Twice ) only lets the dev know that a test
fails. assert.strictEqual( callCount, EXPECTATION ) starts the debugging
process when it fails since it provides the difference in the failure
output. strictEqual() was deliberately used since it's a saner default
and the codebase already favors === equivalency checks.
find tests -name \*.js|
xargs -rd\\n sed -ri '
s%ok\(([^,]+)calledOnce%strictEqual(\1callCount, 1%g;
s%ok\(([^,]+)calledTwice%strictEqual(\1callCount, 2%g;
'
Change-Id: I07c3c6d20e07c5b8107583a01d820e3fbd68a4e1
No longer needed. We can't turn something off again for people
now they expect it to exist.
Clarified usage instructions of PopupsEventLogging to make sure
it's more scary given the implications
Bug: T173952
Change-Id: I7be005b79da498d8e7b7df8f18b60c1327636a2c
Popups is out of beta feature and this code is no longer needed.
Removing code is the happiest activity a developer can do.
Other changes:
* Remove redundant type field on extension.json
(If not set, the extension will default to the "other" section.)
* Repurpose `name` with `namemsg` and make use of existing i18n
messages
Bug: T193053
Change-Id: Iea832cd1f37b0e7df6ff95efd66e4a1ff2a9004e
I've yet to meet the bloke who knew how to take a poke without an
explanation such that they have never mispoke. This patch which renames
pokies to pointers will surely be my masterstroke.
find \
-not \( \( -name node_modules -o -name .git -o -name vendor -o -name dist -o -name package-lock.json \) -prune \) \
-type f|
xargs -rd\\n sed -ri 's%([Pp])(okey|okie)%\1ointer%g; s%([Pp])oke%\1oint%g'
Bug: T190831
Change-Id: I363e6dd49bfcdb9515cd5fab2904a58725b18720
Thumbnails are displayed as SVG image elements. The SVG itself has a
width 3px greater than necessary for landscape thumbnails specifically.
For left-to-right languages, this additional space is empty and
unnoticed. For right-to-left languages, this extra spaces shows as a gap
on one side of the thumbnail and exceeds the popup's bounds on the other
side.
This extra 3px appears to have been mistakenly applied to landscape
thumbnails when it is only applicable to portrait, for which it is
already accounted for. Remove the 3px slop.
Bug: T190831
Change-Id: I6096f416f7e102975c4753a6b093b192aa1b45d7
When the UI is RTL, show preview thumbnails on the left. Otherwise, show
them on the right.
Bug: T190831
Change-Id: Ic1fc54f6547b31908905db8cb2ec4d58f37a6538
• Instead of removing 'mwe-popups-no-image-tri' in
renderer#layoutPreview(), add more conditions to #getClasses().
The addition condition in getClasses() was:
( !hasThumbnail || isTall ) && !flippedY
The removal condition in layoutPreview() was:
flippedX && !flippedY && hasThumbnail && isTall
To combine them, the removal logic is inverted and the conjunction is
taken:
( ( !hasThumbnail || isTall ) && !flippedY ) &&
!( flippedX && !flippedY && hasThumbnail && isTall )
Push the negation inwards:
( !hasThumbnail && !flippedY || isTall && !flippedY ) &&
( !flippedX || flippedY || !hasThumbnail || !isTall )
Expand:
!hasThumbnail && !flippedY && !flippedX ||
!hasThumbnail && !flippedY && flippedY ||
!hasThumbnail && !flippedY && !hasThumbnail ||
!hasThumbnail && !flippedY && !isTall ||
isTall && !flippedY && !flippedX ||
isTall && !flippedY && flippedY ||
isTall && !flippedY && !hasThumbnail ||
isTall && !flippedY && !isTall
Eliminate always false conditions and combine redundancies:
!hasThumbnail && !flippedY && !flippedX ||
!hasThumbnail && !flippedY ||
!hasThumbnail && !flippedY && !isTall ||
isTall && !flippedY && !flippedX ||
isTall && !flippedY && !hasThumbnail
Further eliminate redundancies:
!hasThumbnail && !flippedY ||
isTall && !flippedY && !flippedX ||
isTall && !flippedY && !hasThumbnail
Factor:
!flippedY && (
!hasThumbnail || isTall && !flippedX || isTall && !hasThumbnail
)
Factor more:
!flippedY && (
!hasThumbnail || isTall && ( !flippedX || !hasThumbnail )
)
Eliminate last redundancies:
!flippedY && ( !hasThumbnail || isTall && !flippedX )
The getClasses() test is updated for the new logic.
• Move thumbnail clip path manipulation from renderer#layoutPreview() to
a new function, #setThumbnailClipPath(). The new function flips the
order of the series of if statements so that an if / else block can be
used instead which clarifies that clip-path state is exclusive. In
other words:
if ( a ) { foo.prop = 1; }
if ( b ) { foo.prop = 2; }
if ( c ) { foo.prop = 3; }
if ( d ) { foo.prop = 4; }
Can generically be refactored regardless of condition or value to:
if ( d ) { foo.prop = 4; }
else if ( c ) { foo.prop = 3; }
else if ( b ) { foo.prop = 2; }
else if ( a ) { foo.prop = 1; }
Because prop was originally overwritten which implies if / else-like
priority.
Additionally:
• The entire function call is wrapped in a hasThumbnail conditional
which previously was checked as an input in each case.
• Consolidate the last two conditions since they only differed by a
single boolean input.
• Move the setAttribute() action to the end of the function since the
conditionals just map condition to value and the action is now
identical.
• Revise pokey mask doc to use clip-path terminology. This inverts the
thinking about the mask but better matches usage.
Bug: T190831
Change-Id: Ib460c6c07fcb054f8d425d127c588bb28a1d2473
We change page_title and source_title as a last step
just before we send the event.
Doing it elsewhere is risky at the current time because:
* the non-canonical form is needed for
mwApiGateway to bold title (formatPlainTextExtract). Needing both
would require updates to the Page model.
* title is used by EventLogging (Schema:Popups)
Bug: T191471
Change-Id: I93e7343643dcd9f32a86459907eb0b7051df91aa
The clip-path SVG property was conditionally set in thumbnail.js and
also conditionally set or removed in renderer.js. This patch refactors
the logic to occur in a single place, renderer.js.
The refactor was made with the following considerations:
• The one condition under which thumbnail.js would set clip-path was,
given a thumbnail exists, the thumbnail was not tall and clip-path
would be set to `url(#mwe-popups-mask)`. Otherwise, thumbnail.js would
not set clip-path.
• The logic in renderer.js for setting the attribute doesn't change
since overwriting the clip-path is equivalent to not having a
preexisting value. The case for removing the attribute itself is
replaced by inverting the condition, `flippedY`, and combining it with
the thumbnail.js condition, `!isTall`. The operation is only valid for
an existing thumbnail so the `hasThumbnail` remains unchanged.
This patch also clarifies that the "flipped" classes are exclusively set
by using an if / else chain instead of reconsidering all inputs for each
condition.
Bug: T190831
Change-Id: I4062ec7068dcadecbdbc4791447ea2ed1ce2a1de
We are using EventLogging to track page views not user behaviour.
This is an exception to the rule and requires special handling.
Bug: T190188
Change-Id: If096ccaf0ac884d57744ed57e2f26b51446de2d7
Replace Mustache.js templates with template literals. An effort was made
to minimize additional refactoring, so feel free to ask for more but it
ain't coming in this PS.
Bug: T165036
Change-Id: I4a6a1d93a2922c3a9ef3ae93c47da17a35c644f0
- Add SVG Inline Loader for Webpack. This allows SVG files to be
imported.
- Update the Webpack and test configurations to use the new loader.
- Scope the ESLint rules down to just JavaScript files so that linting
isn't attempted on the SVG.
Bug: T165036
Change-Id: I00bccff4c3167975c19d577be6343dcaca7ddb2d
Creating a different page preview for disambiguation pages.
This patch:
- modifies the Preview model to accept a new 'type' property
- modifies the Restbase Gateway to pass the 'type' prop to the Preview model
- creates a new template to accept both generic/disambig previews
- modifies the renderer to render the new template
- generates icons for new template through resource loader
- adds new i18n strings
- modifies event-logging "preview seen" event to send new "disambiguation" previewType
- updates event logging schema version
- adds tests for Preview model and renderer for new preview type
- does way too much? yes, yes it does.
Bug: T168392
Change-Id: Idc936cc3eabbdd99a3d98f43c66b4cdbb7d24917
Allow developers to use different endpoints for summaries
= developer happiness
This is useful for the following use cases:
* A developer wants to test against a production endpoint via
CORS
* A developer has setup an API where REST is hosted elsewhere
e.g. http://localhost:6927/en.wikipedia.org/v1/
* A user wants to create their own REST summary compatible
endpoint
* A wiki e.g. wikidata wants to use a different endpoint which is compatible
with the summary endpoint.
We are unlikely to use it ourselves on Wikimedia wikis (the
default should suffice) but this will be a powerful tool for
When not configured this will continue to work as per normal
Change-Id: I8a7e12fbc43cddbac678e0d7b81d1e877b747b22
Stripping parentheticals were designed specifically for working around
issues with content inside wikimedia wikis and error prone.
This problem for wikimedia wikis is solved by the mobile content
service.
Given we have no intentions to use the MediaWiki API for summaries.
They are not necessarily useful to third parties and it makes little
sense to maintain them (a third party can configure their own API or
use their own REST endpoint if they really do need them).
Bug: T189042
Change-Id: I2729dc9f172af0afee1c6f0cd563c556b4ae0aeb
This change updates the schema and begins to log
additional information such as source_namespace, id
and title. This information is provided inside the
boot action.
Additional changes:
* Allow camel case in bundle artifact
Bug: T184793
Bug: T186728
Change-Id: I425ffecc018bef2958d0dfe957a40a065e3e6c56
Don't assume that thumbnail URLs contain a dimension delimiter of "px-".
Previously, thumbnail URLs always contained the width. e.g.:
https://upload.wikimedia.org/wikipedia/commons/a/aa/100px-Red_Giant_Earth_warm.jpg
However, thumbnail URLs that actually point to the original are not
sizable:
https://upload.wikimedia.org/wikipedia/commons/a/aa/Red_Giant_Earth_warm.jpg
These are provided, for example, when the thumbnail size requested is
larger than the original. There was code designed to handle this
scenario but it only applies when RESTBase and page preview thumbnail
sizes happen to be in sync. In other words, if RESTBase requests a large
thumbnail on behalf of page previews, and page previews only requested a
small thumbnail, the original may be unexpectedly provided. A
conditional is introduced in this patch to verify that "px-" is actually
detected. If it is not present, the original is used.
Bug: T187955
Change-Id: If4e29dd870aecd6d461cc8203f6576d1bb8844f2
Pageview is consistent with verbiage used by Research and Analytics
Engineering in their reports and documentation, e.g.
https://wikitech.wikimedia.org/wiki/Analytics/Pageviews.
Bug: T184793
Change-Id: I8ae085b4af85aa72f234f3db27f0cac2c4d014e5
* New action added PREVIEW_SEEN
* The action will be used to signal that a page view needs
to be recorded.
* PREVIEW_SEEN is a delayed action which is triggered
as a side-effect of the previewShow action. It is only dispatched
if the user is still previewing the same card and the page
related to the card has preview type `page`
* The pageview changelistener is added when
$wgPopupsVirtualPageViews is set to true.
* The page view changelistener listens for page views and logs
them using EventLogging when needed using
https://meta.wikimedia.org/wiki/Schema:VirtualPageView
Note:
* Currently if a user has enabled the DNT header, the
event will not be logged. There is ongoing discussion on the
ticket and fixing this will be addressed separately.
* Only title and referrer are logged in the initial version.
The task demands that "namespace" is logged but this information
is not provided by the summary endpoints we use so will need
to be added later (if indeed needed) either via a change to that
endpoint of by using JavaScript to parse the URL.
Bug: T184793
Change-Id: Id1fe34e4bdada3a41f0d888a753af366d4756590
This change fixes some issues with assertions not running, removes
unnecessary promise dances, and improves legibility and some code
patterns in the action and integration node tests mainly.
Detailed changes:
* actions.js
* Fully migrate out of jQuery 1 promises (no done/fail)
* Fix linkDwell action not returning the fetch action promise
* actions.test.js
* Simplify setupWait for the tests
* It always autoresolves immediately the wait call to ensure speedy tests
* No waitDeferreds or waitPromises array coordination, rely on action
returned promises instead
* Get rid of that = this in favor of arrow functions
* Rename generic "p" promises to meaningful names
* Add assert.expect for more solid tests (so that we don't skip assertions in
the future if we change them)
* Fix some assertions that weren't being run because of the incorrect promise
being returned (p.then, and then just returning p)
* Get rid of $.when stubbing in favor of waiting for the promise returned from
the action
* Get rid of hacky setTimeout(..., 0) to run assertions after the promises
* integration.test.js
* Get rid of wait(0) calls to hack around asynchronous actions
* Use the action returned promises instead of the waitPromises/Deferreds
* Remove unused "el" parameter being passed to this.abandon in several tests
* Remove unnecessary test helper this.abandonPreview (it was the same as
this.abandon)
* Clarify a bit the last and more complex test with some comments and variable
name changes
* Get rid of that=this in favor of arrow functions
* container.test.js
* Get rid of that=this in favor of arrow functions
* previewBehavior.test.js
* Get rid of that=this in favor of arrow functions
* Get rid of $.each in favor of .forEach
Bug: T170807
Change-Id: I06fafd92a1712f143018e2ddff24fadf1a6882b3
Returning promises from the `linkDwell` and `abandon` thunks and
removing some of the `wait` stubs in the unit test for these actions.
Also converting a fetch callback from a `.fail` to a Promise A+
compatible `.catch`.
Bug: T170807
Change-Id: I4bbf2863db090e222ba926d3bc36a99da4bdb601
Enforce it with eslint.
Ignore:
* Comment lines with eslint disable directives
* QUnit test lines as they contain long subjects (QUnit.* (only, test,
module, skip, etc)
* Strings, since long strings are used extensively in tests
* Ignore template literals for similar reasons
* Regex literals as they may be too long, but can't be easily
split in several lines
* Long urls
See bug for more general proposal for eslint-wikimedia-config.
Bug: T185295
Change-Id: I3aacaf46e61a4d96547c513073e179ef997deb09
Test subject was changed and stopped matching the implementation. In
this particular change the test (a bit convoluted but) tests that wait
is called appropriately, which is why the subject read "should delay
dispatching ..."
Change-Id: I3c8d9d8769f3d1c2869a267af105b9489df86cf5
