`tagName` is only defined on Element, and it returns its tag name.
`nodeName` is defined on Node, and it returns the tag name for Elements,
and a string like '#text' or '#document-fragment' for other types.
We were using both, which made it harder to reason about what types
we're dealing with.
Change-Id: I8e621e5872bdf78c84ec553cfbfcdbf0192f0589
Added in 76289cdf73,
should no longer be needed since we switch to Parsoid's
HTML parser in 3e6ab2c4d2.
Change-Id: Ic0b7ed8089b71f2338e604f68d547759e069f0b2
Raise scope of pageThreads variable (renamed from `result`)
so we don't have to re-bind when it is updated, and use a
flag to ensure we only bind the handlers once.
Bug: T302810
Change-Id: I0a5b122b8e3f656f28f3dd4fa6aca8e7839be06b
Links are generated to:
* Single comments, by a user or by a notification
* Potentially multiple comments (new comments since T),
usually just by notifications.
If in either case no comments are found to highlight, warn the
user so they get some visual feedback that the notification worked
as expected, but that the comment was just deleted since.
Bug: T301602
Change-Id: I090c904481cf6f3e9f964fa43b10001e75b0bc90
Also special-case thumbnail wrappers generated by
MediaTransformOutput::linkWrap, for compatibility with
TimedMediaHandler.
Bug: T301427
Bug: T302296
Change-Id: I7f48d8b2261507c5a33526c54109f5187d062ed3
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
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
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
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
(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
Only show the reload prompt if the 'disappeared' error is detected
when the reply widget is loading. Don't show it if we failed
to save, as it suggests you will not lose content if you reload.
Change-Id: I106f274e180dc97b540d729e31aae575c43f29f0
It's an arbitrary limit, it seems harmless to relax it to support the
use case in the task, even if it's weird.
Bug: T300949
Change-Id: I7c895c7019726758bbae3183b9c3ecbd9eabcf38
Clearing the widget removes all the surfaces, meaning there are
no doc-state variables to cleanup. Switch the order of these
two calls.
Change-Id: I6c095a171096cd700ce4cd31b08fa3b982ab2401
* Document all methods
* Rename comment to threadItem
* Use this.threadItem instead of passing in identical
threadItem in various methods.
* Don't pass threadItem to ReplyWidget as we already
pass the whole CommentController.
Change-Id: If9aad0bcf9f0e4ebf3342b75631ddac8b57f7d87
* 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
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
In PHP, use DOMCompat::getClassList(), provided by Parsoid.
In JS, use `.classList`, available in all supported browsers.
This may fix some bugs where we were incorrectly checking for exactly
one class. The change in isOurGeneratedNode() is needed for
Ib2fa40c5fa389572b0e88ef558728fa06e3621b0.
Change-Id: Ia28d31678fd3d617b69280c4b7857755300fa515
Wikis like jawiki will then be able to customize the message to include
honorifics as they think appropriate.
Bug: T268588
Change-Id: I213fb9fd0a9ed6592ce3548a5b2c3b11a55c1abc
The following values for configuration variables are supported:
$wgDiscussionToolsReplyIndentation = 'invisible'; (default)
$wgDiscussionToolsReplyIndentation = 'bullet';
Bug: T259864
Change-Id: Icefad79630adc6ed35687498614e6a03ede1451b
Reimplement getFullyCoveredSiblings() using compareRanges(), which
checks basically the same thing, but works better and I like it more.
Bug: T297034
Change-Id: I33dc1d088bdee984064315290e378bfbfa830b10
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
For logged out users we store their test state and an anonymous
identifier in local storage. So long as the test is enabled, we include
these in any logging that occurs.
This is done entirely client-side, to avoid any cache issues caused by
state depending on cookies from PHP for logged out users.
Bug: T291307
Change-Id: Ib39e2f2146cdfdac9df5690ee3de75718f0f2731
To better support adding more links to the buttons group:
* Only disable the 'reply' link when the reply tool is open,
not all links in the button group. If future links need
to be disabled (e.g. 'edit') they can be done on a per-case
basis.
* Add a 'separator' class for adding more links.
* Don't hide 'reply' link when in use. This feature was added
very early on (I03a98495) and a long time before we started
greying-out all reply links when the tool was open (I3bb5ad8e7).
It would also lead to a strange display if there were
other links in the button group. Instead change the link to
black to indicate it is in use.
Change-Id: I23cc8d305d1d1c323cb69081749443f6e19de6a4
Set `overflow: hidden;` on the heading element to establish a new
block formatting context, which will contain floating elements.
Most skins already do this, but not Timeless.
https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Block_formatting_context
Bug: T298069
Change-Id: Ic7afd82095b7440cf8b61b3f3fd0085e755a773e
This matches the PHP implementation and was only used in
one place (when fetching authors in new topic tool).
Change-Id: I9d9e774616112e8dc6ab4919846e3abecc24553d
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
Follow-up to bfe6a36514.
We should only check the transclusion data for `comment.name` when
the data for `comment.id` is undefined, not when it's false.
Bug: T297850
Change-Id: Ia47462a7727edd551db88ec18b5b07017f669d2b
This was a logical merge conflict between 4912a1bf5c and 9ded06a655
that I didn't notice when rebasing the latter. Waiting for the
'mobile.init' module to load changes the order of operations if we're
highlighting the new comment after reloading a page, which we also do
after creating new talk page with the new topic tool.
Bug: T295945
Change-Id: Ief4810cc67c1bda9256585da1e2140b3a222dc5e
When our interface initialized on a page that the current user
recently edited (using the reply tool, the full-page source editor,
or any other way), check if any new automatic topic subscriptions
were added and update the interface to reflect that.
This requires doing some API requests after the page is loaded,
because adding auto-subscriptions happens asynchronously in a
DeferredUpdate (potentially after the user is already viewing
the page with their comment saved), and depends on the contents
of the edit.
(When using the reply tool, we could avoid this API request and
replicate the logic, but that's not implemented in this commit
to keep it simple.)
Bug: T284836
Change-Id: Ic0fabda0de4ebbc5e424f49641e6b03ebb4b7e6a
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
This is the minimal fix, they still look out of place, but they don't
break layout of the whole page.
Bug: T292241
Change-Id: I1538962dd266fb455051e4dfe2680e47c4d1bb4c
* Use featuresEnabled.newtopictool to decide when new topic
links might exist on the page, instead of relying on
`#ca-addsection` existing. Change the logic of that feature
flag to check if __NONEWSECTIONLINK__ is on the page.
* Render the add-topic button locally in a hook to replace
the one suppressed by onMinervaNeueTalkPageOverlay. Do so
whenever the newtopictool feature is enabled (see above).
Bug: T270537
Change-Id: I3e3f7403b3b86bb84fcb75a8833919512519b70f
When this code was added in 5feb69612f,
it was inside the `if ( this.modeTabSelect ) { … }`, because switching
was an experimental option and could have been unavailable.
Switching is always available now, but the `modeTabSelect` widget
doesn't exist in the mobile version (because switching interface is a
part of the toolbar there), so this line should be outside the `if`.
Bug: T270536
Change-Id: I7c5959281e77558bfe82bd39d38ba4dda36d590c
MobileFrontend does not use the 'wikipage.content' hook, and its
interface will not re-initialize properly after we update the page
contents with the new comment. Reload the whole page instead.
Bug: T270536
Change-Id: I3f81e4d77faed367606e47678b8896051982359d
When an exception is thrown by our code elsewhere, and caught by the
promise mechanisms, it bubbles up here in the `code` variable, and
`data` is undefined.
Change-Id: I8a0ac49d22d254f353797fc8978871502ae8b9de
This causes the username to be selected on mobile, when
we want to place the cursor after it.
Bug: T294616
Change-Id: I29012ecd04cd553bf78ffff477babafacfeabb31
`this.limit` is never set, which mean the API default of 10
was used.
The client limit is actually `defaultLimit` which is set to 8.
Double this so we can filter blocked users and still probably
have some results.
Change-Id: I8184fa0ce1527280f4503bcf638372421287f51a
Such users will never be able to reply to you on most pages,
so we shouldn't suggest pinging them. The may be able to reply
on their own user talk page, but in that case they will be
included in localUsers.
In the best case they just clog up your search results, in the
worst case they are offensive names which haven't yet been hidden.
Bug: T294783
Change-Id: I2445ed6dc98c10f8580b2c36106dd3e98bb876d6
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
The current experience (implemented in T282789) is unobtrusive and
maybe even helpful.
This reverts commit 35e97c24fe.
Bug: T281009
Change-Id: I0514e8f8960600edf12b51fff6de305e46cf8c34
Normal link: `<a>reply</a>`
Google Translate: `<a><font><font>reply</font></font></a>`
Using jQuery's `closest` method to make sure we go up if the event
target isn't the data-mw-comment element should avoid problems.
Bug: T245563
Change-Id: I19ffb9a5b91617b98b0f00e4d185c01bcde093b0
Since Id9afb2dd0212e4b871bb6a7a9d8762e1bcb81d6a included in core since
MediaWiki 1.38 the uppercase of the first character of the parameter
auprefix is not needed anymore.
Bug: T291339
Depends-On: Id9afb2dd0212e4b871bb6a7a9d8762e1bcb81d6a
Change-Id: Ic14ca9c9c61d2a50bdbaff50b56302a60ed17a96
It was previously applied on the whole wrapper of the anon
warning, including the buttons. In some cases (on mobile, and
on some sites with weird customized site CSS), this causes the
buttons to lose some styling. Apply it only on the label.
Follow-up to 522b7932d7.
Bug: T270536
Bug: T291000
Change-Id: Ia4bc99fc219a80efbf46a7cc196ea29720a34de8
Currently the message appears too often when the user is not really
intending to interact with the widget.
Bug: T287901
Bug: T288316
Change-Id: I38b4c0b8817b9a9238fb6adc91ab2d1231650eff
Notifications are bundled by section, so instead of linking to the
comment, link to the section.
Additionally, add a parameter to the URL listing all the comment IDs
from the bundle, and highlight them all and scroll to the topmost one.
Having to handle both URL fragments and URL query parameters makes
this code kind of a mess :(
Also, some unexpected changes became necessary:
* EventDispatcher.php: Store the section title in events using
HeadingItem::getLinkableTitle() instead of ThreadItem::getText().
The result is mostly the same, except in case of wacky markup like
images or extension tags. We can more reliably use it to link to the
section on the page, and we already use getLinkableTitle() when
generating edit summaries in the reply tool for this reason.
* dt.init.less: Change the mix-blend-mode for the highlights from
'multiply' to 'darken', so that multiple overlapping highlights do
not look more opaque. This affects how the highlights look on
non-white backgrounds and images (they're less blue, and on darker
backgrounds entirely invisible), but it seems worth it.
Bug: T286620
Change-Id: I21bb5c003abc2747f0350d3f3af558dfb55693e9
This was copied from somewhere else (probably VisualEditor), but it is
not needed here.
Using the multipart/form-data encoding is beneficial when sending long
binary data (such as the compressed HTML VisualEditor sends), and is
also required when uploading files, but it is not helpful when sending
a short query like we do here.
Also rename a variable.
Change-Id: I9bcce2ce1ca7c218e4cd147960d1070dd23ea9fa
This will avoid a flash of the empty-state while we're reloading the
page to get new tabs.
Refactor out the new topic controller's clear behavior from its teardown
behavior, so we can still wipe out the storage when redirecting.
Bug: T288314
Bug: T288320
Change-Id: I6a5313b5e5b3bc9925e5cdaea04d8fbd3dc796af
This includes the dtrepliedto URL functionality from
I3f81e4d77faed367606e47678b8896051982359d.
Bug: T274831
Bug: T274832
Bug: T277329
Change-Id: I035d04f30c8312b0cb42902d3bf940df1482ffb3
This is the same method as used by VisualEditor
(ve.init.mw.DesktopArticleTarget.prototype.replacePageContent).
Bug: T275698
Change-Id: Idcf7c79b8d5565b0ae36c6e9d42b66662c1acc8d
* Move getTitleFromUrl() from parser to utils. It's a generic method,
the PHP equivalent is already in utils.
Bug: T277371
Change-Id: Id960e5f60af02bdeb0a3a68f43b7a695eb035139
I noticed that the NWE new section editor would open sometimes even
though we attempt to prevent it. This way seems to work more reliably.
Change-Id: I0ce1640b8c1dd098bf3f0d41dc4fdc276a0c5fd5
Problems with the current setup:
* Each CommentController must have exactly one link.
For T277371 we want multiple, and for T282205 we might want zero.
* CommentController objects must be constructed immediately.
They are implemented to make this pretty fast, but it's still
unnecessary work to do on page load.
* Only one link may be activated at a time, and activating one affects
the styling of others, so CommentController has to use global state
to check if it can set up and to update them.
Instead introduce ReplyLinksController, which knows about all reply
links and which one is active, and emits events that allow
CommentControllers to be constructed on demand.
Change-Id: Iabdeded2e71e598ae78703a6ff9410d0cfba397c
Considerations:
* Using the same edit notices as VisualEditor, except 'anoneditwarning'
* No extra frame/styling is added (on Wikimedia wikis, the notices
often already have them)
* 'talkpagetext' message is not shown (on Wikimedia wikis, they are
mostly about signing your posts with tildes, which is not necessary)
Bug: T269033
Change-Id: Idc5ff29f093c75a14c3a3479888295d5bf630f6d
The .ve-init-mw-target-surface class is no longer added since
Ic5320f6747907542285674d386c7a59c9e857f0a.
Also apply changes similar to I8d20a830dc48f6a098b0f9e9a7c7c1656de0fe56
to avoid potential styling issues with nested surfaces.
Bug: T284567
Change-Id: I58a49f0137e8804fbd73de20048eb2ffdbbfbe77
Previously, our highlights were placed in a node at the end of
the page, and positioned absolutely in relation to the whole
page. Now we insert the highlight in the DOM near the comment,
and position it in relation to that.
This way it remains positioned correctly when the page shifts
(e.g. collapsing the table of contents), and disappears when
the page content is hidden (e.g. opening visual editor).
Bug: T281471
Change-Id: I60afc4b94b2e23376105638542563e595a1811d9
The modifier crashes if endContainer is a <p>/<dd>/<li> node, and
it often is if we try to exclude the end marker from the range.
It doesn't matter for T281471, so let's just leave it that way.
(This is also more similar to the range produced by our parser.)
Follow-up to c4ba8e921a. No idea how
I missed this when testing that patch.
Also, improve comments.
Bug: T281471
Change-Id: If6aba34acf29c37d06fb0ca92547f78b58695597
This produces nicer results when we call Range#getBoundingClientRect
on them later, when drawing comment highlights (using Firefox).
I'm not sure how this will affect everything in modifier, but it
doesn't seem to be causing issues. If it does cause trouble, then we
should instead adjust the range this way in controller#highlight.
Bug: T281471
Change-Id: I6f204b858990023f42f564cca7a2d24d322f872e
Per Manuel Arostegui in T263817#7033384. The limit is 5000.
(I picked it arbitrarily, there's no real rationale for it.)
Also log a warning when any user reaches half of the limit,
so that we might make a decision about changing this mechanism
before it starts affecting users. Maybe at that time we'll
have data to show that it's safe to remove the limit.
Bug: T263817
Change-Id: I18a8ee0ad7383759229c5721d5253fb591457d4d
The issue occurred when replying to a comment consisting of multiple
list items, starting with a <dt> (instead of the expected <dd>), so
that the comment is considered to be unindented.
Modifier tried to add the reply directly inside the list (<dl>) rather
than inside the last list item (<dt>), which caused it to be confused
about indentation levels and try to un-indent more times than there
were indentations.
The simplest solution, given the existing code, is to add the reply
outside the list instead, in a new list. This results in a "list gap"
(<dl><dt>...</dt><dd>...</dd></dl><dl><dd>...</dd></dl>), but I think
it's acceptable for this rare case.
There are separate tests cases for old Parser and for Parsoid HTML,
because they parse the original wikitext differently (with the old
Parser producing HTML with a list gap too).
Bug: T279445
Change-Id: Ie0ee960e7090cf051ee547b480c980e9530eda51
If curLevel or desiredLevel are calculated incorrectly, this loop
could never end.
In JS, something would throw an exception before going infinite, but
PHP is happy to trot along despite accessing properties of null and
attaching multiple children to a document node.
Bug: T279445
Change-Id: I1784f550ec3a23dcded4f2b1def97e51cb414b7b
We added it because the initial designs for the subscribe action were
much easier to implement like this, and topic "containers" (T269950)
would have required it.
However, the latest design of the subscribe action will not need it
(T279149), and topic containers are still very far away, so let's
remove it for now.
Bug: T280433
Change-Id: I21a23e9bea43f24d265750926fbd62b99038d3f1
As of 7ad6328223, we also use this data
to check whether comments exist on the page, not only whether they're
transcluded.
Follow-up to 42ce942c86.
Bug: T275821
Bug: T273413
Change-Id: I95eb85354e7b84cc10ab703d28315d0667696f4c
