This was broken in three different ways:
* On the way in, we were applying whitespace to an array of elements
rather than the actual element, so the whitespace wasn't stored.
* Whitespace processing on the way out was skipped for aliens because
they had their own code path. Refactored this so alien openings and
regular openings share much more code, including whitespace output.
* Somewhat unrelatedly, innerPost output was broken for paragraphs
containing inline elements, because the inline elements' processing
polluted lastOuterPost. Discovered this because my test with inline
aliens also happened to be the first test of whitespace preservation
in paragraphs with inline content elements. Fixed by explicitly
skipping content nodes when outputting whitespace.
Fixed these issues and added a test case.
Change-Id: I8edb61a008e60ace886b1a841b3417682ec39c32
* For the most common case:
- replace ve.extendClass with ve.inheritClass (chose slightly
different names to detect usage of the old/new one, and I
like 'inherit' better).
- move it up to below the constructor, see doc block for why.
* Cases where more than 2 arguments were passed to
ve.extendClass are handled differently depending on the case.
In case of a longer inheritance tree, the other arguments
could be omitted (like in "ve.ce.FooBar, ve.FooBar,
ve.Bar". ve.ce.FooBar only needs to inherit from ve.FooBar,
because ve.ce.FooBar inherits from ve.Bar).
In the case of where it previously had two mixins with
ve.extendClass(), either one becomes inheritClass and one
a mixin, both to mixinClass().
No visible changes should come from this commit as the
instances still all have the same visible properties in the
end. No more or less than before.
* Misc.:
- Be consistent in calling parent constructors in the
same order as the inheritance.
- Add missing @extends and @param documentation.
- Replace invalid {Integer} type hint with {Number}.
- Consistent doc comments order:
@class, @abstract, @constructor, @extends, @params.
- Fix indentation errors
A fairly common mistake was a superfluous space before the
identifier on the assignment line directly below the
documentation comment.
$ ack "^ [^*]" --js modules/ve
- Typo "Inhertiance" -> "Inheritance".
- Replacing the other confusing comment "Inheritance" (inside
the constructor) with "Parent constructor".
- Add missing @abstract for ve.ui.Tool.
- Corrected ve.FormatDropdownTool to ve.ui.FormatDropdownTool.js
- Add function names to all @constructor functions. Now that we
have inheritance it is important and useful to have these
functions not be anonymous.
Example of debug shot: http://cl.ly/image/1j3c160w3D45
Makes the difference between
< documentNode;
> ve_dm_DocumentNode
...
: ve_dm_BranchNode
...
: ve_dm_Node
...
: ve_dm_Node
...
: Object
...
without names (current situation):
< documentNode;
> Object
...
: Object
...
: Object
...
: Object
...
: Object
...
though before this commit, it really looks like this
(flattened since ve.extendClass really did a mixin):
< documentNode;
> Object
...
...
...
Pattern in Sublime (case-sensitive) to find nameless
constructor functions:
"^ve\..*\.([A-Z])([^\.]+) = function \("
Change-Id: Iab763954fb8cf375900d7a9a92dec1c755d5407e
Introduced the ve.AnnotationSet class to manage sets of annotations. This
is a generalization of ve.OrderedHashSet, a class that manages a set
using an array and an object keyed by hash.
Converted everything that stores, tracks or passes around annotations to
use ve.AnnotationSet. In particular, this means the linear model now
contains AnnotationSets instead of hash-keyed objects.
This allows us to maintain the order of annotations in the linear model,
and will help fix bugs with annotation ordering and splitting.
Change-Id: I50975b0a95f4cc33017a0b59fdede9ed1eff0124
[jshint]
ce/ve.ce.Surface.js: line 670, col 9, Too many var statements.
ce/ve.ce.Surface.js: line 695, col 6, Missing semicolon.
ce/ve.ce.Surface.js: line 726, col 22, Expected '===' and instead saw '=='.
ce/ve.ce.Surface.js: line 726, col 41, Expected '===' and instead saw '=='.
ce/ve.ce.Surface.js: line 733, col 13, Too many var statements.
ce/ve.ce.Surface.js: line 734, col 24, Expected '===' and instead saw '=='.
ce/ve.ce.Surface.js: line 1013, col 13, Too many var statements.
ce/ve.ce.Surface.js: line 1019, col 17, Too many var statements.
ce/ve.ce.Surface.js: line 1023, col 18, Too many ar statements.
ce/ve.ce.Surface.js: line 1027, col 13, Too many var statements.
dm/annotations/ve.dm.LinkAnnotation.js: line 70, col 52, Insecure '.'.
dm/ve.dm.Converter.js: line 383, col 29, Empty block.
dm/ve.dm.Converter.js: line 423, col 33, Empty block.
Commands:
* jshint .
* ack '(if|else|function|switch|for|while)\('
* Sublime Text 2:
Find(*): (if|else|function|switch|for|while)\(
Replace: $1 (
* ack ' ' -Q # double spaces, except in certain comments
Change-Id: I8e34bf2924bc8688fdf8acef08bbc4f6707e93be
This will cause ve.dm.SurfaceFragment.prototype.insertContent() to place
the selection after the insertion as well.
Change-Id: Ifa7e627daceb90408422eb58c110d475f34ba1e2
This commit fully utilizes all four positions in the internal.whitespace
array. Outer whitespace is now preserved as well, and is duplicated
either in the adjacent sibling (one node's outerPost is the next
sibling's outerPre) or in the parent (a branch node's innerPre is its
first child's outerPre, and its innerPost is its last child's
outerPost). Before restoring saved whitespace, we check that these two
agree with each other, and if they disagree we assume the user has been
moving stuff around and don't restore any whitespace in that spot. The
whitespace at the very beginning and the very end of the document (i.e.
the first node's outerPre and the last node's outerPost) isn't
duplicated anywhere, nor is inner whitespace in content nodes.
The basic outline of the implementation is:
* When we encounter whitespace, strip it and store it in the previous
node's outerPost. Also store it in nextWhitespace so we can put it in
the next node's outerPre once we encounter that node.
* When we encounter whitespace in wrapped bare text, we don't know in
advance if it's gonna be succeeded by more non-whitespace (in which
case it needs to be output verbatim), or not (in which case it's
leading whitespace and needs to be stripped and stored). The fact that
annotations are nodes in HTML makes this trickier. So we write the
whitespace to the temporary linmod and store it in wrappedWhitespace,
then if it turns out to be trailing whitespace we take it back out of
the data array and record it the usual way.
* Because text nodes can contain any combination of leading whitespace
actual text and trailing whitespace, and because we may or may not
already have opened a wrapping paragraph, there are a lot of different
combinations to handle. We handle all of them but the resulting code
is pretty dense and verbose.
More low-level list of changes:
In getDataFromDom():
* Added helper function addWhitespace() for storing whitespace for an
element
* Added helper function processNextWhitespace() for processing any
whitespace passed on from the previous node via the nextWhitespace var
* Rename paragraph to wrappingParagraph. Make wrapping default to
alreadyWrapped so we can simplify wrapping||alreadyWrapped and
!wrapping&&!alreadyWrapped. Add wrappingIsOurs to track whether the
wrapping originated in this recursion level (needed for deciding when
to close the wrapper).
* Add prevElement to track the previous element so we can propagate
whitespace to it, and nextWhitespace so we can propagate whitespace to
the next element.
* Remove previous newline stripping hacks
* Integrate the logic for wrapping bare content with the outer
whitespace preservation code
* Remove wrapperElement, no longer needed because we have a dedicated
variable for the wrapping paragraph now and what was previously inner
whitespace preservation for wrapper paragraphs is now covered by the
outer whitespace preservation code.
In getDomFromData():
* Reinsert whitespace where appropriate
** outerPre is inserted when opening the element
** This covers outerPost as well except for the last child's outerPost,
which is handled as the parent's innerPost when closing the parent.
** innerPre and innerPost are inserted when closing the element. Care is
taken not to insert these if they're duplicates of something else.
* Propagate each node's outerPost to the next node (either the next
sibling or the parent) using parentDomElement.lastOuterPost. We can't
get this using .lastChild because we will have destroyed that child's
.veInternal by then, and we can't tell whether a node will be its
parent's last child when we process it (all other processing,
including first child handling is done when processing the node itself,
but this cannot be).
* Special handling is needed for the last node's outerPost, which ends
up in the container's .lastOuterPost property.
Tests:
* Allow .html to be null in data<->DOM converter tests. This indicates
that the test is a one-way data->DOM test, not a DOM->data->DOM
round-trip test. The data will be converted to HTML and checked
against .normalizedHtml
* Update existing tests as needed
* Add tests for outer whitespace preservation and storage
* Add test for squashing of whitespace in case of disagreement (this
requires .html=null)
Change-Id: I4db4fe372a421182e80a2535657af7784ff15f95
data-mw-gc is ancient and unused. We do need to detect and alienate
generated nodes, but that is now based on RDFa types. Removing the
data-mw-gc stuff for now because it doesn't work anyway, will replace it
with proper detection later.
Replaced instances of data-mw-gc in the test suite with unregistered
node types.
Change-Id: If3f5898d382a436fa57929013264c53af5e840ba
Also:
* Made a fragment with a null range become a null fragment
* Fixed incorrect order of arguments for binding a handler to transact event
* Added getters for surface, document and range
* Fixed several instances of passing a document instead of a surface into the constructor of a new surface fragment
* Fixed closest mode in expandRange - needed to check if parent existed before checking for it's type
* Fixed uses of ve.Transaction (doesn't exist) that were supposed to be uses of ve.dm.Transaction (does exist)
Change-Id: Ide13d9d2d1637399188c98c2e8b6e0826caeecc4
When a document is created, it should take it upon itself to make sure it has a new reference to the data using slice, not place this on the caller. Callers that do not use slice will often find strange and mysterious things going on and not know why. The real reason is that multiple documents sharing a reference to the same data array leads to seriously messed up behavior.
Change-Id: Ic4e25fcd9bf3f41a805003520a8f38e2768f5dbf
domToData wraps bare content in paragraph elements, which were then
converted to <p> tags by domToData. With this fix, HTML with "missing"
<p> tags actually round-trips through the editor correctly now, rather
than having <p> tags added wherever VE believes they should exist.
* Mark generated paragraph elements with .internal.generated = 'wrapper'
** This signifies the wrapper was generated but its contents were not,
so the right thing to do when converting back to HTML is to remove
the wrapper and keep the contents. We might want to use other values
of generated in the future.
* Unwrap nodes with generated=wrapper when converting to HTML
Tests:
* Add 'generated': 'wrapper' as appropriate. Only affects 1 test
* Remove 'normalizedHtml' for this test because it is no longer needed
** Need to keep 'normalizedHtml' for now because we normalize hrefs
* Eventually the main example should test bare content, but that
requires touching a lot of stuff. The main example could use some
beefing up anyway.
Change-Id: I277ad5fe3f64e07c1bbf49007d6bbaecc90b7466
This allows us to put other internal data in there in the future. Also
passing it through the Node constructor properly now.
* ve.dm.Node
** Rename fringeWhitespace property to internal
** Add internal parameter to constructor
** Remove setFringeWhitespace()
* Increase the number of parameters passed through by ve.Factory to 3
* Pass through .internal from linmod to nodeFactory in ve.dm.Document
* ve.dm.Converter
** Rename .fringeWhitespace to .internal.whitespace and make it an array
** Store a temporary reference to .internal in domElement.veInternal
* Add internal to all node constructors except TextNode
Tests:
* Update for fringeWhitespace->internal rename
* Add third parameter to ve.Factory tests
* Add .internal to getNodeTreeSummary
Change-Id: If20c0bb78fee3efa55f72e51e7fc261283358de7
To do this, we are using the replacer callback of JSON.stringify, which is supported by all of our target browsers including IE8. We are also leaning on Object.keys and Array.reduce, the latter of which required adding a new fallback implementation for some browsers which do not support it yet.
Change-Id: Ifa285ca3da4d94d962464f09414591532bbea79c
Because the Parsoid prefix format changed from /mw:Foo to /mw/Foo , the
href format for internal links has changed from "/Foo" to "Foo". So the
href is now simply the title, except that it may be preceded by one or
more "../" if the title of the page we're on contains a '/'.
So instead of stripping the leading slash from internal link hrefs and
putting it back on the way out, only strip any leading "../"s and dump
the titles directly into the hrefs on the way out.
Also update the link test case for this, and add a test case for the ../
stripping.
Change-Id: I3e0bdde20f22cda34eb45fc351de5e780419b6a2
Annotation types with more than one slash such as 'link/ExtLink/URL'
weren't being processed correctly because .split( '/', 2 ) throws away
everything after the second slash. Instead, don't pass a limit to
.split(); the code for reconstructing a slash-separated string from
multiple components was already in place.
Also add test cases for URL links and numbered links.
(Do you like the lines-of-code to lines-of-test ratio in this commit,
Trevor? ;) )
Change-Id: I7add87396447a01b1c23a4f9bfd63d2e8fd861ce
This makes things like
== Foo ==
* Bar
render without the leading and trailing spaces, while still
round-tripping those spaces.
* Added a .fringeWhitespace property to the linear model and ve.dm.Node
** Object containing innerPre, innerPost, outerPre, outerPost
** Only inner* are used right now, outer* are planned for future use
** Like .attributes , it's suppressed if it's an empty object
* In getDataFromDom():
** Store the stripped whitespace in .fringeWhitespace
** Move emptiness check up: empty elements with .fringeWhitespace have
to be preserved
** Move paragraph wrapping up: .fringeWhitespace has to be applied to
the generated paragraph, not its parent
** Add wrapperElement to keep track of the element .fringeWhitespace has
to be added to; this is either dataElement or the generated paragraph
or nothing, but we can't modify dataElement because it's used later
* In getDomFromData():
** When processing an opening, store the fringeWhitespace data in the
generated DOM node
** When processing a closing, add the stored whitespace back in
* In the ve.dm.Document constructor, pass through .fringeWhitespace from
the linear model data to the generated nodes
Tests:
* Change one existing test case to account for this change
* Add three new test cases for this behavior
* Add normalizedHtml field so I can test behavior with bare content
Change-Id: I0411544652dd72b923c831c495d69ee4322a2c14
* Restricting "camelcase":
No changes, we were passing all of these already
* Explicitly unrestricting "forin" and "plusplus"
These are off by default in node-jshint, but some distro of jshint
and editors that use their own wrapper around jshint instead of
node-jshint (Eclipse?) may have different defaults. Therefor
setting them to false explicitly. This also serves as a reminder
for the future so we'll always know we don't pass that, in case
we would want to change that.
* Fix order ("quotemark" before "regexp")
* Restricting "unused"
We're not passing all of this, which is why I've set it to false
for now. But I did put it in .jshintrc as placeholder.
I've fixed most of them, there's some left where there is no clean
solution.
* While at it fix a few issues:
- Unused variables ($target, $window)
- Bad practices (using jQuery context for find instead of creation)
- Redundant /*global */ comments
- Parameters that are not used and don't have documentation either
- Lines longer than 100 chars @ 4 spaces/tab
* Note:
- ve.ce.Surface.prototype.onChange takes two arguments but never
uses the former. And even the second one can be null/undefined.
Aside from that, the .change() function emits
another event for the transaction already. Looks like this
should be refactored a bit, two more separated events probably
or one that is actually used better.
- Also cleaned up a lot of comments, some of which were missing,
others were incorrect
- Reworked the contentChange event so we are no longer using the
word new as an object key; expanded a complex object into multiple
arguments being passed through the event to make it easier to work
with and document
Change-Id: I8490815a508c6c379d5f9a743bb4aefd14576aa6
Stack traces, line numbers, etc. All the approaches I've seen are bad hacks. This is the best way to go.
Change-Id: Ib12e9d2ecfe610bcc89d046005e35cc13efa3d99
Throwing strings is bad because it doesn't include a lot of important
information that an error object does, such as a stack trace or where
the error was actually thrown from.
ve.Error inherits directly from Error. In the future we may create
more specific subclasses and/or do custom stuff.
Some interesting reading on the subject:
* http://www.devthought.com/2011/12/22/a-string-is-not-an-error/
Change-Id: Ib7c568a1dcb98abac44c6c146e84dde5315b2826
* There were only 3 files with single quotes, fixed them all.
* Added option to .jshintrc (be sure to use the latest version of
node-jshint since this is a fairly new addition to the library).
Change-Id: I8bf8895ce56bf86e3bed244279a9d32269e44763
This just cleans up a few places where single quotes should have been used instead of double quotes.
Change-Id: I6c53652e71ab96842ed5bb41fb1e0b8c923eb25d
Also:
* Removed a lot of dead code in Surface that was used in the now dead and gone sandbox.
* Changed from throwing an exception when calling getBalancedData on a range that produces no results from selectNodes to just returning []
Change-Id: Icf27094724eae5b90eec21308f9e26afe877e3ee
'''Kranitor commits''' are commits by Krinkle with his janitor hat on.
Must never contain functional changes mixed with miscellaneous changes.
.gitignore:
* Add .DS_Store to the ignore list so that browsing the directories
on Mac OS X, will not add these files to the list of untracked
files.
* Fix missing newline at end of file
.jshintrc
* raises -> throws
* +module (QUnit.module)
* remove 'Node' (as of node-jshint 1.7.2 this is now part of
'browser:true', as it should be)
Authors:
* Adding myself
MWExtension/VisualEditor.php
* Fix default value of wgVisualEditorParsoidURL to not
point to the experimental instance in WMF Labs.
Issues:
* ve.ce.TextNode:
- Fix TODO: Don't perform a useless clone of an already-jQuerified object.
- Use .html() to set html content instead of encapsulating between
two strings. This is slightly faster but more importantly safer,
and prevents situations where the resulting jQuery collection
actually contains 2 elements instead of 1, thus messing up
what .contents() is iterating over.
* ve.ce.Document.test.js
- Fix: ReferenceError: assert is not defined
* ve.dm.Document.test.js
- Fix: ReferenceError: assert is not defined
* ve.dm.Transaction.test.js
- Fix: ReferenceError: assert is not defined
* ve.dm.TransactionProcessor.test.js
- Fix: ReferenceError: assert is not defined
* ext.visualEditor.viewPageTarget
- Missing dependency on 'mediawiki.Title'
Code conventions / Misc cleanup
* Various JSHint warnings.
* Whitespace
* jQuery(): Use '<tag>' for element creation,
use '<valid><xml/></valid>' for parsing
* Use the default operator instead of ternary when the condition and
first value are the same.
x = foo ? foo : bar; -> x = foo || bar;
Because contrary to some programming language (PHP...), in JS the
default operator does not enforce a boolean result but returns the
original value, hence it being called the 'default' operator, as
opposed to the 'or' operator.
* No need to call addClass() twice, it takes a space-separated list
(jQuery splits by space and adds if needed)
* Use .on( event[, selector], fn ) instead of the deprecated
routers to it such as .bind(), .delegate() and .live().
All these three are now built-in and fully compatible with .on()
* Add 'XXX:' comments for suspicious code that I don't want to change
as part of a clean up commit.
* Remove unused variables (several var x = this; where x was not
used anywhere, possibly from boilerplate copy/paste)
* Follows-up Trevor's commit that converts test suites to the new
QUnit format. Also removed the globals since we no longer use those
any more.
Change-Id: I7e37c9bff812e371c7f65a6fd85d9e2af3e0a22f
Convert underscores in the href attribute to spaces in the linear model,
and back to underscores when going back to HTML. This ensures the link
targets displayed to and edited by the user look nice
Change-Id: I4855fce28ad8b724284c53881abc7b99b59b9079
This means we don't have to rely on data-rt.sHref. It also means that
we'll now be showing the canonical link target in the link inspector
rather than the link target as entered by the user, but that's fine.
Also change test to have href differ from sHref to show that we use
href.
Change-Id: Idabdbf2579663ef1efb47d6a73f39743c9f64f3b
This is ugly but makes things work again. I intend to clean this up once
we have a better attribute API
* Recognize mw:WikiLink, mw:SimpleWikiLink, mw:ExtLink,
mw:NumberedExtLink and mw:UrlLink
* Support is incomplete because we can't get to the annotation text with
the current API
* Preserve all unhandled attributes rather than special-casing data-mw
* Update remaining code using data-mw (sHref and stx extraction) to
account for the data-mw -> data-rt rename
* Update tests accordingly
Change-Id: Ia13d3008a6d4cdc8828f9acda5aa797566bc597f
