• 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
We already use pixels to define font sizes in the pixels so rather
fight Vector's styles explicitly declare the font size in the header
(which it controls) instead for more readable code.
Also copy across several rules that are implied by Vector but needed
for the settings dialog so that this renders nicely in Monobook and
Minerva.
Change-Id: I196b8af23fe6ebcdb472c059cf1536d251a5b978
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
Enable the Babel transpiler so that ES6 template literals,
destructuring, and arrow functions can be used in production.
"last n versions" syntax was not used so that builds are more
reproducible.
Bug: T165036
Change-Id: I553b6d14cc368c7b4366f68d13038c3d505f5429
- 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
Specifically, clarify why the timestamp property is merged into the data
bag that represents the event when it is.
Change-Id: I184de734e66490fc728a4f9c2f84bf0765aeed08
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
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
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
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
Event timestamp (as reported by `window.performance.now()`) is also sent
along with every action.
Bug: T180036
Change-Id: Ie3a648298005d51b8b4fbaa6a53e78caf50fa2d3
Making use of standard close icon and add `opacity` transition
for slight user feedback on states.
Bug: T50067
Change-Id: I258a50452eba8f9d0bfb550c2ad1a90ef7b6dc1f
Aligning anonymous settings dialog appearance with dialogs
elsewhere by
- setting base `font-size` to 14px to conform with Vector elsewhere –
size is not inherited to the dialog as it's a direct child of `body`
- increasing contrast on descriptive paragraphs to conform with
WCAG level AA
- reducing inner `padding`s slighty to conform with 8px grid
- aligning `border-color` and `box-shadow` values
- replacing static value with LESS variable
Also removing stylelintrc override rule which has been only around
for the old `box-shadow` value.
Bug: T178607
Change-Id: I738e0be11f3d1c94ea03288e0dddc1b983a6c729
Aligning SVGs to WikimediaUI color palette. Also optimizing
by help of SVGO and aligning them to each other, among changes:
- removing unnecessary, unused raster images
- removing invisible, application specific metadata
- unifying identation
- bringing attributes in order
Bug: T178257
Change-Id: Ief0ce99568e6a92700f2c8e4eb5990b989402389
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
Setting text color to `#222` and border color to `#a2a9b1` as
everywhere else.
Also making use of more recent LESS functionality with multiple
arguments per mixin to remove unecessary duplication of code and
change static values to central LESS variables where applicable.
Change-Id: I394c7e7e1369ff38b7ea91c7faebe773bcb2948d
.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
Under some unknown circumstances getPreviewCountBucket() is called
with a value that is not a -1 or a natural number. When that happens
function returns 'undefined bucket' which causes eventLogging to
fail. I wasn't able to reproduce the issue, it might be specific
to browser/os. The safest way is to return 'unknown' for any other
case.
Bug: T168371
Change-Id: I374bb629762a86ac06a18e775d3c1a14682c9f55
Instead of registering global variables in a function, make it pure
return the external interface and set it to mw.popups in the
src/index.js entry point.
Explicitly comment on index.js what is being set and why.
Bug: T171287
Change-Id: I94d467bfa7fa6e56033dd254518ad50b5dde5bfc
Changes:
- introduced js module defining mw.popups object
- introduced isEnabled() method which checks the redux store to retrieve
isEnabled status
Bug: T171287
Change-Id: I523369831e2aa8a915ed1cb001b35d13b770f9da
Why: Because they are the approved standard by TC39 and Ecma for
JavaScript modules.
Changes:
* Wrap mw-node-qunit in run.js to register babel to transpile modules
for node v6
* Change all sources in src/ to use ES modules
* Change constants.js to be able to run without
jQuery.bracketedDevicePixelRatio given ES modules are hoisted to
the top by spec so we can't patch globals before importing it
* Change all tests in tests/node-qunit/ to use ES modules
* Drop usage of mock-require given ES modules are easy to stub with
sinon
Additional changes:
* Rename tests/node-qunit/renderer.js to renderer.test.js to follow
the convention of all the other files
* Make npm run test:node run only .test.js test files so that it
doesn't run the stubs.js or run.js file.
Bug: T171951
Change-Id: I17a0b76041d5e2fd18e2d54950d9d7c0db99a941
Opera 12.10 supported unprefixed animations, gradients, transforms &
transitions. See http://www.opera.com/docs/changelogs/unified/1210/
Removing support for Opera 12.0x versions.
Change-Id: I3476db173433c430f654e12ea1f17d2721410b83
Depends-on: Ie8edbcd7f85c713ea2156706ea3a3a7b423d8d9d
Previously, if the browser didn't support the Beacon API, then
instrumentation/eventLogging#isEnabled would bucket the user with a
sampling rate of 0, which is equivalent to returning false. This change
simply does the latter.
Additional changes:
* Update the documented module names of the instrumentation/eventLogging
and statsv modules.
Bug: T168847
Change-Id: I7ae5c10da42ca614b5b1a6619f9555e5665344cf
We had instrumentation for over 4 weeks and duplicate events rate
was very low. We want to keep stats so we check the duplicate events
rate but there is no need to filter those.
Bug: T167365
Change-Id: I72585beb21e9db589e45eeace657ef25f432abc9
Changes:
- introduced new event 'disabled', sent from settings popup
- added unit tests for 'disabled' event handling
Bug: T167365
Change-Id: I048b38122b8843199c86fd1ed9ec2ff21767e114
In order to debug the EventLogging instrumentation in production
environments, we want to be able to bucket ourselves at will.
When the debug flag (?debug=true) is passed send all events
for given page view.
Bug: T168847
Change-Id: Id1b13b0ecaa791b4f26be4d1151bdbbe5270b64d
Changes:
- set margin-top and margin-bottom to 0 on following elements:
ul, ol, li, dl, dd and dd
Bug: T168941
Change-Id: I80478de046d7944fde3c0de3f96f5c9dc4623c36
Changes:
- when event doesn't have linkInteractionToken do not check for
duplicated tokens
- hygiene, move event duplication logic into separate functions
for better readability
Bug: T168449
Change-Id: I3ae197567ec9f67e104af109d4f1a1c1a6769d32
Following on from I4f653bba, since the schema and statsvInstrumentation
modules are similar, let's group/rename them:
schema -> instrumentation/eventLogging
statsvInstrumentation -> instrumentation/statsv
Change-Id: Ic59e0da7d4917f6733fd090f15d3c269af863f05
Currently, the mw.eventLog.Schema class samples per pageview. However,
we expect that if a user is bucketed for a session, then all
EventLogging events logged during that session are in the sample.
Moreover, loading the class in the way that we did - asynchronously,
using mw.loader#using - introduced an issue where the eventLogging
change listener would subscribe in the next tick of the JavaScript VM's
event loop and miss the "pageLoaded" event being queued (see T167273).
Changes:
* Make the schema module follow the form of the statsvInstrumentation
module, i.e. make it expose the #isEnabled method, and add the
associated getEventLoggingTracker function.
* Update the eventLogging change listener accept the tracker returned by
getEventLoggingTracker.
* Update/fix related JSDoc documentation.
Bug: T167236
Bug: T167273
Change-Id: I4f653bbaf1bbc2c2f70327e338080e17cd3443d4
preview/model is just a module/namespace object, not a builder class or
similar.
This patch changes a couple of imports to reference some of the exposed
functions of the preview/model where required.
In a shiny future, this pattern would be:
const { createModel, createNullModel } =
require( '../preview/model' )
Bug: T165018
Change-Id: If6ad4611538ca4f24e2443c0c3ed433275e995a6
gateway/*/rest were copies of gateway/restProvider just passing
a different provider. Docs were the same, they were untested, and
looking at them they seemed like unnecessary abstraction.
This patch removes the plain vs html structure, and separates gateways
like before, by endpoint.
There is a light utility in gateway/restFormatters.js that adapts the
call from the rest gateway to use formatters.js functions. It needs
testing, that I'll add in the next patch.
The flow for creating a gateway ends up as follows:
1. index.js calls gateway/index#createGateway( mw.config )
2. createGateway chooses based on wgPopupsGateway and invokes
* mediawiki.js#createMediaWikiApiGateway or
* rest.js#createRESTBaseGateway w/ restFormatters.js#parsePlainTextResponse or
* rest.js#createRESTBaseGateway w/ restFormatters.js#parseHTMLResponse
Changes:
* Removed src/gateway/{plain,html}/rest.js
* Extracted formatter functions to src/gateway/restFormatters.js
* src/gateway/plain/mediawiki.js -> src/gateway/mediawiki.js
* tests/node-qunit/gateway/plain/mediawiki.test.js ->
tests/node-qunit/gateway/mediawiki.test.js
* gateway/restProvider{,.test}.js -> gateway/rest{,.test}.js
* Change gateway/index.js#createGateway to properly call the rest
gateways with the rest formatters
Bug: T165018
Change-Id: Ia75695dfc192aad5bc581a68882514bad6c29646
And add tests, given it is growing in complexity.
Additional changes:
* Interface ext.popups.Gateway -> Gateway in docs
Bug: T165018
Change-Id: I8a12333ad9d14d6a7fbde11afc42f607881e8ea3
... from the statsvInstrumentation module so that the bucketing logic
can be shared with other instrumentation modules.
Change-Id: I5732fa539a3911939fa85fa88c102fa8dcfa5613
Previews with tall image now have 9 lines of text as opposed to
the previous 7 lines.
Bug: T165978
Change-Id: Ib3d39f5164663f9c9ccaa3c0a974d524fc2f9463
Refactor existing Restbase gateway and extract shared logic into
shared Restbase provider. Also introduced new createNullModel()
which defines an empty preview model.
Additionally improve naming in new gateways/formatter so function
names are more explicity.
* Htmlize() became formatPlainTextExtract() as it should be used
only with plain text extracts
* removeEllipsis() became removeTrailingEllipsis() as it removes
only trailing ellipsis.
* src/gateway/index.js defines gateways by configuration name stored
in extension.json
Bug: T165018
Change-Id: Ibe54dddfc1080e94814d1562d41e85cb6b43bfc1
Depends-On: I4f42c61b155a37c5dd42bc40034583865abd5d7a
Follows-up 79f3b318d0.
Number#toString supports up to Base 32.
Same collision behaviour but with a shorter string.
Typical length with Base 32: 7 (max: 11)
Typical length with Base 16: 9 (max: 14)
Change-Id: I91e91341cbecdec24549ace6a6300550f5b449ee
This reduces the number of 301 Redirect responses when fetching previews
from RESTBase.
Bug: T167633
Change-Id: I830947ab79e72dcc023193412c8d5bcee986e23f
Page Previews should be able to consume HTML response generated by
MediaWiki. First we need to move out plain text crunching from
renderer.js and model.js. Mediawiki and Restbase gateways will have
to parse/htmlize plaintext into nice HTML by themselves.
Bug: T165018
Change-Id: I5d7e9f610bb809aa9fb035a4a9f96e9e8796c9d8
...instead of 1 event per link
Supporting changes:
* Delegate events on the container when booting up
* Check eligibility of title on event triggered
* Pass the title from the event handlers into the actions instead of
storing it in the dom
* Add title.fromElement as sugar over isvalid(gettitle())
* Not tested as it is sugar over the other 2 functions
* Fix action tests and integration tests
processLinks to be removed next
Bug: T165572
Change-Id: I4d9837706dc77ec64121ac94410c0d2da83692e4
... and update the RESTBASE_PROFILE constant to the latest "stable"
profile for the endpoint.
Prior to this change the Accept header sent by the rest gateway was
application/json; charset=utf-8profile="..."
This was discovered while responding to T166605.
Change-Id: I00f277e724c561634b26c9ab10bd35332c6dba91
When there's an interaction, then the "tapped settings cog" event should
have the same properties set as the other interaction-specific events.
This was discovered while QAing T164256.
Bug: T164256
Change-Id: I4749b52656203c7e0c42ae742556ee996eee322a
This ensures all files belong to a module.
When generating documentation no globals should be present
Bug: T158236
Change-Id: I134f38620fe46db11ba94dbede739f4336e0482c
The benefit of this is if there are any problems with the
documentation they will not enter our codebase.
We do a similar thing in MobileFrontend
Bug: T158236
Change-Id: I30329dd868fe596c490f95354c3226c9cd4a2fc7
... and the previewType property as well.
Per the Popups schema [0], the "opened" action should have the
perceivedWait and previewType properties set.
Bug: T166323
Change-Id: I957d123434a6b750aff6f5279865321a08367382
Changes:
* Assign exports to exports rather than reassigning module.exports so
that JSDoc can guess inner members.
* Tidy up parameter types with JSDoc's "nullable" syntax.
Bug: T158236
Change-Id: I7261d1bb3924c9f14301490f54d7813dcffc959b
Given that interactions end up with an event logged, there shouldn't be
a reason to keep an interaction active after it's corresponding final
event has been logged. See Tbayer's state graph.
This patch removes the current interaction if an event with that token
is logged, effectively finalizing it and making it impossible for the
token to be reused from the state tree again.
Additional changes:
* Pass the logged event with the action EVENT_LOGGED so that the reducer
can determine if it needs to do anything else.
* Since the interaction is removed, when undefined, guard against
actions that use state.interaction freely. (Only allow BOOT,
LINK_PREVIEW, and EVENT_LOGGED)
Bug: T161769
Bug: T163198
Change-Id: I99fd5716dc17da32929b6e8ae4aa164f9d84c387
This fixes a bug in I8a63d82, where the pageTitleHover and
namespaceIdHover properties of EventLogging events weren't being set.
Bug: T164256
Change-Id: Ie2c2d253f6508b89d48129fd17a902e5ded7cad5
Action changes:
* Include the namespace ID in LINK_DWELL.
Reducer changes:
* Add the createEvent helper, which adds the pageTitleHover and
namespaceIdHover properties in the event.
* Create "dismissed", "dwelledButAbandoned", and "opened" events using
the createEvent helper.
Additional changes:
* Store the target page's associated mw.Title using jQuery.
* Update the eventLogging reducer's test cases so that all LINK_DWELL
representations include title and namespaceID attributes.
* Create the createStubTitle factory function, which returns a minimal
usable mw.Title stub, and use it in the actions and integration test
cases.
Bug: T164256
Change-Id: I8a63d82a65324680dff9176020a8ea97695428c4
It enables certain optimizations, see:
* https://webpack.js.org/guides/production-build/
Code is minified by uglifyjs.
Aditional changes:
* Add banner so that sources are not minified by ResourceLoader
Bug: T160061
Change-Id: I50c9148ebf2d860db42a24225bc128bfcfe56927
I6a38a261 made the eventLogging change listener count and discard
duplicate events and count duplicate tokens.
While we isolate the issue(s) that lead to duplication (reuse, likely)
of tokens, make the eventLogging change listener discard events with
duplicate tokens as well.
Bug: T161769
Bug: T163198
Change-Id: I0dbb16c37814d39d7aec35c8fb7cc7309704c550
Binding the behavior has been left out as it requires some refactoring.
Ideally, that functionality should be tested via integration tests, which
is already done.
Bug: T133022
Change-Id: If2fd472847eb3557de97c7ec9619e8831e9bda6d
Per the Popups schema [0], the "dismissed" action expects the
perceivedWait property to be set.
Happily, the time until the preview was shown is already recorded and
used to determine whether or not to create a "dismissed" or
"dwelledButAbandoned" event.
Reducer changes:
* When creating the "dismissed" event, set the perceivedWait property to
the already accumulated timeToPreviewShow.
[0] https://meta.wikimedia.org/wiki/Schema:Popups
Bug: T164256
Change-Id: I3fd253f1c2ff5fa8e24c9225060d728ffd8dfd27
Since Ieea378c9 all QUnit tests are run in Node.js and not in the
browser. Tidy up references to QUnit inside of the codebase and tooling.
Changes:
* Don't exclude src/processLinks.js in Istanbul code coverage reports.
* Don't test for window.QUnit in createSchema. We no longer run the risk
of sending beacons while running the QUnit test suite as it's no
longer run in the browser.
Bug: T160406
Change-Id: Ifb6adb84b8019dd69231b50af00bf978b708fc60
The tests were removed in Iae0a78d0b8a13353de70794b67387f2c3bab44c6.
The ultimate goal is to refactor the renderer code and make it
testable, but before doing so we need to add tests to cover the existing
code. This will give us confidence that we won’t accidentally break
anything when we refactor.
Some of the tests have been removed as the functionality covered by
those were moved to model.js in I20f29657fcf94101a71ed13c0920508db71292ce.
Bug: T133022
Change-Id: I7b20324dd5fe8a428cdd96959b65bc82d44fb515
The ultimate goal is to refactor the renderer code and make it
testable, but before doing so we need to add tests to cover the existing
code. This will give us confidence that we won’t accidentally break
anything when we refactor.
Bug: T133022
Change-Id: I3ecbfb9bb3ac9c63fdd40df502796748c62949fe
The tests were removed in Iae0a78d0b8a13353de70794b67387f2c3bab44c6.
The ultimate goal is to refactor the renderer code and make it
testable, but before doing so we need to add tests to cover the existing
code. This will give us confidence that we won’t accidentally break
anything when we refactor.
Bug: T133022
Change-Id: I897276a1a953f6be62e4c2d4a24e0f22fc6ef141
With the final goal to remove the real mw stubs, move the processLinks
tests away from test cases, and split getTitle to be tested standalone.
Supporting changes:
* Move getTitle out to it's own module
* Will be tested separately in a followup commit
* Remove global declaration of mw.popups.processLinks (not needed any more)
Bug: T160406
Change-Id: Ieebd1257a2476081c67a318d3f05dffa1d3b9bdd
The eventLogging change listener is responsible for ensuring that the
internal state of Page Previews matches its external state (that
perceived by the user and UA). It does this by logging events with
ext.eventLog.Schema#log. This makes it the perfect place to track and
discard duplicate events enqueued by the Page Previews codebase observed
in T161769.
Make the change listener track events that it's logged by storing hashes
of the dynamic parts of them in memory. If the eventLogging change
listener sees the same event more than once, then it discards it and
increments a counter in StatsD.
This behaviour should be enabled for a matter of days as we should see
whether the duplicate events are being enqueued by the Page Previews
codebase immediately.
Bug: T163198
Change-Id: I6a38a2619d777a76dd45eb7300079e1f07b07b12
The statsv change listener depended on both the analytics tracking
function and whether it should log metrics to StatsD. We can simplify
the behaviour of the change listener by passing in a function which
doesn't log metrics to StatsD if such logging is disabled.
The change listener is now more isolated from other components.
Moreover, sharing the analytics tracking function with other components
is simpler as there's no repeated code.
Bug: T163198
Change-Id: Ibf4785fa4c27c1ad4739f02410f57412f56ff481
... by running the following:
svgo --disable removeXMLProcInst --pretty --folder images/
svgo --disable removeXMLProcInst --pretty --folder resources/ext.popups.images/
Also, add an XML declaration to resources/ext.popups.images/close.svg so
that it's recognised as an SVG by libmagic. This isn't strictly an issue
because of the way the SVG is served by the ResourceLoaderImageModule RL
module but it's consistent with the other SVGs.
Change-Id: I10b2286d6577701ba3b9a8651d5165fa81b8d293
If the user dwells on a link for long enough, then the gateway makes a
request, which is allowed to complete regardless of whether or not the
response is required.
If the user abandons the link but the request completes before the
abandon completes - currently, ABANDON_END_DELAY is 300 ms - then the
preview will be rendered temporarily.
Fix this by not rendering the preview if the user has started to abandon
the link.
Bug: T163350
Change-Id: I154dde4e3ccaed3d11cb023c85c44451fc0ad957
* Merge mwe-popups-icon with mwe-popups-settings-icon
* Remove PNG - now generated by the ResourceLoader module
* Adjust popup footer paddings/widths and store them in variables
* RTL and LTR compatible
Bug: T133956
Change-Id: I14ccd7b6731e9ec49f9959411fd17f7c9fdf43be
When the user dwells on a link and there's enough room to display a
preview above it, then the preview should rest atop the link rather than
above the mouse.
Bug: T161366
Change-Id: Ia7266f6e5c272817581bdbcb3710429b266556e4
If the user CmdOrCtrl+Clicks on a link, then the link will remain
focussed. If a preview is shown, clicks another element on the page, and
then there'll be no token to include in ABANDON_* with.
Bug: T162924
Change-Id: Ie2237aa55ea9a11070498b66c73b8bf1898d8d44
I09d8776 introduced a bug in the eventLogging reducer where reducing
LINK_CLICK would remove any accumulated interaction state but not close
the interaction.
Update the associated tests so that they correctly test the reduction of
this action.
Bug: T162924
Change-Id: Ia03e719c228ee96f279c1fa89252dc6b6371a8e9
... the interaction.
Following on from Iccba3c4c, LINK_CLICK shouldn't be considered the end
of an interaction because the link or preview can still be abandoned by
the user. Unlike ABANDON_END and LINK_DWELL, therefore, LINK_CLICK
musn't destroy the interaction but indicate that no more events should
be enqueued for the interaction.
More concretely, if the user has clicked the link or the preview, then
a "dwelledButAbandoned" or "dismissed" event shouldn't be logged.
Changes:
* Distinguish between "finalizing" and "closing" an interaction, where
the latter is the current behavior of ABANDON_END, LINK_DWELL, and
LINK_CLICK, in the eventLogging reducer and associated tests.
* If the interaction is finalized, then either the "dwelledButAbandoned"
or "dismissed" events shouldn't be logged.
Bug: T162924
Change-Id: I09d8776da992053f89a77508e29a7cde3cfeeac6
As noted in T160406, the only QUnit tests that requires a running
MediaWiki instance is the test for mw.popups.processLinks. The function
itself is isolated from the rest of the codebase.
Now, as noted in T162876#3182198, during boot the
ext.eventLogging.Schema module is loaded asynchronously with
mw.loader.using. Since boot is unconditional and happens ASAP this
happens when the tests are loaded and run.
In the short term this can be avoided by not making the tests depend on
the entire codebase. The long term solution is laid out in T160406.
Supporting changes:
* Bundle assets with webpack@2.4.1.
Bug: T162876
Change-Id: If1ee1853ba7a9b2a66b24bb93b4e6062b92b0dba
... in the eventLogging reducer.
Like ABANDON_END, the LINK_CLICK should be considered the end of the
interaction in the context of the EventLogging instrumentation, i.e. no
events should be logged after the user clicks the preview or the link.
See T159490#3172692 for additional context.
Bug: T159490
Change-Id: Iccba3c4c2b6121016ff7923c11b1622bc046ad6b
Like the FETCH_COMPLETE and ABANDON_END actions, the PREVIEW_SHOW action
was delayed but not conditionally reduced. As ABANDON_END is delayed,
there's a potential for a race and if ABANDON_END is reduced before
PREVIEW_SHOW, then there's no interaction to reduce the action into,
which causes an error, e.g. T159490#3165276 and T162373. Making
PREVIEW_SHOW require a token stops the error occurring in this scenario.
An alternative would be to clear the timeout created in
ext.popups.Preview#show in #hide. However, this would be inconsistent
with actions#fetch and actions#abandon.
Bug: T159490
Change-Id: Ibd2c0c6f45e4392582cc6ed08517f6ca1146d57a
Mirroring all other actions that are dispatched after some delay, add
the token to the PREVIEW_SHOW action.
Supporting changes:
* Pass the token to ext.popups.Preview#show so that it can be passed to
actions#previewShow.
Bug: T159490
Change-Id: I128fd56e770ed09d5d0dc55db73d11b013049c79
... instead of using the active element.
In the case of the eventLogging reducer, this fixes scenario 4.1 from
T159490#3150331, which was caused by late FETCH_COMPLETE actions being
reduced regardless of whether interaction had been finalised or a new
interaction had started.
Bug: T159490
Change-Id: If9d718625b0302ea2f75a778005643b4eef62bde
Treat these responses not as an API failure. Show a generic
preview whenever the server responds with a 404.
Bug: T160744
Change-Id: Id6169d9d4c7493f5b6511cc78fe65d448cdadc03
I6d9ff52b introduced a regression where if a logged out user clicks the
settings cog then an error is thrown.
For now, passing the event to the behavior for further processing is
required. However, it's clear that this makes the
ext.Popups.PreviewBehavior abstraction leaky.
Bug: T162324
Change-Id: I9dea04eb7435f9349e60d477f5701ec5dd655ebd
We have to be careful about the namespaces here, and then we don't
need the awful `.html( .html() )` hack. (I honestly have no idea
why that even worked for some browsers, it really shouldn't have.
The comment next to it is wrong.)
* Construct the 'svg:svg' element with the right namespace
* Set 'xlink:href' attribute on 'svg:image' element with the right namespace
Doing this correctly makes the thumbnails work in Opera 12, and it also
works as before in (at least) Chromium 57, Firefox 53, IE 11 and Edge.
I can't find out what version of Safari the other hack here was
supposed to apply to, but the code was wrong in both cases, and the
hack was mistakenly also applied to modern Chromium.
Useful resources for dealing with SVG embedded in HTML while scripting:
* http://stackoverflow.com/questions/6701705/programmatically-creating-an-svg-image-element-with-javascript
* http://jsfiddle.net/UVFBj/8/
* https://www.w3.org/Graphics/SVG/WG/wiki/Href#xlink:href
Bug: T161799
Change-Id: I30b2a1291811296424018e013bd07055ae7551d7
Require that two promises are resolved (or one is rejected) before the
FETCH_COMPLETE action is dispatched. The first promise represents the
gateway request and the second represents an arbitrarily long delay. If
the first resolves before the second, then there'll be a delay until the
second resolves; whereas if the first rejects, then there's no delay.
Change-Id: I496fe317337745c593594efff26688c46d661bf3
Mixing in the delay was introduced in If3f1a06f so that the total RTT
for an API request could be calculated. Now that the FETCH_END action is
dispatched when the gateway request ends and not when the preview model
is resolved, this additional information (state) is redundant.
Change-Id: I7e6ffe0945ffedd9425525fa7da855e729d50b77
Ideally, the preview model is resolved after 500 ms, regardless of
whether the internal gateway takes 100 or 300 ms. Given this, there's an
important distinction to be made between the "fetch" ending and it
completing and their associated actions.
Changes:
* Dispatch the FETCH_COMPLETE action when the preview model is resolved.
* Update the reducers accordingly.
Change-Id: I62c9cb0430284b76338ea80bd170cac5af4be9d0
If the user has disabled PP via the settings dialog or they aren't in
the experimental condition, then link titles shouldn't be emptied.
Because this behavior has to respond to the user enabling/disabling PP
within the same page session, change the linkTitle change listener
rather than conditionally registering it.
Bug: T161277
Change-Id: I53c1a1d3e4436e2ffe08da27da388f394f4e8817
Binding the behavior to the preview before it's shown means that the
application will respond to user interactions with the preview even
though it's transparent.
This fixes scenario 4 from T159490.
Bug: T159490
Change-Id: Ia2d06869868d07af60bdeb49d46612a4a0dc02e9
If the user abandons link A (or preview A) and immediately dwells on
link B, then log a "dismissed" or "dwelledButAbandoned" event.
In this context, "immediately" means before the ABANDON_END action is
dispatched, which, currently, is 300 ms after the ABANDON_START action
is dispatched.
Bug: T159490
Change-Id: I49f0f5dfb3e6c08844f1794fee8cb6170e93981b
Reducer changes:
* Add tests for ABANDON_END case.
* Extract the body of the ABANDON_END case into the createAbandonEvent
helper function.
Additional changes:
* totalInteractionProperty -> totalInteractionTime elsewhere in the same
file.
Bug: T159490
Change-Id: Ifff34271395f330b83cfe487e84800fe2d6f3811
Reducer changes:
* Make the eventLogging reducer queue a "tapped settings cog" event when
reducing the SETTINGS_SHOW action.
This was discovered while testing I6ce7d72b.
Bug: T159490
Change-Id: I6ce7d72b364d20c71b0e2cfed98e99f7997895e5
Like dwelling and abandoning, clicking on a preview is the same as
clicking on a link.
This fixes scenario 3 from T159490.
Bug: T159490
Change-Id: I6d9ff52b62bec93ebfcc9b6d267a46cf961852fb
For now, mirror the interaction modelling in the preview reducer in the
eventLogging reducer to handle the user either:
* Repeatedly dwelling on and abandoning a link.
* (Repeatedly) moving their mouse between the link and the preview.
This fixes scenarios 1, 2, 5, and the general issue from T159490.
Bug: T159490
Change-Id: Ia771f325e541c107348b16b47c5b786c97847652
Step 1 of T161284. Given that the median API response time (as measured
by the client) is ~115 ms [0] and the API response is artificially
delayed so that the preview starts fading in at 500 ms, we can increase
the API request delay to 150 ms without affecting the current UX while
decreasing the number of incidental HTTP requests triggered by the user
glancing their mouse over a link to another page.
[0] https://grafana.wikimedia.org/dashboard/db/reading-web-page-previews
Bug: T161284
Change-Id: I4c4a766467cdb4cd47c4231c1106c35bab67855e
When EventLogging is unavailable do not initialise the EL-related code
or try to send any events.
When EL is enabled for a brand new user we request an additional module
during boot causing an additional HTTP request. Page Previews continues
to boot normally regardless of whether the request fails.
This approach doesn't impact boot or first paint time. Once the module
is loaded once it should be cached locally, subject to the
ResourceLoader's policy. Moreover, the RL will not attempt to load the
module twice so this doesn't impact the performance of other modules.
Bug: T158999
Change-Id: I7ed7f00d52279151ece23e5aced4f2adb0f7fdc3
... container.
I19e67ae4 hide the overflowing parts of the SVG image element in IE9-11
for a number of cases but not all of them, e.g. see T139297#3089714.
Moving the overflow: hidden property to the SVG element fixes the above
case and is clearer.
Tested in IE9-11 on Windows 7, and Chrome (56.0.2924.87) on macOS Sierra
(10.12.3).
Bug: T139297
Change-Id: I9c397d7333766b40abbf14b6ade96788f5023dfa
Changes:
- remove focus events listeners as they are triggered after switching tabs
- show PagePreview on keyup event
Bug: T158631
Change-Id: I7533f896604e0e0a8ea6e900ae4f7d12b6458836
IE doesn't appear to update/redraw the SVG image element when Setting
its clip-path attribute to '', not removing it. This is problematic as
the attribute is always set to a default value (in the createThumbnail
function) before the preview is laid out.
Bug: T160237
Change-Id: I4559ff5018b8f4ecf06f6f5d9462a999d9726b94
For logging to work:
1. $wgWMEStatsdBaseUri needs to point to a valid statsv endpoint,
e.g. 'https://en.wikipedia.org/beacon/statsv'.
2. $wgPopupsStatsvSamplingRate needs to be set. Note that the codebase
already contains the EventLogging functionality, which is configured
separately. Separately configuring different logging mechanisms
allows us to avoid sampling mistakes that may arise while choosing
one or the other. For example, let's say we want to use EventLogging for
10% of users and statsv for 5%. We'd sample all users into two
buckets: 50/50. And then we'd have to set the sampling rates as
20% and 10% respectively, only because of the bucketing above. To avoid
this kind of complications, separate sampling rates are used for each
logging mechanism. This, of course, may result in situations where a
session is logged via both EventLogging and statsv.
3. The WikimediaEvents extension needs to be installed. The extension
adds the `ext.wikimediaEvents` module to the output page. The
logging functionality is delegated to this module.
Notable changes:
* The FETCH_START and FETCH_END actions are converted to a timed action.
* The experiments stub used in tests has been extracted to the stubs
file.
Logged data is visualized at
https://grafana.wikimedia.org/dashboard/db/reading-web-page-previews
Bug: T157111
Change-Id: If3f1a06f1f623e8e625b6c30a48b7f5aa9de24db
... thumbnails.
A good example of the difference in behaviour of the PageImages API is
here <T156800#3087507>. The API defers to File#transform, which scales
the largest dimension of an image, not always the width, e.g. if an
image is 1000px x 2000px and the request is for a thumbnail "of 1800px",
then the thumbnail will be 900px x 1800px.
Bug: T156800
Change-Id: I64bc2244ee78a594298d8637233b0da1083700eb
Keep all configuration-like values in one file.
Changes:
- moved EXTRACT_LENGTH to constants.js file
Change-Id: Ibe5ecfc60f2c09a30a9ecb3586bc5fb6a7365476
Webpack 1.14.0 is an old version, switch to using latest stable which
has better documentation, tree shaking, ES2015 modules and a core team
of contributors with funding. See https://webpack.js.org/
Additional changes:
* Recompile the frontend assets
Change-Id: I2c5940276e99dee104d04c6a0b83d8ab36a99df5
If the image isn't an SVG then it shouldn't be scaled past its original
dimensions. Handle the case where the requested thumbnail can't be
generated on the server as the original is too small ( < 320px,
currently [0]) in the same way.
Moreover, if the image happens to have the exact dimensions that we're
requesting (300px or 600px wide, currently [1]), then use the original
image to avoid unnecessary work/pressure on caches.
Supporting changes:
* Update the SVG_RESTBASE_RESPONSE fixture to use the extension returned
by RESTBase (and the PageImages extension) for the thumbnail:
.svg.png.
[0] https://github.com/wikimedia/restbase/blob/master/v1/summary.yaml#L121
[1] https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/constants.js#L2
Bug: T156800
Change-Id: I5d0aa161e80869e4b4f5425d906d1e753047a3a3
Instead of importing the modules from sources (which you should do if
you properly define NODE_ENV and use uglifyjs from webpack) import the
already compiled files on the npm packages.
This results in 5kb less on the compiled bundle.
Change-Id: I83732ea79a59b611c117ddcf4c262948c795f642
Following on from I19e67ae4, IE9-11's treatment of SVG elements that
overflow their containers means that the truncating pseudo-element and
the settings cog is occluded in a portrait-mode preview.
"Pad" the extract horizontally using a margin to force the SVG element
into the correct position.
Bug: T156800
Bug: T139297
Change-Id: I0da6af6d4cbcc69c6465b37714856e59199ae6e4
Unlike modern browsers, IE9-11 (at least) don't hide the overflowing
parts of an SVG image element. Make this explicit by adding the
overflow: hidden property to the preview's container element.
Bug: T156800
Bug: T139297
Change-Id: I19e67ae4584d90c02dc5a2dd1c8bdb5773cd2283
Now that most unit tests are run in node with common.js for loading
sources there is no reason to keep global variables around exposing all
the sources.
Only exception is the only qunit integration test processLinks.test.js
which still consumes mw.popups.processLinks, which is the only global
variable remaining in the codebase.
Changes:
* Remove references to mw.popups in code comments and reference the JS
file instead
* Remove popups.js which exposes all common.js modules as global
variables
* Export mw.popups.processLinks in processLinks.js for testing in
processLinks.test.js
Change-Id: I91066654b9282f73a80eb1ba5018bd091656c61d
We used to query the MediaWiki API to only return non-free images.
This patch allows us to query the API for images with any license.
The RESTBase end point is already returning images with any license.
Bug: T158632
Change-Id: I9ac60b6f74a7f7eb2cb160ee522c2c3a26dd0858
All the other masks use an 8px offset for the triangle. But this
specific mask used 250 - 243 = 7px offset.
Bug: T153840
Change-Id: Ib72842d18bd844ff37509cf5bf1dedd4e0f99dbc
Rather than manipulating the URL of the original image to get the URL of
the appropriately sized thumbnail, manipulate the URL of the thumbnail
image.
While we could manipulate either the thumbnail or original image URL,
there are subtle differences between the two, so manipulating the latter
makes the generateThumbnailData function as simple as possible, e.g. we
don't have to splice in "/thumb" after "/commons".
Also, ensure that the thumbnail's dimensions are scaled appropriately.
Bug: T156800
Change-Id: I6825bad14b1131dc81f23dcf120cf8ffb7d7b4f6
* Logged in users bypass bucketing. They keep working as before.
* When Page Previews is configured as a beta feature, logged out users
won't see the feature.
* If an anonymous user has enabled/disabled the feature using
the settings cog then they will see or not see the feature
depending on the value of their setting.
* The other anonymous users are bucketed. By default 90% of these
users see the feature, the other 10% don't. These numbers can be
controlled by the config variable `wgPopupsAnonsEnabledSamplingRate`.
Bug: T157700
Change-Id: I5307587b10f4849c4e82d3b064ff759121c2de67
mw.storage#get doesn't take a default value to return if the underlying
storage is disabled or the key is missing. In the former case it'll
return false and in the latter it'll return null, i.e. in neither case
will it return undefined.
Bug: T157700
Change-Id: I3f653c11468e17b64765e85ebb3b8f03e311352a
Because of the globals mw.popups.wait usage and mocking in both actions
and integration, they need to be migrated in a single step, fixing them
both to require wait.js and mock using mock-require instead of the
global variable.
Additional changes:
* Fix FIXMEs about actions.js using the global mw.popups.wait instead of
the require one.
* Fix the unit tests to use require mocking for wait.js instead of
global variable mocking in both integration and actions tests
* Change tests that use deferreds and promises to be async qunit tests
(Deferreds are asynchronous with jQuery in node, apparently they
weren't in the browser)
* Change integration.test.js to use require on Redux and ReduxThunk
Change-Id: I8e3e87b158bd11c9620e77d0a73e611cf9e82183
The "checkin" part of the Popups schema was superseded by the
ReadingDepth schema, the implementation of which is tracked by T155639.
As well as removing all checkin-related code, update the Popups schema
to the latest version - the version that doesn't have the checkin
property.
Bug: T155639
Depends-On: I762ec3fc91decf3cffa869dbd783faf62f01329a
Change-Id: If764917b6e121e1f9db980a4efa30c0f7a166197
CSSJanus v1.1.3 doesn't appear to flip the "to right" argument for the
linear-gradient CSS function. As with the position of the truncating
element in I0d50a8b5, the direction of the gradient doesn't vary with
the text direction of the current page but that of the target page.
Bug: T158858
Change-Id: I4b6fcf68bdf57722348513f12c7b19f80b2545c4
The render change listener is hard coupled to the renderer file, so in
order to migrate the test, instead of stubbing a global variable, we had
to either inject the renderer into the change listener factory as done
everywhere else, or mock the require call.
In order to do one thing per commit, we're mocking the require call
here to get the migration done, but added a FIXME to use dependency
injection instead in a future change.
Change-Id: I50f82cdc9664d34b8a8ccc1ff368f7209404159d
In index.js. Instead of using the global variable/object popups, require
things from their files so that we can remove the global variables when
we can run qunit tests with commonjs in node.
Change-Id: I85408f01eca27f97cf46b2076176fcc16c037829
This change resizes thumbnails to the appropriate width
based on the value of mw.popups.gateway.THUMBNAIL_SIZE
Tests cover
* When requested thumbnail is < than original size
* When requested thumbnail is > than original size
* When requested thumbnail is an svg and originalimage
smaller than requested thumb size
Bug: T156800
Change-Id: Ib375b97e2bc959e91de5177efc3df1f2ded54a5b
This way, src contains sources, and dist contains distribution files.
Also, add some documentation about the folders in the README and an adr.
Change-Id: Ie0b9f6475b8423b90e927633d883bde3cd5d5e4d
In order to automatically verify in CI that the built assets are up to
date with the commited sources, we need to keep the built assets in
a folder separate from the RL assets.
* Rename the compiled assets folder to industry standard `dist`
Change-Id: I8c5898f9bb29fee7164a7038b835a5f7efd33dbc
So that production code can be debugged properly. Independent source
maps files aren't loaded until the developer tools are open.
Change-Id: Ic8c3c10315d3a3be0f42505834005a3cab77d130
Until this appears in core it makes sense to package it up as
part of ext.popups resource loader module.
Version numbers from npm are pinned to exact versions to control the
upgrade path of the libraries instead of leaving it to semver on
developers machines.
Bug: T156333
Change-Id: I33368ecc3c8e911d96f846669bcd831c182749f2
boot.js is renamed to index.js and popups is now bundled
up inside popups.js
To support qunit tests, the library is still exposed as
mw.popups with a FIXME to remove later when it is no longer
necessary
Bug: T156333
Change-Id: Ieb6b4b0344af2701f99ef0fcc786d2378fd2fceb
Generate changeListeners via webpack
We now use a build folder to build the JavaScript for
our ResourceLoader modules. This is the first change
in a line of changes.
A source map is provided for debug support.
Bug: T156333
Change-Id: I771843d1ddb4b50adedc3fa53b30c2f1d8a76acb
Sometimes we make choices on the users behalf if we don't have
sufficient information, so this name is more applicable.
If beta features is disabled and the user is anon they have not
explicitly opted in.
Change-Id: I5d816f569fc54f8bf74d6e5a06246b7fa7036e06
* Create new namespace mw.popups.gateway to contain them
* Create folder resources/ext.popups/gateway/
* Split gateway{,.test}.js into gateway/{mediawiki,rest}{.test,}.js
* Extract stateless functions out of factory scope now that there are no
name collisions between the factories.
Bug: T123445
Change-Id: Ib256871c3e4cfe3f13361cb66d4e9a67e9823c7b
Instead of using constructor functions, use factory functions to
generate the gateway objects, because of:
* Consistency on the approach in the repository, no constructor
functions are used, the factory function pattern prevales.
* Real private data with closures
* No use of `this`, which is dynamic and unbound in JS and a source of
errors. In contrast, by using a factory function, the
function/methods-of-the-object are tightly bound to the internal data.
Additional changes:
* Specify more the type of createRESTBaseGateway's parameter to improve
clarity on the intent of the parameter
* From: @param {jQuery} api
* To: @param {Function} ajax function from jQuery for example
Supporting documentation:
* https://medium.com/javascript-scene/javascript-factory-functions-vs-constructor-functions-vs-classes-2f22ceddf33e
Change-Id: Iacbb098b646843a01f459b15343057e2e4851d35
Prepare to shift to a JS bundler by placing definitions at bottom
of page (these are currently assignments to mw.popups). In future
these are easily switched to module.exports line
Change-Id: Iaa5d78fee1cdb6fc572d2f1781f1d4fb59475e84
* Introduce the new config variable `PopupsAPIUseRESTBase`.
* Create a gateway interface that exposes the getPageSummary
method. Both MediaWiki API gateway and RESTBase gateway
implement this interface.
Bug: T123445
Change-Id: I71a8a848f3143fa4a0dfd4ca182ee086903110bc
Extract the getBaseData function from the eventLogging reducer so that
changes in the shape of the base data will be isolated to that function.
Additional changes:
* Clarify that the eventLogging reducer is specific to the Popups
schema. Add a TODO for revisiting the name, at least.
Bug: T156893
Change-Id: I99c102d3a1dbf77d526e5ab33e99929f351f5768
The existence of these files was causing me much confusion as I thought
they were still being used in the mpga branch.
If this is note the case they should be removed to make the repo easier
to understand.
Changes:
* Move the ext.popups.animation.less into ext.popups since that's where
its shipped.
* Remove files which are not being used.
* Since csslint now runs on ext.popups.animation.less fix the linting
issues (previously linting was not occuring)
Change-Id: Iae0a78d0b8a13353de70794b67387f2c3bab44c6
In I6bbff6a5 a preview was assigned the "extract" type if it had an
extract whereas it should've been assigned the "page" type.
Bug: T152004
Change-Id: I5e0cdc8f528647f1e20873753be29c6b85b3a384
Note this is a breaking change in the sense that it breaks the function
signature for createModel which no longer needs to be passed the last
modified time. Given this change is in the mpga branch at current time
and hasnt been exposed for public use I have deemed this okay.
The change removes tests relating to the last modified bar, removes
lines in the templates and updates existing tests and code to reflect
the new function signature for create model.
Settings icon is floated to right which will be flipped for RTL
users.
Bug: T137775
Change-Id: I7737e37d956c62f1f1c0694d7a25a58d91651f4d
* Load the close icon (previously was not loaded)
* Take opportunity to simplify selectors in setting panel and remove
unnecessary id selectors
* Correct size of header to be consistent with mediawiki ui icon
Note: there's a slight movement in the heading text's position
presumably due to inconsistiencies between the text
inside the Done/Save buttons
Bug: T154645
Change-Id: Icd978b45051c43f24ee1c9a8574a961b942082f1
The preview model is included in the FETCH_END action so, when reducing
that action, pluck the preview type from the model and add it to the
interaction state.
Change-Id: I0e51a85b4dada8f9a17ec4882e0c13b2c4382d24
Per T152004, if the preview has an empty extract, then it's a "generic"
preview; it's an "extract" preview otherwise.
Bug: T152004
Change-Id: I6bbff6a507fe3e4f03b3a44f64d875e7dbb512df
Following on from I20f29657, simplify the gateway by making it return
instances ofext.popups.PreviewModel.
Supporting changes:
* data -> model in mw.popups.renderer#render and the
create{,Empty}Preview helper functions.
Bug: T152004
Change-Id: I531f609056ae813ae5e6b1d53010d77be0799ec1
As noted in the review of I71a8a84, creating a new object to represent
the thumbnail is confusing. It's also somewhat wasteful.
Since both the MediaWiki API and RESTBase both agree on using "source"
as the key for the thumbnail's URL, it's reasonable to use it in the
gateway and renderer.
Bug: T152004
Change-Id: Ie1346d13b6e88004b817f5968448188ed83cfda4
After the gateway has made a request, it processes the page. This
processing both mangles and processes the MediaWiki API response. The
latter step contains two pieces of related business logic:
1. Deciding if a page has been modified recently.
2. Processing the extract/deciding if it's viable (see processExtract).
In order to support switching between the MediaWiki API and RESTBase and
to simplify the event logging reducer when it comes to instrumenting the
"type" of preview, extract this business logic into a model.
Bug: T152004
Change-Id: I20f29657fcf94101a71ed13c0920508db71292ce
I4c15296e introduced the wgPopupsConflictsWithNavPopupGadget client-side
config variable, an always-available flag that represents whether the
user has enabled the NavPopups gadget. This property has been missing
from the Popups instrumentation since I8a3f5835.
Changes:
* Add the value of wgPopupsConflictsWithNavPopupGadget to BOOT action
and reduce it into the base instrumentation data.
Bug: T151058
Change-Id: I09924d0cb10e96b0a846940506ebfeaa7086cd17
Ieb00709e changed the names of the page content language related
variables returned by the API from langdir and langcode to
languageDirection and languageCode respectively without updating the
renderer.
Moreover, the extract truncation mechanism introduced in I2ec0c04
couldn't handle RTL languages.
Both of these issues were found while writing a test plan for Page
Previews.
Change-Id: Ied3dbd7cf82749198e792a2f9d2241582aeff25c
Since the footer link part of the settings system - clicking the link
opens the settings modal - it seems appropriate that it should be
controlled by state managed by the settings reducer.
Bug: T146889
Change-Id: I3a8549dbf1952cd0556f663496c55de91acaf2c0
As before, we need to send the Page Previews blob that logged out users
can enable/disable previews.
Changes:
* Unconditionally add the ext.popups RL module to the output.
* Send the value of Popups\PopupsContext#isEnabledByUser to the client
as $wgPopupsIsEnabledByUser.
* Make mw.popups#isEnabled return $wgPopupsIsEnabledByUser if the user
is logged in.
Bug: T146889
Change-Id: Id2ccfaa81a327e222fff400f4a5f22f96c99ea31
The behavior of the cog varies when:
* The user is logged in/out.
* Page Previews is enabled as a beta feature.
Since the behavior of the cog doesn't vary per-preview, it can be
determined at boot time and passed to the renderer. However, in order to
keep the renderer stateless, we pass the behavior to it when a preview
is added to the DOM.
Changes:
* Add the mw.popups.createPreviewBehavior factory function, which
returns an object that encapsulates how a preview responds to the user
dwelling on it, abandoning it, and clicking on the cog.
* Invoke mw.popups.createPreviewBehavior at boot time and pass it to the
mw.popups.changeListeners.render change listener, which then passes it
to mw.popups.Preview#show.
* Make mw.popups.Preview#show responsible for binding event handlers
and configuring the cog based on the behavior.
Bug: T146889
Change-Id: I39d7d0afd7b1fe896019a1b3a82ee907bfb20edd
The SchemaPopupsSamplingRate config variable is inconsistently named and
even forwarded to the client as wgPopupsSchemaPopupsSamplingRate.
Changes:
* SchemaPopupsSamplingRate -> PopupsSchemaSamplingRate
* Forward PopupsSchemaSamplingRate to the client as
$wgPopupsSchemaSamplingRate.
Bug: T146889
Bug: T146434
Change-Id: I80d6a0abccf462e2eb0fd96af6849b5e82b49c26
Per Id1339dc2, the LINK_ABANDON_* and PREVIEW_ABANDON_* actions can (and
should) be merged, as they are always reduced in the same way.
Change-Id: I71b30d4d2774deb4efea9e565f2ccd7383bf08c1
Initially, the link element was added to LINK_ABANDON_* to determine
when an interaction should be reset. However, this wasn't enough to
protect against timing-related bugs, e.g. T154923. Now that tokens are
used to determine when interactions should be reset, the link element
can safely be removed from the LINK_ABANON_* actions.
Supporting changes:
* Update the return type of mw.popups.actions.linkAbandon in its
DocBlock.
Change-Id: Iaaed7a4846af75f9c4051d7bc761022ea5b3f6cf
I2ecf575b introduced a regression where the linkTitleChangeListener
wouldn't destroy the active link's title if it had been dwelled on
within 300 ms of the previous active link being abandoned.
The LINK_ABANDON_END action is dispatched after 300 ms and ignored if
the active link has changed.
Change-Id: If0c16afd6ca1c44f0e7eed497f62f0190005a619
... instead of using the active elements, and check also the user
dwelling flag to only act if the user is not dwelling.
Preview and link abandon end and start share now test cases, meaning it
can be collapsed into one action creator in a followup commit.
Change-Id: Id1339dc23e8f1b1b7514c769cd09f8bdadc759b4
When dwelling back to a link, previously a new LINK_DWELL action would
be sent and the reducer would reset the interaction as if it was a new
one.
This shouldn't happen, since dwelling back to the active link (from
the preview, for example) is not a new interaction, and should not
create a new interaction or hide and show the preview.
With this change, the preview reducer has the notion of new or repeat
interactions, and only resets state on new ones, and only sets
isUserDwelling when dwelling on a the link of the current interaction.
Also when the interaction is repeat, we guard on the action creator and
don't trigger the wait or the fetch request.
Change-Id: I71cde81cbfe50b6f955e562e7e5b57d0f920fdb9
... after dwelling on preview.
Currently, the asynchronous PREVIEW_ABANDON_END action wouldn't be
ignored if the user were to have dwelled on the link that the preview is
about. The action is only ignored if the user is dwelling on the
preview, i.e. if preview.isUserDwelling is truthy.
Set preview.isUserDwelling to truthy if the user is dwelling either on
the link or on the preview.
Bug: T154923
Change-Id: I8e6989790da5884992ee9dda9e4895e2e58e6694
Consider the user repeatedly dwelling on and abandoning a link. Testing
whether or not the element has changed before modifying state is not
sufficient. Fortunately, we already generate a unique token and include
it in the LINK_DWELL action for use in the eventLogging reducer - the
so-called "interaction token".
Change-Id: Iae9544055f7eccb4f7f071797eb360bc3456861a
* To speed the time the tests spend running
* To make them reliably execute and not depend on magic numbers
Additional changes:
* Use mw.now instead of Date on checkin.js
* Split big setVisibleTimeout test into several smaller ones
* Extract helpers for the tests
Change-Id: I9d3233fccf6de0997f968d096e375df996e87786
After the preview fade out animation has completed, then the preview
element should be removed from the DOM as it'll never be used again.
Supporting changes:
* Add the preview element to the DOM in ext.popups.Preview#show so that
it mirrors removing it from the DOM in #hide.
Change-Id: I6b3adc962aa13fbd46dce5f4c4f741299e43c6d9
