Commit graph

339 commits

Author SHA1 Message Date
Ed Sanders c96c076a4e Follow-up I46a58f6a: Add missing test runner
Change-Id: I17438cf55959d146265f3aafa5940f7fe0de19e1
2022-06-07 16:09:04 +01:00
Ed Sanders ab05e4e24f Add test coverage for ApiDiscussionToolsPageInfo::getThreadItemsHtml
Change-Id: I46a58f6a2ec5f0e1b750c67e85d27081f5fab544
2022-06-06 18:40:30 +01:00
Bartosz Dziewoński 6a59149132 Ignore LRM and RLM in more places in the timestamp
We previously ignored them before timezone indicator (e9c401e3aa),
but they can end up in other places too, e.g. after the time.

Now we ignore them after every token. This is way overkill, but it
shouldn't hurt.

Bug: T308448
Change-Id: I20f7aaa34dba23f2a2faf1be258c1aea32ab770f
2022-05-17 02:00:22 +02:00
jenkins-bot fd6297b8a8 Merge "Remove data-mw-comment-name attribute from subscribe links" 2022-03-29 21:01:25 +00:00
jenkins-bot 081fa1008a Merge "Use Sanitizer::stripAllTags() when generating notification snippets" 2022-03-28 22:42:39 +00:00
Ed Sanders 48fdcf1056 Remove data-mw-comment-name attribute from subscribe links
The HeadingItem's name is now present in the DOM, so just traverse
to it and use that instead.

Change-Id: If28e1588742513d606e3d8fcfb259b85acc0a873
2022-03-28 22:30:59 +01:00
jenkins-bot a07871d7d7 Merge "Add item name to the JSON output of HeadingItem" 2022-03-25 11:51:42 +00:00
David Lynch 71049f4ce8 Add item name to the JSON output of HeadingItem
The name is needed for the topic subscription API

Bug: T285971
Change-Id: Iedbebdfd65d03ab01b22b35781803655749aa269
2022-03-25 11:45:50 +00:00
jenkins-bot 35b3fd2fc0 Merge "CommentParser: Replace uses of Title with TitleValue" 2022-03-23 01:14:16 +00:00
jenkins-bot 334e1a3f33 Merge "Fix parsing of non-English titles in tests" 2022-03-23 01:02:12 +00:00
Bartosz Dziewoński c5375e05b9 CommentUtils: Fix isSingleCommentSignedBy() with empty heading
Change the order of checks to ensure that we have at least one comment
before we try comparing ranges, to avoid issues with empty headings
having collapsed ranges. It should be a tiny bit faster this way, too.

Bug: T304377
Change-Id: I59ad30cfc075dcec882e048d2d199744efec2114
2022-03-22 00:12:42 +01:00
Bartosz Dziewoński c7723baf72 CommentParser: Replace uses of Title with TitleValue
Another small step towards removing the reliance on global state.

Change-Id: Ifb4a5bcbef6606d02f1c7aa7385d72822cb0bad0
2022-03-18 18:24:34 +00:00
Bartosz Dziewoński b68832ace0 Fix parsing of non-English titles in tests
We were calling Title::newFromText() before setupEnv(), which meant
that the title for each test case was parsed using the default rules
for English, rather than the rules for the specified wiki.

This only makes a practical difference for tests with self-links.
Changed the only such test to demonstrate the fix.

Change-Id: I45561f1c9f0d149e2b743f0000b742bf6fc014af
2022-03-18 18:24:07 +00:00
Bartosz Dziewoński 01b253c5b6 Don't allow the root node to be treated like a comment frame
Also fix a bug where headings would be ignored while checking for
comment frames. See task for detailed explanation.

Bug: T303396
Change-Id: I6495826b4b050ea80680e0798ac6ab4497a7c09e
2022-03-10 17:45:08 +00:00
Bartosz Dziewoński 77614a2d02 tests: Fix root node / container handling
Since times immemorial, and for reasons lost to history, our test code
was adding an extra <div> wrapper before parsing the HTML used for
tests. This wasn't a problem, until now, because I want to add some
tests for T303396 that need to check that the *real* wrappers present
in some test cases are handled correctly.

Changes to test cases mostly remove a leading "0/" from serialized
ranges, corresponding to removing the extra wrapper.

Change-Id: Ia50e3590538c8cd274b02d2a937ba1a3fbb4ac89
2022-03-10 18:43:58 +01:00
jenkins-bot dd24b0edcd Merge "Improve handling for comments after fake headings using wikitext ;" 2022-03-10 16:21:18 +00:00
jenkins-bot 32d9ef573a Merge "CommentParser: Avoid using a dynamic undeclared property" 2022-03-10 00:22:16 +00:00
jenkins-bot 76478dda26 Merge "Move signatureScanLimit to a constant in JS" 2022-03-10 00:22:14 +00:00
jenkins-bot bd43c2a139 Merge "Add test case for fake headings using wikitext ;" 2022-03-10 00:17:49 +00:00
Bartosz Dziewoński 4c29304484 CommentParser: Avoid using a dynamic undeclared property
Change-Id: Iefa8dea83bc0d31b9c6b3509189eeaa652dd9ea0
2022-03-08 23:30:11 +00:00
Bartosz Dziewoński 063174e71c Use instanceof for checking for text/element nodes in PHP
It is friendlier for static analysis tools like Phan, which can't
infer anything from the `->nodeType === …` checks, and we were already
using it in most places.

Fix newly revealed Phan failures (and one unneeded suppression).

Change-Id: Id789f05e16a210f7ba22ca7514587c392fac0741
2022-03-08 23:28:39 +00:00
Bartosz Dziewoński 0030f4cb9b Disable the biggest JS modifier test cases temporarily
Bug: T303074
Change-Id: I9bd284feb4ede27aadf99904fd230d9bfd778351
2022-03-04 20:22:11 +00:00
jenkins-bot 3c91a800ed Merge "Improve detecting already signed comments" 2022-03-02 14:14:13 +00:00
jenkins-bot 094b77b4bb Merge "Handle reply/topic preview entirely server-side" 2022-03-02 14:13:59 +00:00
jenkins-bot e15ccb8a07 Merge "Highlight all comments since the oldest in a thread bundle" 2022-02-28 23:24:12 +00:00
Ed Sanders dc8b4e8d4f Highlight all comments since the oldest in a thread bundle
For topic subscriptions, further restrict this to comments
in the same thread.

Bug: T302014
Change-Id: Ifba218871122901031a891034e709b886fc406da
2022-02-28 21:58:10 +00:00
jenkins-bot e4fa34f025 Merge "Don't insert comment markers inside replaced elements (like <video>)" 2022-02-28 17:16:11 +00:00
jenkins-bot 542da89530 Merge "Don't detect comments within references" 2022-02-28 16:47:21 +00:00
Bartosz Dziewoński 1e3ce9c88a Don't insert comment markers inside replaced elements (like <video>)
Also special-case thumbnail wrappers generated by
MediaTransformOutput::linkWrap, for compatibility with
TimedMediaHandler.

Bug: T301427
Bug: T302296
Change-Id: I7f48d8b2261507c5a33526c54109f5187d062ed3
2022-02-22 15:11:34 +00:00
Bartosz Dziewoński 8a2715bdd5 Move signatureScanLimit to a constant in JS
Change-Id: Ieb60c148fd060ab62e4a493e2d0dff6c051f945c
2022-02-21 22:42:14 +01:00
Bartosz Dziewoński 0ecc8a4c05 Improve detecting already signed comments
Previously, we required a signature at the end of the comment.
This was a pretty rough heuristic that did not correctly handle
many comments that we would consider entirely properly signed
in CommentParser (e.g. comments wrapped in formatting like
<small>…</small>, comments with a post-scriptum or in parentheses,
or comments generated by various templates).

Now we process the user input using the same code that adds reply
links, and only add a signature when we detect that there really
isn't a signature (including template-generated), or if the signature
is in the wrong place and would result in the reply link showing up
in the wrong place as well (not at the end of the comment).

Bug: T278442
Bug: T268558
Bug: T278355
Bug: T291421
Bug: T282983
Change-Id: I46b6110af328ebdf93b7dfc2bd941e04391a1599
2022-02-21 21:21:26 +00:00
Bartosz Dziewoński 4244418e56 Don't detect comments within references
Bug: T301213
Change-Id: Ifd5198651c8ed0ce53379fb5e35938089cd54a09
2022-02-21 19:57:44 +00:00
Bartosz Dziewoński 85165543f4 CommentParser: Inject a forgotten service
Also sort alphabetically.

Change-Id: I9e77c4aa1fba930f382e3c4f17ac0504c2f06668
2022-02-21 20:15:54 +01:00
Bartosz Dziewoński 1d43a024f9 Handle reply/topic preview entirely server-side
We were rendering the preview in a completely different way from how
we would add the real reply, and the results would be different
sometimes, particularly for multi-line comments with messed-up markup.

Render it server-side instead, in a very similar way to real replies
(generating a DOM list node and transforming it through Parsoid),
although without the whole context of the page to improve performance.

We can remove a lot of client-side code that was used solely for this.

This will allow the preview to accurately display the signatures when
we change how they are added (T278442), without us having to implement
those changes again from scratch for the preview.

Change-Id: I53341f4d4075c25b67ec3b3032bff9b8a880dcd3
2022-02-21 17:42:28 +01:00
Bartosz Dziewoński 8e44b43df0 Split off ThreadItemSet from CommentParser
Goal:
-----
Finishing the work from Iadb7757debe000025e52770ca51ebcf24ca8ee66
by changing CommentParser::parse() to return a data object, instead of
the whole parser.

Changes:
--------
ThreadItemSet.php:
ThreadItemSet.js:
* New data class to access the results of parsing a discussion. Most
  methods and properties are moved from CommentParser with no changes.

CommentParser.php:
Parser.js:
* parse() returns a new ThreadItemSet.
* Remove methods moved to ThreadItemSet.
* Placeholder headings are generated slightly differently, as we process
  things in a different order.
* Grouping threads and computing IDs/names is no longer lazy. We always
  needed IDs/names anyway.
* computeId() explicitly uses a ThreadItemSet to check the existing IDs
  when de-duplicating.

controller.js:
* Move the code for turning some nodes annotated by CommentFormatter
  into a ThreadItemSet (previously a Parser) from controller#init to
  ThreadItemSet.static.newFromAnnotatedNodes, and rewrite it to handle
  assigning parents/replies and recalculating legacy IDs more nicely.
* mw.dt.pageThreads is now a ThreadItemSet.

Change-Id: I49bfe019aa460651447fd383f73eafa9d7180a92
2022-02-21 16:22:32 +00:00
Bartosz Dziewoński 4613ae78e7 Change CommentParser into a service
Goal:
-----
To have a method like CommentParser::parse(), which just takes a node
to parse and a title and returns plain data, so that we don't need to
keep track of the config to construct a CommentParser object (the
required config like content language is provided by services) and
we don't need to keep that object around after parsing.

Changes:
--------
CommentParser.php:
* …is now a service. Constructor only takes services as arguments.
  The node and title are passed to a new parse() method.
* parse() should return plain data, but I split this part to a separate
  patch for ease of review: I49bfe019aa460651447fd383f73eafa9d7180a92.
* CommentParser still cheats and accesses global state in a few places,
  e.g. calling Title::makeTitleSafe or CommentUtils::getTitleFromUrl,
  so we can't turn its tests into true unit tests. This work is left
  for future commits.

LanguageData.php:
* …is now a service, instead of a static class.

Parser.js:
* …is not a real service, but it's changed to behave in a similar way.
  Constructor takes only the required config as argument,
  and node and title are instead passed to a new parse() method.

CommentParserTest.php:
parser.test.js:
* Can be simplified, now that we don't need a useless node and title
  to test internal methods that don't use them.

testUtils.js:
* Can be simplified, now that we don't need to override internal
  ResourceLoader stuff just to change the parser config.

Change-Id: Iadb7757debe000025e52770ca51ebcf24ca8ee66
2022-02-19 19:51:57 +01:00
Bartosz Dziewoński 99b5de8038 Split Data class into ResourceLoaderData and LanguageData
The Data class contained utilities for two unrelated purposes.
Split each half to a separate class.

Notably, this improves the signature of the getLocalData() function.

Change-Id: Icde615fb9d483fee1f352c34909b37f8ffde8081
2022-02-19 19:37:34 +01:00
Bartosz Dziewoński ae9f26a9e5 Various code quality tweaks
(suggested by PhpStorm)

composer.json:
* Document required PHP extensions

Parser.js:
* Remove incorrect param documentation
* Fix some typos in comments (missing parentheses)

CommentParser.php:
* Fix some typos in comments (missing parentheses)

ImmutableRange.php:
* Remove unused property
* Add a `throw` to indicate that code path is unreachable

SubscribedNewCommentPresentationModel.php:
* Add missing `return false`

CommentParserTest.php:
* Remove unnecessary pass-by-reference

CommentModifierTest.php:
* Remove unused variable

CommentParserTest.php:
* Don't construct Element objects directly. PHP's DOMElement allows
  it, but Parsoid/Dodo's doesn't, and we use the latter for static
  analysis. This generates all kinds of confusing warnings.

Change-Id: Ia9598ebea0e99830dd485296e94a9d96acc4b258
2022-02-19 19:36:52 +01:00
Bartosz Dziewoński e414d1acaf Improve handling for comments after fake headings using wikitext ;
Bug: T265964
Change-Id: I77db68928c5426fd885a277eec52c6e164d559bb
2022-02-11 23:35:32 +00:00
Bartosz Dziewoński 62766e846c Add test case for fake headings using wikitext ;
Bug: T265964
Change-Id: I31db66dc011b3d13dd46426f15286c5b3b5c9254
2022-02-12 00:35:02 +01:00
Bartosz Dziewoński 91e1bb15cc Use Sanitizer::stripAllTags() when generating notification snippets
It adds white-space between block tags and strips invisible tags.

It may be slightly slower (it takes HTML as input rather than DOM, so
we need to serialize the HTML first and then call it, rather than only
find and concatenate text nodes), but the difference is negligible,
and it seems better to use this method than to try to re-implement it.

Test runtime went from ~9.0s to ~9.5s locally, when testing using:
  php tests/phpunit/phpunit.php \
  extensions/DiscussionTools/tests/phpunit/ThreadItemTest.php \
  --filter getText

Bug: T219138
Change-Id: I0cb89ebd2160e1ef499b78573c6688f493a4c42f
2022-02-10 22:23:24 +01:00
Bartosz Dziewoński 165ca9b847 Improve CommentModifier::addReply() API for re-use and testing
Goal: To be able to re-use or test the transformations we previously
  performed in addWikitextReply() / addHtmlReply(), without requiring
  a Comment object or adding the result as a reply.

Change-Id: I040c4be9b6b9bddba661f30fd0566f8850673074
2022-02-03 21:12:48 +00:00
Bartosz Dziewoński 15f0867b75 Limit where whitespaceParsoidHack() is used
* We don't need it anywhere in JS, since we're not sending that HTML
  to Parsoid.
* We only need it on the nodes directly containing our reply lines in
  PHP, not all over the place.

Change-Id: I0a04388225f32654dda2f599442cd27a303b5d0a
2022-01-29 22:42:46 +00:00
Bartosz Dziewoński bacf6a8fc6 Remove unused code in JS modifier
I don't think we're going to need this in the client again.

Change-Id: Id38405c14edfd7ac45afad8f32cad64594ae7bc0
2022-01-29 21:39:45 +00:00
Bartosz Dziewoński 110a59200f One more tweak for comparing comment ranges to transclusion/DOM ranges
When we encounter a node that doesn't represent comment contents, e.g.:
* a [reply] link we inserted (T297034#7641334)
* an {{outdent}} template (see changed test case)

…we should ignore it together with its descendants (like in
Parser#nextInterestingLeafNode), instead of processing descendants
and possibly detecting comment contents in them.

Follow-up to 8de940b587,
72b9c2c6f5.

Bug: T297034
Change-Id: Ib2fa40c5fa389572b0e88ef558728fa06e3621b0
2022-01-24 17:42:18 +00:00
jenkins-bot 3a4a6bff16 Merge "Add another test case for transclusions overlapping comments" 2022-01-21 22:25:46 +00:00
Bartosz Dziewoński 4e4e9f1c80 Add another test case for transclusions overlapping comments
Prior to 8de940b5, the comments on this page would not be marked as
partially transcluded.

Bug: T298408
Change-Id: Ib7eb8b4113151048c0e778b3530600d98dd8f705
2022-01-17 23:52:02 +01:00
Bartosz Dziewoński b7cbd714ca Add tests for bullet indentation
Bug: T259864
Change-Id: If38016564b67ee7217fe7328b40973aa244ff467
2022-01-14 00:27:04 +00:00
Bartosz Dziewoński 1ce3d7ab7e Work around Karma test runner timeouts caused by large tests
Change-Id: I8c7d32dac073e1f0510e57f26ba81ff22f005f0a
2022-01-14 00:26:37 +00:00
jenkins-bot 3699158e81 Merge "Handle selflinks by returning the current page's title" 2022-01-12 21:46:35 +00:00
jenkins-bot 7f329ca9a2 Merge "Enable wikis to customize the syntax used for replies" 2022-01-12 21:32:49 +00:00
Ed Sanders f80ff74fc6 Handle selflinks by returning the current page's title
Bug: T287818
Change-Id: I67f10ac9976581279d1e6a477e90d55875ebab20
2022-01-12 21:18:04 +00:00
Ed Sanders 34011b7a07 Parser: Pass in title of page being parsed
Will be used to parse selflinks in the future.

Change-Id: I2bc29d1c5c69cb6309f582f162f9af7d96ce8913
2022-01-12 21:17:59 +00:00
Ed Sanders 1fed7115f4 Tests: Add original titles to test cases
These are not used for anything yet, but soon the parser will
want to know the title of the page it is parsing.

Change-Id: I02fa5d63fae78f3e92032d93bc27ac5c744faecb
2022-01-12 22:16:03 +01:00
Bartosz Dziewoński 7b1053300a Enable wikis to customize the syntax used for replies
The following values for configuration variables are supported:
$wgDiscussionToolsReplyIndentation = 'invisible'; (default)
$wgDiscussionToolsReplyIndentation = 'bullet';

Bug: T259864
Change-Id: Icefad79630adc6ed35687498614e6a03ede1451b
2022-01-12 20:54:04 +00:00
Bartosz Dziewoński 72b9c2c6f5 Ignore some invisible nodes when looking for comment frames
Reimplement getFullyCoveredSiblings() using compareRanges(), which
checks basically the same thing, but works better and I like it more.

Bug: T297034
Change-Id: I33dc1d088bdee984064315290e378bfbfa830b10
2022-01-11 17:01:53 +00:00
Bartosz Dziewoński 8d51ce32b6 Test cases for adding replies outside of frames
Bug: T297034
Change-Id: Ida57b2638d3434c9b47b120942348ebfe0f96d37
2022-01-11 17:01:47 +00:00
Bartosz Dziewoński 8de940b587 Improve detecting transcluded comments again
Previously: 569db3603c (2020-06).

Unfortunately we've found cases where the previous implementation
doesn't work correctly, resulting in comments being added to the wrong
pages or page corruption.

Bug: T289873
Bug: T298051
Change-Id: Id867b3005ebc46906d6df852a525fcaec9e6b19b
2022-01-11 16:07:44 +00:00
Bartosz Dziewoński 492dbd7847 Fix inserting comment start markers when they're outside of any wrappers
Comment boundaries are stored as a DOM parent node plus a child index.
Because of that, inserting anything into the DOM before a comment –
such as another comment's start/end markers – would cause us to insert
subsequent comments' markers into the wrong places.

This issue didn't affect many pages, because usually any parent node
would have just one comment in it. Only pages with comment boundaries
outside of any wrappers (directly inside the root node) were broken.

Just process the list in reverse to fix this.

Bug: T298096
Change-Id: Iccffc36b71e9fcf3d72c4db2b9459d39042f7a2d
2022-01-11 16:07:37 +00:00
Bartosz Dziewoński d24b04ee71 Test case for several bugs from it.wp
Servers as another test case for partially transcluded comments,
and a test case for comment start markers placed outside of
paragraphs.

Bug: T298051
Bug: T298096
Change-Id: Id07d2f57708c037578cb653c83848490c9a15fc6
2022-01-05 23:17:51 +00:00
Bartosz Dziewoński 4da9a13c45 Test cases for partially transcluded comments
Bug: T289873
Change-Id: I115a46eb4858dccd6056534d727f88d8513b391b
2021-12-17 00:55:33 +00:00
Bartosz Dziewoński 52c09788a1 Remove unused JS version of ThreadItem#getTranscludedFrom
We're probably not going to use it again, and I don't want to make the
effort of rewriting it in Id867b3005ebc46906d6df852a525fcaec9e6b19b.

Change-Id: I0b02533f7c9b8c1b0df271e03a74063f123d0dff
2021-12-17 00:54:37 +00:00
jenkins-bot cd8f426ad4 Merge "Add missing typehints" 2021-12-02 21:13:49 +00:00
Bartosz Dziewoński f68f91e883 Set $wgUsePigLatinVariant = false while running tests
Data used for the tests assumes there are no variants for English,
and some tests fail when there are. Correct behavior with language
variants is tested using other languages.

Change-Id: I348a0ba0389c2a18644ce5e05c7f37d8f26a8c55
2021-12-01 23:25:30 +01:00
Ed Sanders 8e4f08182e Add missing typehints
Change-Id: Ia25c5bea1834a3fdd26f32a9d5ed097789329824
2021-12-01 14:57:09 +00:00
Bartosz Dziewoński 0d57aa9762 Automatic topic subscriptions (on any edit)
Bug: T284836
Change-Id: Ia42ad087218fd91a0cdd1664157d1049738e3c01
2021-11-15 22:45:42 +01:00
jenkins-bot 9fbf2b3177 Merge "Avoid splitting about-groups starting with an empty <span>" 2021-11-15 21:38:56 +00:00
jenkins-bot 2b68d4f301 Merge "Test case for splitting about-groups starting with an empty <span>" 2021-11-15 21:37:36 +00:00
Bartosz Dziewoński 83ba496919 Avoid splitting about-groups starting with an empty <span>
Usually this isn't a problem, because the comments are marked as
template-generated and we don't allow replying to them. But we had a
special case where we were trying to skip over some invisible
elements, which was causing us to skip into the middle of the
about-group in some cases. When Parsoid sees that, it serializes the
contents twice.

Bug: T290940
Change-Id: I9fe0b8d43ab874ccef371990799f77bfc46bc954
2021-11-15 16:03:38 +00:00
Bartosz Dziewoński e6de1c0462 Test case for splitting about-groups starting with an empty <span>
Bug: T290940
Change-Id: I632d351cf61980a48c0a16c4bdb3656dac83a584
2021-11-15 16:03:28 +00:00
Ed Sanders 3f5756f495 List methods not covered by unit tests in TODO sections
Change-Id: Ia06c3c726b7bc9758454aa8b2f7ea34cbada4c09
2021-11-13 15:27:38 +00:00
jenkins-bot e03820ed9d Merge "Suppress events from comments that are more than 10 minutes old" 2021-11-09 23:02:24 +00:00
jenkins-bot aeb7443715 Merge "CommentItem.php: Store timestamp object instead of string" 2021-11-09 23:02:22 +00:00
Ed Sanders 0fba9b0048 Suppress events from comments that are more than 10 minutes old
Bug: T290803
Change-Id: Ic0e23f439eef8a1b785f408d4557bec0abe9104b
2021-11-09 16:37:46 +00:00
Ed Sanders a86d308d66 CommentItem.php: Store timestamp object instead of string
We do something similar in CommentItem.js with a moment object.
The object can be converted to a string when required.

Change-Id: Id7221e9201db0d89c3b771574634c878c9515ca0
2021-11-09 16:37:45 +00:00
Ed Sanders 7c3e583bec build: Update eslint-config-wikimedia to 0.21.0
Change-Id: I72de463d5a878e555eeed0e7ce2772e1d3a46f06
2021-11-08 19:03:40 +00:00
Ed Sanders f4c12e120a Define documentable types in eslintrc instead of inline
These types can be passed a parameters to any file without
creating a dependency, so it makes more sense to allow
the globally.

Change-Id: I5504465fd997b46547642e7046993b370b85586e
2021-10-17 14:38:39 +01:00
Alexander Vorwerk 0935bb1271 MediaWikiTestCase -> MediaWikiIntegrationTestCase
MediaWikiTestCase has been renamed to MediaWikiIntegrationTestCase in 1.34.

Bug: T293043
Change-Id: I485c5c5f0376ab60cdec49e934c6e7eea8c9feb5
2021-10-12 00:40:27 +02:00
Ed Sanders 605e7322b8 eslint: Lint root folder with server rules
Change-Id: I372eef293983bff0c79ad8aa0da1c7e5d07b1e44
2021-10-07 17:37:52 +01:00
Bartosz Dziewoński c1f4668806 Change CommentParser and ImmutableRange to use offsets in codepoints instead of bytes
The PHP DOM extension measures lengths and offsets in Unicode codepoints.
Our PHP code used UTF-8 bytes, causing some offsets to be slightly off.
Now it mostly uses Unicode codepoints as well (we're forced to use bytes
in a few places, because preg_match returns offsets in bytes).

In practice, this had no visible effect to the user. It caused the
markers `<span data-mw-comment-end="..."></span>` to be placed at
the end of their container instead of the correct position when the
timestamp contained multibyte characters (e.g. "ź" in Polish); but
the correct position is usually at the end of the container anyway.

In the test cases, the only difference is placing these markers before
a trailing line break inside `<p>...</p>` tags rather than before it.

The patch also accidentally fixes another bug, where element nodes
with no children (mostly <img>) were incorrectly excluded when calling
cloneContents(), because they were treated as if they were text nodes.

Change-Id: Iccdccf1078598f4b62cab96225e9c85a4c0e93ee
2021-09-27 19:04:16 +00:00
jenkins-bot abd6c2fedd Merge "Enhance Echo user talk edit and mention notifications" 2021-09-24 02:17:57 +00:00
Bartosz Dziewoński 435b0c65c7 Enhance Echo user talk edit and mention notifications
If the user talk edit or mention coincides with exactly one new comment:
* Change the primary link to be a direct link to the comment
* Add a text snippet to notifications that don't already include one
  (user talk edits that are not new sections).

This is done for all such notifications, regardless of whether anyone
has topic subscriptions enabled.

Bug: T281590
Bug: T253082
Change-Id: I98fbca8e57845cd7c82ad533c393db953e4e5643
2021-09-20 15:05:42 +02:00
Bartosz Dziewoński a6a547f2b2 Add some tests covering ThreadItem::getHTML() and related methods
* ThreadItem::getText
* CommentItem::getBodyText (used when generating notifications)
* ThreadItem::getHTML (may soon be used in API)
* CommentItem::getBodyHTML (may soon be used in API)
* ImmutableRange::cloneContents (the common implementation for all
  of the above)

The outputs are only lightly reviewed. This is mostly meant to
document the current behavior rather than the expected behavior,
to avoid making unintentional changes while refactoring.

Change-Id: I14471ee4969aa3d0b5577d9de2a6d4462fab4d09
2021-08-24 07:54:09 +02:00
Bartosz Dziewoński ad04b24ffd Create a hidden revision tag for talk page comments
Bug: T262107
Depends-On: I21159d03eebaf46ad94f4273ba698a59b8019185
Change-Id: Iceddfaf6a4bcc5e8b5c85c8cd5638bf14aa7db03
2021-08-16 15:42:51 +00:00
Bartosz Dziewoński 47510a22f3 EventDispatcher: Fix ignoring level 3+ headings
The code (prior to d25825a754) assumed
that level 3+ headings would always follow a level 2 heading or the
placeholder heading, but we don't generate a placeholder heading if
there are no comments in section zero.

Add more tests to confirm that comments under level 3+ headings (that
are not sub-headings of level 2), and level 1 headings, are ignored
when generating notifications, and do not mess with normal headings.

Bug: T288775
Change-Id: Ic57b56752a4797cb01234f66e0ed7b849752bd70
2021-08-16 15:42:06 +00:00
Bartosz Dziewoński b46893eb7d Remove pointless uses of preserveWhiteSpace property
This DOMDocument property has no effect, because we do not use
DOMDocument methods for parsing HTML, but rather DOMUtils::parseHTML()
provided by Parsoid.

Change-Id: I1d9e73e53f2d44f41cf9dcda4f06ac8647671096
2021-08-09 23:45:48 +02:00
Bartosz Dziewoński 076242e3b4 Revert "Silence JQMIGRATE warnings when running tests"
This reverts commit f075e37303.

No longer needed after Iaf786cd0f1d870cbcf0b968b7adce616c82df3d8
in MediaWiki core, and now causes exceptions because
UNSAFE_restoreLegacyHtmlPrefilter is undefined.

Bug: T280944
Change-Id: I0dbd6fcb5dce939de334815e9fe371425cf5641f
2021-08-06 18:19:42 +00:00
jenkins-bot 10c23d0eb1 Merge "Deal with document body consistently" 2021-08-06 03:08:28 +00:00
jenkins-bot 56f4dbbf86 Merge "Test cases for interactions with events generated by base Echo" 2021-08-03 23:18:46 +00:00
jenkins-bot d14f31fd08 Merge "Improve notifications for comments posted in close succession" 2021-08-03 23:17:55 +00:00
jenkins-bot 352a4d0555 Merge "Test cases for comments posted in close succession" 2021-08-03 20:44:02 +00:00
Bartosz Dziewoński 8de8d80cde Deal with document body consistently
Use `DOMCompat::getBody( ... )` as a nicer getter than
`->getElementsByTagName( 'body' )->item( 0 )`.

Remove overly defensive checks and redundant annotations on its
return value. Since we're dealing with HTML documents throughout,
the document body is guaranteed to exist.

We previously needed some of them to convince Phan when it thought
the body may be null, but this seems to no longer be needed.

Change-Id: If7aee7b6adbfa78269c7ba28b26a6eaa21fe935b
2021-08-03 15:12:55 +02:00
jenkins-bot 1f4706a308 Merge "Recognize links to add a new topic that use Special:NewSection" 2021-08-02 17:42:30 +00:00
Bartosz Dziewoński 80704b6e80 Test cases for interactions with events generated by base Echo
Adding test cases in a separate commit to make it easier to review how
the test results change after I98fbca8e.

* For mentions, the 'mentioned-users' extra parameter is copied to our
  event (which is then used to avoid duplicate notifications).
* For user talk page edit, nothing special happens right now (we use
  the target page title to avoid duplicate notifications, but this is
  not apparent from the test case, since page titles are not present).

Bug: T281590
Bug: T253082
Change-Id: I153e7735f63f1e2643ed881281d807313cd699c3
2021-08-01 12:27:33 +02:00
Bartosz Dziewoński a5099739a6 Improve notifications for comments posted in close succession
In case 4 and case 6, no notifications are expected. In all other
cases we now get the expected notifications.

Bug: T285528
Change-Id: I9e813bb3a053bc1232783f9eae1ad75672b4fa7e
2021-08-01 12:27:33 +02:00
Bartosz Dziewoński 78cb03c471 Test cases for comments posted in close succession
Adding test cases in a separate commit to make it easier to review how
the test results change.

As expected, in every case, no notifications are generated right now.

Bug: T285528
Change-Id: I25308754112c521d2db8c54ef0c82373456d9e31
2021-08-01 12:27:33 +02:00
C. Scott Ananian 25272e7a4a Don't refer directly to PHP dom extension classes; avoid nonstandard behavior
These changes ensure that DiscussionTools is independent of DOM
library choice, and will not break if/when Parsoid switches to an
alternate (more standards-compliant) DOM library.

We run `phan` against the Dodo standards-compliant DOM library,
so this ends up flagging uses of non-standard PHP extensions to
the DOM.  These will be suppressed for now with a "Nonstandard DOM"
comment that can be grepped for, since they will eventually
will need to be rewritten or worked around.

Most frequent issues:

* Node::nodeValue and Node::textContent and Element::getAttribute()
can return null in a spec-compliant implementation.  Add `?? ''` to
make spec-compliant results consistent w/ what PHP returns.

* DOMXPath doesn't accept anything except DOMDocument.  These uses
should be replaced with DOMCompat::querySelectorAll() or similar
(which end up using DOMXPath under the covers for DOMDocument any way,
but are implemented more efficiently in a spec-compliant
implementation).

* A couple of times we have code like:
  `while ($node->firstChild!==null) { $node = $node->firstChild; }`
and phan's analysis isn't strong enough to determine that $node is still
non-null after the while.  This same issue should appear with DOMDocument
but phan doesn't complain for some reason.

One apparently legit issue:

* Node::insertBefore() is once called in a funny way which leans on
the fact that the second option is optional in PHP.  This seems to be
a workaround for an ancient PHP bug, and can probably be safely
removed.

Bug: T287611
Bug: T217867
Change-Id: I3c4f41c3819770f85d68157c9f690d650b7266a3
2021-07-30 18:15:40 -04:00
C. Scott Ananian 5203d30ea6 Use DOMCompat::newDocument() to create a new Document
For compatibility with Parsoid's document abstraction (Parsoid may
switch to an alternate DOM library in the future), don't explicitly
create a new document object using `new DOMDocument`; instead use
the Parsoid wrapper `DOMCompat::newDocument()`.  This ensures that
the Document object created will be compatible with Parsoid.

There are a number of other subtle dependencies on the PHP `dom`
extension in DiscussionTools, like explicit `instanceof` tests; those
will be tweaked in a follow-up patch
(I3c4f41c3819770f85d68157c9f690d650b7266a3) since they do not affect
correctness so long as Parsoid is aliasing Document to a subclass of
the built-in DOMDocument.  Similarly, the Phan warnings we suppress
do not cause runtime errors (because of the fixes included in
c5265341afd9efde6b54ba56dc009aab88eff83c) but phan will be happier
once the follow-up patch lands and aligns all the DOM types.

Bug: T287611
Depends-On: If0671255779571a91d3472a9d90d0f2d69dd1f7d
Change-Id: Ib98bd5b76de7a0d32a29840d1ce04379c72ef486
2021-07-30 18:15:11 -04:00
Bartosz Dziewoński d0e4aeaecb Fix notifications when new comment is under subheading
The user interface only allows you to subscribe to level 2 headings.
But we would generate events for whatever heading was the closest,
If it was e.g. level 3, no one would receive that notification.

Now we generate events for the closest level 2 heading, or we don't
generate the event at all if there isn't one (if the only headings are
of level 3 and below, or level 1, or if the comment is added before
the first heading on the page).

Bug: T286736
Change-Id: Iae99853070e353ab81c9cc29ef1d53c877adfc66
2021-07-24 05:28:10 +02:00
Bartosz Dziewoński 801b57b0f4 Add PHPUnit integration tests for EventDispatcher
Bug: T286608
Change-Id: I711483be80d455f4439e96d37844ee4552619a92
2021-07-24 05:28:04 +02:00