Reduce layout/style thrashing by measuring all required geometries
at event handler, not waiting for delays/redux/style changes.
Use CSS bottom instead of top, to avoid having to measure the popup
before positioning it, if it's placed above the link ("flippedY").
Disable some test cases that relied on implementation detail of using
"top" CSS.
Change-Id: Id0cbf506009b824d0fb6af4d6fe220e2f69aaaad
This is a performance optimization - removing
all the titles when initializing the popup extension
reduces DOM manipulation during hover, removing/reinstating the title
attribute.
When the popup extension is loaded, the default "title" behvior is unnecessary.
Change-Id: I1a85394b6b67eabee50a8d554bfd9b62de2a24d3
The tests were skipped for some time already because the beta
feature mode is taken into account on the test systems. The tests
need to login and enable the beta feature mode to be executed. This
is fixed with that patch.
The patch also fixes broken tests due to changes that made one test
obsolete and another that needed adjustments.
There are also comments added to places where code can be removed or
altered if the feature gets out of beta status.
Bug: T268134
Change-Id: Ib96d23f3cb6c6130fd5880a78fafd252bf706475
Now that the footnote label links to the references section, we don't
need this additional link.
Bug: T265482
Change-Id: Ib9b2939eb49e7b826c7699a5b7fa0e8255fa9da1
I played around with a lot of options, and settled with:
* When there is nothing but text, but the text is all
whitespace, don't show it.
* Make sure an <img> with no text is still shown. This is done
by checking for child elements.
A possible future enhancement could be to check the visible
width of the element as well. Unfortunately this fails in
tests. Everything is 0 in tests.
Bug: T240543
Depends-On: I2929a86b6a09f3b72e5e2f4151cb13f52446897d
Change-Id: I94ed575abcd69241c82480ade07017e61cc26c9c
We no longer intercept reference clicks, now clicking on a reference
label will scroll to the reference definition in the same way as when
reference previews is disabled.
Metrics about clicking on the label are collected by the Cite
extension, are are unaffected by this change.
Bug: T265482
Change-Id: I2929a86b6a09f3b72e5e2f4151cb13f52446897d
Elements that are marked as collapsible (often tables, but can
actually be anything) are most certainly marked as such because
they are big and don't fit in a popup.
Another plugin makes tables sortable.
In both cases non-functional UI elements appear in the popup.
We decided:
* Hide collapsible elements (no matter if currently collapsed
or not), and show a placeholder text instead.
* Remove sortable arrows.
This is a baseline patch that solves everything, except the
(i) icon is missing. This will be added in the next patch.
Bug: T220208
Change-Id: I58f3036bf4988d0ebe5716b0a54506446fca10c3
There are 3 types of links:
* Links with no class are links to other pages in the same wiki.
* Links with "extiw" are interlanguage as well as interwiki
links to other known wikis (e.g. from Wikipedia to Wikidata).
* Links with "external" point to somewhere else on the web.
Bug: T215419
Bug: T239230
Change-Id: Ia25db94d02670a919fc003f3e3562725de2e8eae
The rule no-shadow gets enabled in the version eslint-config-wikimedia
0.17.0.
In prepration for the automatic update with libup this needs to be done
in a separate patch set due to T262450
Change-Id: I66d405aef6d2b777e9a7f63734f2b5a5649d2080
Update NPM packages: webdriverio, wdio-mediawiki.
Replace NPM packages:
- wdio-mocha-framework with @wdio/mocha-framework.
- wdio-spec-reporter with @wdio/spec-reporter.
- wdio-junit-reporter with @wdio/junit-reporter.
New NPM packages: @wdio/cli, @wdio/local-runner, @wdio/sync.
Commands that return data no longer wrapped in `{ value: … }`.
Values are now returned directly.
Replace:
- `browser.element` with `$`.
- `chromeOptions` with `'goog:chromeOptions'`.
- `password` with `mwPwd`.
- `username` with `mwUser`.
- `moveToObject()` with `moveTo()`.
- `isVisible()` with `isDisplayed()`.
Bug: T258620
Change-Id: If339ff08b5e87c91905a65927ccd7e1b72b9f8e4
In this patch we introduce a new config variable and update its default
to be the same as the production value of $wgPopupsPageBlacklist.
When this code is deployed everywhere, we will remove wgPopupsPageBlacklist
from mediawiki config and defer to the Popups extension as the source of
truth from now on.
Bug: T254676
Change-Id: Ifebdcf8f5eec854a2b947dc390eaf47704a5c5eb
AVoid the term if possible, in all internal code. The only remaining
use of the word is in the public $wgPopupsPageBlacklist config
variable.
Change-Id: Ib238731270f473ad44fcff13df617a70433f2452
Popups is enabled on the Wikipedias but disabled by default, which makes
it more difficult to enable on third-party wikis.
Things should be enabled by default, and them WMF configs should disable
features when they are not used/available.
Supporting changes:
- Rename PopupsContextTest#testAnonUserHasDisabledPagePreviews and
remove the associated data provider, which provided only one datum
- Set $wgPopupsReferencePreviews to false in
PopupsContextTest#testShouldSendModuleToUser in order to isolate the
code under test
Change-Id: Ib8cc7041d792bed0a19d18e14506627099a69bef
This is a direct follow up for I4d31805. Note the previous patch was
only testing the code path when Beta is disabled. But the bug was about
what happens when Beta is enabled. This is now properly covered by
these tests.
Bug: T240947
Change-Id: I54c5e9751e14a94808e85d935e1972eea0395002
It appears like we accidentially copy-pasted the behavior for
PagePreviews and made ReferencePreviews behave the same, not taking
into account that the later feature is in Beta mode.
Bug: T240947
Change-Id: I4d31805ee9b2045c49c7ab4179c5a4adbcba0394
Notice how this actually reduces the size of the final, compiled index.js.
It's not much, but still.
One issue I noticed is that the coverage reports for the JS code stopped
working. I have no idea why.
Bug: T208951
Change-Id: I2fe92579574b3b1ba4d2dd064899eee944045a96
The signature mw.RegExp.escape() is deprecated since MediaWiki 1.34.
By the way, this is the 4th or even 5th time in my short career this
tiny, single line (!) helper function is moved around and I need to
update all my code. This felt already ridiculous when it happened the
2nd time.
Change-Id: I4d05a49120aff64ebc316d0af7736c62385d9307
This was motivated by two things:
1. Notice the bad class name in UserPreferencesChangeHandlerTest. This
was possible because the default mock builder does not check if a class
exists, *and* there was no type hint in place. createMock() *does* check
if a class exists.
2. We are free to use createMock() from PHPUnit 5+ now. It's not only
shorter but more strict and reliable.
Change-Id: I301a171d587026eab0a62575ab2fdbfd7733c661
Logs events to the ReferencePreviewsPopups EventLogging schema, in
order to understand whether Reference Previews is helpful for
end-users.
This will be removed along with the older tracking, as soon as our analysis
phase is finished.
Incidentally disables a lint rule for the generated JS, it's about
readability so irrelevant to the minified code.
Bug: T214493
Change-Id: I2638611ba67b785338f7e98a1c4b08a5e829812d
A method that is expected to return, for example, a boolean true
should not return an other value that PHPs loose == comparison might
also consider true.
Same for all other loose equality checks.
Change-Id: If729c6e97d5337eee10b717da76dad428218ff69
The eventlogging `isEnabled` function determines when to sample,
this patch removes the `isAnon` conditional so that we can measure
reference reading habits for all users.
Also guards against a non-function navigator.sendBeacon, which was
previously intended but incorrectly tested.
Bug: T214493
Change-Id: I42cb3082fb85c7900426a2055dfa3c2f6ecfd968
I was able to track the issue down to the changes made in this patch:
https://gerrit.wikimedia.org/r/331563
This patch is mostly a simplification of ce8a2d4
(I9a73b3086fc8fb0edd897a347b5497d5362e20ef):
- Don't make wait#wait() abortable. This adds complexity and isn't
needed since the token is rechecked in the .then() of
actions#linkDwell(). The request is permitted to continue and fetch if
that token still matches and is never issued otherwise.
Once a request has been issued, that request is still abortable.
However, note that calling XHR.abort() is just a request to abort and
may not be granted. Whether or not XHR.catch() is invoked is what
dispatches the FETCH_ABORTED action (or doesn't if the request to
abort was denied).
- Remove the abort tests for wait#wait() since the functionality is no
longer provided.
- Pass the preview token in the FETCH_ABORTED action and reduce
FETCH_ABORTED the same way as ABANDON_COMPLETE in reducers/preview.
The follow-up patch, I3597b8025f7a12db0cf5d83cce5a77abace9bae3, adds
integration tests for the specific bug fix. Note that these Selenium
tests are incompatible with the content proxy, so it is probably best to
simply unset $wgPopupsRestGatewayEndpoint and $wgPopupsGateway and allow
the defaults to be used. Also note that the tests are incompatible with
recent versions of Node.js (so use NVM) and emit many deprecation
warnings (so set deprecationWarnings to false in
tests/selenium/wdio.conf.js) An example run of the tests looks something
like:
chromedriver --url-base=wd/hub --port=4444 &
# If any changes are made locally, also run `npm -s start &`.
MW_SERVER=http://localhost:8181 \
MEDIAWIKI_USER=foo \
MEDIAWIKI_PASSWORD=bar \
DISPLAY= \
npm -s run selenium-test
Live testing may be performed as well. Remember that RESTBase requests
are incompatible with MobileFrontend's content proxy hack so ensure to
comment it out if $wgPopupsGateway is configured for RESTBase (see
T218159).
1. Open the DevTools network tab.
2. Disable the browser cache. Chromium, at least, won't abort requests
coming form the cache.
3. Hover back and forth quickly over a preview. In Chromium, canceled
requests are labeled and appear red. This is a good scenario to test.
With the patch, a preview should always be shown when ultimately
resting on a link. Without the patch, it is possible to rest upon the
link with no preview showing. This may require several attempts.
Bug: T219434
Change-Id: I9da84b0296dd14e9ce69cb35f1ca475272fb249a
As mentioned in T205744, EventLogging schema ResourceLoader modules have
been deprecated. This removes those modules from loader calls.
Bug: T221281
Change-Id: I1b7355c69e09756f50ccd1c1955b45cae4a64b9e
This fixes page previews appearing when there is self link to a page
that has a file extension in it's name.
Bug: T222869
Change-Id: I5caf610fa76986026948a5b7b55537723752b755
Introducing the REFERENCE_CLICK action that will fetch and show the
preview for the clicked reference right away without any delay.
The main goal of the new chain of events introduced with the reference
click is showing the reference preview right away. The actions triggered
by the dwelling include delays in multiple parts of the process.
If there's a dwell action-chain in progress when the click action is
executed, the related promise ( that might still include steps with
delays ) and the reference preview is retrieved and shown right away
re-using the token.
In the case where there's no dwell action running ( e.g. when the click
was triggered via touch ) we create a new token and start from scratch.
In either case we want to avoid, that multiple clicks trigger multiple
actions and abort early when there's already a click action in progress.
Bug: T218765
Change-Id: I073a93be2d17a21178aebe12267765f60a2811b9
Pretty much all usages of this function do *not* use the second
parameter to pass a page name prefixed with a namespace, but pass the
page name only.
This patch here removes the redundancy. The namespace prefix is now a
generated one, saying "Namespace <number>:…". It turns out no test
relies on this so far.
Bug: T220097
Change-Id: Ibd45d49c91e86a2647afe676a5e3bb07dfeab6ed
config.get( 'wgTitle' ) returns the unnormalized title of the current
page while title.getName() gets the normalized title (e.g. with underscores ).
On pages with spaces in the check failed before this patch.
Bug: T220097
Change-Id: I58a532627bb27be030cbc553f1181a89109edd80
Named references may include non-ascii characters, so we decode the fragment before matching against reference IDs.
Bug: T220196
Change-Id: I63bba59fa8f0f6aa95aeadbb1f85745d480988bd
It is set based on the same conditional that loads the code,
thus checking it inside the loaded code is a no-op and adds
extra HTML to the <head> that blocks text/layout rendering and
delays fetching of Popups JS.
Bug: T219342
Change-Id: I9c1f4b3861ce2cecb654eb0a78469a616730a40b
I just realized on another test set, that this is actually not implemented
in a way where it is working correctly. The return value of the browser.exectue()
is returned as part of an object and not directly. So the condition was always
true and the wait until did not really wait for anything.
As a result I'm quite confident the pause is not necessary.
Change-Id: I274bdee0b3c39c418a2b61881d56f89889c53485
The test would lead to an exception when 'mediaWiki' is not defined. The
exception would then also abort the whole execution so in this context it's
safer to use typeof with 'undefined'.
When the mediaWiki js base is loaded though, loader.getState() is guaranteed
to be available since it is part of the root module.
This also uses "mw" instead of "mediaWiki" for consistency in test.
Change-Id: I1262d0b5c4a1136f4d2294f125336e72118c6e2c
- test page loading and waiting for the scripts moved to beforeEach
- removed unnecessary abondonLink call
- removed unnecessary browser.pause()
Change-Id: I28eb7b9b48f105315bf41f7a41e5a1e6ec21cb2b
I had to disable ESLint to be allowed to upload this patch. It starts
complaining about something in code I did not even touched. The error
message does not make any sense to me (something about globals being
forbidden in code where I can not spot anything that would be remotely
global).
Change-Id: I6d4b178a65126c4b81b87d99142a6cdc845ae5ee
Two big chunks of code (the "click" and "scroll" event handlers) have
not been covered with QUnit tests before. I found this was not that
complicated and worth the effort.
Note we already have browser tests in place for these features. These
are still required because the scroll feature can not fully be tested
when the popup is not actually rendered on screen.
Bug: T214971
Change-Id: I58111489fe6c4bed65efec59f9fc4184c828b2b3
With this I want to pull apart testing two things at one, checking if the
URL is escaped and if it is put to the right position.
So this adds an explicit test, that makes sure that urls in the popup are
escaped safely and lets the big test become more general in that regard.
Bug: T214971
Change-Id: I09b5225a8370e8b1337b2cf6ca03ccb79b3a64aa
For the reference preview tests we extended that with a lot of stuff
that we do not really use in the tests atm. Lets only have stuff in there
that's really relevant for the tests.
Change-Id: I03c6e00445e9bfe48572fd1b19a7ef1ecd472f4e
When a thumbnail in portrait-mode is narrower than the 200px
expected width, the SVG clip-path should be shifted on the x-axis
in order to align with the thumbnail image.
Adds extra test-cases to validate this logic.
Bug: T204627
Change-Id: I9359c9fb335e5fad3f7d5ba33ee89d2a1f26b8b2
During story time on 2019-03-12 it was decided to consistently talk
about "References" in all messages. Main motivation is that this is
the term the community is most familiar with, and it is also the term
that is used in reference section headings most of the time.
Bug: T215063
Change-Id: Iaab8d2c0da1546a3c9d27bc8e2e1c784050ed135
The method itself has not much to do with gateways as such, it's
more about the general preview type selection. Since the preview
types "live" in the model, I thought it might good to move it there.
Doing that the "original" getPreviewType method in the model was renamed
to avoid conflicts. If I get this right, that method is quite specific to
page previews, since it processes the output from the TextExtracts API-
request. - Therefore I also removed the TYPE_REFERENCE there, because this
code path will never be reached with that type afaik.
Inspired by the comments in Id1fa7dad59d8fe80bc60c1e2d7c3fb4087e52d1f and
as preparation for that patch.
Bug: T215420
Change-Id: Ic9e24a73e945c7d56435c656ecfdb42b65601d22
I believe these additional newlines all make the code easier to read.
It's easier to see what belongs together, and what is a separate thing.
I found the Squiz.WhiteSpace.FunctionSpacing sniff very helpful to
enforce this code style. We enabled this already in almost all WMDE
codebases. It is not yet part of the upstream MediaWiki rule set, but
discussed.
Change-Id: Ibdf788529b28637bf98e7940c2516852c3afcef7
The majority of this was fixed just recently via I8de42df. The sniff
makes sure all old and new code conforms to this style.
Change-Id: I8aecf3653d021fc8f86abcdc5939529864f86a48
-> Use single quotes for string literals.
-> Document missing parameter in test method.
This is just to improve on code readability, consistency
and maintainability.
Change-Id: I8de42df9f856ecb409637fe33b5f84b8bed1b547
* Explain the difference between the two hooks that both set config vars.
* Remove by-ref & that are not needed. This is a non-breaking change.
Even if the code calling a hook handler provides a variable by reference,
the hook handler being called does not need to require a reference.
Removing these & makes the code less confusing and easier to read.
* Replace an OutputPage type hint with a more narrow IContextSource.
That's all this code actually needs to know. It doesn't need access to
the entire OutputPage, so it doesn't need to require it.
* Update the signature of the ResourceLoaderGetConfigVars handler to
match the caller.
Bug: T215896
Change-Id: Ie1e4b50cd954812f71dd628003b8e9d40fdf5fe8
As suggested in I69ad209.
As far as I can tell the idea was to be able to pass logger-specific
configuration to the logger factory, e.g. to be able to toggle certain
things the loggers would do. But this is not used at the moment. There
is not much value in keeping unused code around. It can esaily be
introduced again later when it turns out it is needed.
Furthermore, I'm told most of the logging functionality should be
removed anyway. See T193051.
Change-Id: I6b1ddb2a65eacc0e096f2ba44922d63e63212a65
Rename selectInitialGateway() to selectGatewayType() in
gateway/index.test.js. This test's import name was out of sync with the
declaration which made it more difficult to grep and reduced
consistency.
Change-Id: I60ae359043fdf76015d75b6438992ef6061a4d72
This happened one time before and was fixed via T212419. It now came
back after I2670331, a patch for T214861 that made the title parser
accept more links than before.
This patch implements a decision tree that goes as follows:
1. If it's a link to another page, it can't be anything but a page
preview, as having a peek into another page is effectively the
definition of a page preview.
2. Otherwise it's probably a reference preview, but only if:
* It's a link with a jump mark.
* The link is actually marked as being a "reference".
* Reference previews are enabled.
3. If neither of these definitions fit, do nothing. Note this is not
"hiding errors" we would be able to fix in this code. There are many
ways to manually arrange wikitext in a way that it looks like one or
the other, without fulfilling all requirements. Unfortunately the
user who would see error message or dysfunctional popups would not
be the same as the user who wrote such wikitext.
Bug: T216683
Change-Id: I8d021f19ddc73a261e6a0c62959ddd0cb1d3182d
- Drop unused moment, Redux, and ReduxThunk globals.
- Instead of using a confusing and deprecated boolean, specify that
globals are "readable" / "writable"
https://eslint.org/docs/user-guide/configuring#specifying-globals
Change-Id: I711a4ea7c51c9a560f20fc28bc99bd932c7e1e25
These are annotations for ESLint as described at:
https://eslint.org/docs/user-guide/configuring#specifying-globals
I'm not sure where the `…: false` comes from. I assume it is a mistake
and does not have an effect.
I tried to move these annotations closer to the line they are about in
case there is only one line. And move it to the top when there is more
than one line using the global.
Change-Id: I4bd112c5fddd8a97d829a9b91707b8eb7cd7a332
While reviewing the code of the reference endpoint I found it encodes
two CSS class names, "mw-reference-text" and "reference-text". So far
our scraping gateway only implemented one.
https://phabricator.wikimedia.org/diffusion/GMOA/repository/master/
This patch also extracts a piece of code to a named function. This makes
it much easier to read, I feel.
Bug: T214908
Change-Id: I9d1bb1f4c21eb9d57a6b763ca1f756e6cf7049e0
These icons are currently unused because the gateway does not deliver
the necessary information. This will be used starting with I6223cbb.
This patch aims to introduce all resources needed by the later.
Bug: T214908
Change-Id: Ie0c3c059222700169f2605c3123554c74d974256
The definition of "self-link" in this context is an <a href="…">
element that points to the exact same URL as the current document's
location, excluding a possibly different #… fragment. This is typically
the case when the <a> element does not contain a full URL, but something
like href="#fragment". JavaScript's HTMLAnchorElement.href getter
automatically expands this to be a full URL.
Example:
var a = document.createElement( 'A' );
a.href = '#test';
console.log( a.href );
Notes:
* This new code assumes the wgPageName setting properly reflects the
page name requested via the current document's location. Core does
give us this guarantee.
* The only URL element that is intentionally not compared is the
protocol.
* This accidentially fixes T215899 as well, because the namespace check
is now bypassed for self-links (as it should).
Bug: T214861
Bug: T215899
Change-Id: I2670331cbbdebf7dc9fc70d7342724534f9f54ec
This patch also removes misplaced empty lines at the beginning of a
scope. In PHP code we even have a sniff for these. In JavaScript we
don't, but I suggest to be consistent about this.
Change-Id: Ic104ae8fe176da1dafa9bc783402adecb71de1f0
According to the standard.
Note: In a previous patch I removed the <?xml …?> line. This conflicts
with what https://www.mediawiki.org/wiki/Manual:Coding_conventions/SVG
suggests. However, I think the removal is ok in this particular case
because this file will never be shown as an image, and never be shown
standalone. Instead, it will be inserted in the documents DOM. The XML
header doesn't matter anyway then.
Change-Id: If23ad54985abb30f8c92500546bd04eeca44fab3
I decided to keep the comments because they are sooo helpful, but
tried to shorten them a bit. The biggest change is the indention with
tabs and the much more compact <path> elements. The shapes are the
exact same. I manually confirmed this for all four.
Change-Id: I2d1294c9ae7e398dcbe2d111c42848d17be8a67e
I tested this with all 16 possible combinations:
* The pointer can show up in all 4 corners.
* The popup can contain a thumbnail or not. The code for the pointer
is very different then, because the SVG masks are only relevant in
this scenario.
* The thumbnail can be tall or not.
* I even tested tall popups without a thumbnail. This is a combination
that is impossible in production scenarios.
I found 3 issues. This patch fixes 2 of them:
* The pointer is misplaced in the bottom-right corner when the popup
does not have a thumbnail (as reported in T215194).
* The pointer is misplaced in the upper-right corner when the popup
shows a thumbnail.
* The pointer in the upper-right corner is gray instead of white when
the popup is tall, but does not show a thumbnail. As this is not
relevant in production, I did not fixed it.
It seems both misplacements are because of the same reason: For some
reason, calculations are done based on the assumption the popup would
be 300px wide, but it is 320px wide.
Note I did not just added 20 everywhere, but manually alligned the
pointer triangles so they are placed exactly the same distance from
the corner as in the three other corners.
Note I did not tested this (yet) in RTL scenarios.
Bug: T215194
Change-Id: If0ca63d4d4b6e8083c7de1517fe32f49671a40e6
This is now possible since the render functions return jQuery objects.
All this code is exclusively used in the pagePreview.js file, and
doesn't need to make the already very big renderer.js file even
bigger.
Note the tests for all renderers have always been collected in a
single file. That's why the test case does not move.
Change-Id: I0c24638751c5f0e93d2bc0f3f4bb61fa0cf50d15
This just adds a simple test if passing the id of the clicked
reference source footnote works.
Bug: T213905
Change-Id: Ifc6549aa0203f19a5b24fa854b0aaf0cfb25674d
To make sure that we enable the link highlighting in the Cite extension we want
to trigger the click handler on the original footnote link. This is done by
passing the id of the source element to the model and the renderer.
Bug: T213905
Change-Id: I0bd59ac326269f3c0850946851fb79b611dc2a57
