• 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
We already request an idle callback when loading the code in the init
module.
Doing so here seems redundant.
Bug: T191089
Change-Id: If132c3331c49a4e74be70a5486a8270a8ce380bd
...by using a delegate event handler on the document instead of waiting
in the hook for the content div.
Bug: T191089
Change-Id: I88baa12a9aad9ed5e6c1288b39089843c19cec6c
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
This moves the footer icon into the ResourceLoaderImage module
providing us a consistent way of serving image assets.
This means we no longer need to provide PNGs for icons
However, given mw-ui-icon-large is not large enough for the given
use case we do have to wrestle with icon styles and override them
to get the desired result. I think this is a small price to pay given
icons are now discoverable
Change-Id: I38b62c01fd930dcbfb73b95e6128885cb483f86e
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
Comment became a bit stale. This updates it to reflect reality and
normalizes the verb forms for consistency.
Noticed as part of https://gerrit.wikimedia.org/r/c/420839/2/src/index.js#167
Additional changes:
* Merged all the const statements in L63 into one, for consistency.
Before, some were merged and some were independent.
Change-Id: I23aa824bb811f03a3630b4695e84c468bd9cd8b5
In I7395e3438836149becdd576942bdaf6f21b4163f the settings templates
were rewritten so that they no longer displayed an image.
descriptionText and images were dropped from the template but not from
the template provider. These are artifacts from relating to that patch
and are no longer used.
Change-Id: I1be7ef288d37f338e83dab3cf041e628a06608d2
Use i18n close label in the settings dialog since we have it instead of
hardcoding English. A cross is shown instead of the text so this change
may not be visible in all browsers.
Change-Id: I66284b04fe905cc8e460ea10b56c88cacd66ed28
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
Instead load it via mw.loader.using
We retain the module name ext.popups as this will be present
in cached HTML, however now it will load the bulk of the code
inside ext.popups.main
Bug: T176211
Change-Id: Ibe212721807d3698dc45ef46b2dbde15ca9d2f70
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
These are now taken care of by the Mobile-Content-Service's
summary endpoint and no longer needed here!
They are actually breaking certain previews so time for these
to go!
Bug: T183833
Change-Id: Icd2a21127c2f5881943564eca4df6bed3c15e223
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
- Removing the javascript positioning of the settings dialog.
- Placing the settings dialog inside the settings overlay.
- Using flexbox to center the settings dialog.
Bug: T157072
Change-Id: If8d929fe019a04ed5f96aa593779841a52f58eff
In the mediawiki gateway fetch uses mw.Api which when calling ajax
returns a promise (not a deferred).
Thus .promise() here is unnecessary and happens to work because of
jQuery promises but it is not a standard method on JS promises so it
shouldn't be used on promises, only on deferreds.
Change-Id: Iec609b90bffad8b99b3908897dfb72d7c4ed5481
Functional changes
- Show the default / error preview for all extract request failures
except those due to network circumstances (such as CORS) or no
connectivity (offline). Previously, the error preview was displayed
only for missing pages.
- FETCH_COMPLETE was previously only dispatched after FETCH_END. Now
it's also dispatched after FETCH_FAILED. The additional "fetch
complete" is not expected to impact logging. The states of success
are: START, END, COMPLETE. The new failure states are consistent with
success: START, FAILED, COMPLETE.
Testing
Errors may be stimulated in a number of ways including:
- Timeout: add a timeout field to RESTBaseGateway /
MediaWikiGateway.fetch().
http://api.jquery.com/jquery.ajax/
- Bad request: change MediaWikiGateway.fetch's action field to
`Math.random() > 0.5 ? 'query' : 'fail'` and RESTBaseGateway.fetch's
url field to
`RESTBASE_ENDPOINT + ( Math.random() > 0.5 ? encodeURIComponent( title ) : '%%%' )`.
- Desired Gateway can be configured in Gateway#createGateway().
- Note: T184534 describes a circumstance where cached previews may not
appear when offline. Disable browser caching to avoid confusion.
Bug: T183151
Bug: T184534
Change-Id: I7332284da0e0fb1ecd234a6f1e146ebd9ad8d81f
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
Event timestamp (as reported by `window.performance.now()`) is also sent
along with every action.
Bug: T180036
Change-Id: Ie3a648298005d51b8b4fbaa6a53e78caf50fa2d3
When bundled by ResourceLoader this can interfere with debugging
of other concatenated files.
The nomin instruction introduced in 7bd29bb058
was meant to avoid RL minifying an already
uglified bundle, but that shouldn't matter here.
Bug: T177344
Change-Id: I90829668544e7c4ff7ddfdbb90d91b88a27a69f4
.catch is only available with jQuery 3, which is only now starting to
roll out (see https://phabricator.wikimedia.org/T124742)
This patch rolls back the usage of .catch introduced in
https://gerrit.wikimedia.org/r/#/c/373327/ to use .fail, but only the
source part, given that patch introduced unit tests for this behavior
Bug: T174724
Change-Id: I6e1c342d812b35846061266bc59e493e87423fd8
We cannot serve .map files from production servers. This makes
Popups extension difficult to debug. As a simplest solution we
decided to store map files as .json files so we can easily access
js maps files.
Changes:
- changed webpack config to store map files under .json extension
Bug: T173491
Change-Id: Iaa55f75a8c5f3e8f1f169b3ac33241cc54f0413f
Number.isNaN is a new function introduced in ECMAScript 6.
MediaWiki only requires ECMAScript 5 supports from browsers.
Notably, Opera 12 does not have Number.isNaN. Instead, use
the global isNaN function (which behaves the same except for
non-numeric inputs).
Change-Id: If436cd26b21ce0336dfbc37144f6226e7b948e5e
Instead of using the non-standard old Promise implementation by relying
on .fail, migrate it to .catch.
In this specific case, where we were chaining promises with $.when, the
semantics from going from fail to catch actually change. .fail keeps the
promise in a rejected state, while .catch will change it to resolved
unless another error is thrown.
As such, when changing it to .catch, the catched error will be re-thrown
to keep the promise in a rejected state, so that $.when.then is not
executed.
Additional changes:
* Make actions.js#fetch return its promise for ease of testing.
* Test FETCH_FAILED, which was fully untested.
Bug: T173819
Change-Id: Ibd380b714586979c6e60b4c969d17f36a0796b52
Introduce PopupsAnonsExperimentalGroupSize config variable. This defines
a population size who will be subject to experimentation. If the group
size is undefined or 0 (default) and PopupsBetaFeature is false
(default value) Popups will be enabled for everyone. If it is any other
value, half that group will see page previews.
Drop the config variable PopupsSchemaSamplingRate - we will now only
EventLog when an experiment is occuring. This means we can simplify the
MWEventLogger class as shouldLog will always be truthy. Given server
side eventlogging is only used for preference changes
traffic should be low and not need sampling.
Introduce getUserBucket which determines whether a user is in a bucket
on, off or control based on the value of
PopupsAnonsExperimentalGroupSize. Add tests showing how these
buckets are calculated.
Caution:
A kill switch wgPopupsEventLogging is provided for safety.
It defaults to false. Before merging, please check if any config changes
are necessary.
Bug: T171853
Change-Id: If2a0c5fceae78262c44cb522af38a925cc5919d3
There are some cases when the preview count stored in local storage
evaluates to NaN. When this happens we should override the value
to zero, store it in localStorage, and return it.
Bug: T168371
Change-Id: Ic44b7c7b5b716f6a0859f33278d56d2d95bbfb3e
We haven't seen the PP EventLogging instrumentation produce duplicate events
for weeks.
Bug: T172106
Change-Id: I6f3d7c0cdbf23161f63259e4d20d8a710376468b
