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
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
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
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
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
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
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
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
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
* Force arrow-parens
* Disable no-prototype-builtins for time being
* Drop unnecessary maxlen rule
Change-Id: Iceb0fe47354a5753202d2c6ad9e1a9c76791f744
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
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
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
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
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
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
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
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
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
Update the placeholder extract and button text shown when a page preview
is unavailable from:
"popups-preview-no-preview": "Looks like there isn't a preview for this page"
"popups-preview-footer-read": "Read"
To:
"popups-preview-no-preview": "There was an issue displaying this preview"
"popups-preview-footer-read": "Go to this page"
Bug: T183151
Change-Id: I0600dbc2e4d51a13675041d3c0741a793f4eae37
Functional changes:
- Require page URL when constructing a PreviewModel null object. These
models have valid titles and are used to display a preview when an
extract is unobtainable. When presented with an empty URL, their
linkage incorrectly pointed to the browser's current URL. Additional
tests were added to verify the fix.
- Check missing title in addition to falsy response in RESTBase gateway
and update the test assertion to check title. It isn't clear if this
can happen in the wild.
- Forbid state mutation in the conclusion of
MediaWikiGateway.getPageSummary() with a call to Deferred.promise().
This is consistent with the rest of repo including RESTBaseGateway.
http://api.jquery.com/deferred.promise/
Nonfunctional changes:
- Collapse two RESTBase gateway 404 tests into one as the scenarios and
expectations were very similar.
- Add failure HTTP status to 'MediaWiki API gateway handles API failure'
test stub HTTP response for consistency with other cases.
- Add nullity expectations to JSDocs touched and fix a couple typos
throughout.
- Make the gateway tests a little more consistent by collapsing Deferred
variable usage where appropriate.
This change is necessary to the completion of T183151 which uses the
PreviewModel null objects for additional error cases.
Bug: T183151
Change-Id: Ib77627fb9c80d8e806208bbafcfc615b130e3278
