Commit graph

273 commits

Author SHA1 Message Date
Trevor Parscal 1e2502a726 Added some unit tests for ve.dm.SurfaceFragment
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
2012-08-17 10:48:40 -07:00
Trevor Parscal 264b97df7f There's no such thing as ghosts, just sneaky array references
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
2012-08-17 10:37:28 -07:00
Catrope 7319038ed6 Strip generated <p> tags in dataToDom
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
2012-08-16 16:09:28 -07:00
Catrope ce60b54d33 Rename fringeWhitespace to internal
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
2012-08-16 15:14:01 -07:00
Trevor Parscal c8b4a28936 Added key-sorting to make hashes referentially transparent
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
2012-08-15 11:14:44 -07:00
Catrope 6f8307d5d2 Update LinkAnnotation for Parsoid href changes
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
2012-08-14 11:03:37 -07:00
Catrope 70fa9c8aeb Recognize annotation types with multiple slashes correctly
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
2012-08-13 18:22:31 -07:00
Catrope 63e6702c52 Strip and preserve inner leading&trailing whitespace in the linear model
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
2012-08-10 17:23:53 -07:00
Catrope d93b82347b Clean up a few commas and some indentation
The commas were resulting from converting this:
var foo = 3,
	bar = 5;

to this:

foo = 3,
bar = 5;

Change-Id: I0223b34a30d947c6a51f0601727b0c3850239e66
2012-08-10 16:49:14 -07:00
Timo Tijhof 23c5b0d02c Make use of new jshint options
* 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
2012-08-10 02:50:30 +02:00
Trevor Parscal d8ee3c2c29 After much research on error objects, native = good, custom = bad
Stack traces, line numbers, etc. All the approaches I've seen are bad hacks. This is the best way to go.

Change-Id: Ib12e9d2ecfe610bcc89d046005e35cc13efa3d99
2012-08-08 10:48:53 -07:00
Trevor Parscal b4de3ead08 Throw ve.Error instead of string literals
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
2012-08-08 06:19:00 +02:00
Timo Tijhof 9b49a7ce81 Clean up: Single quotes
* 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
2012-08-07 07:02:01 +02:00
Trevor Parscal 2eb0d4e51f Javascript, unlike PHP doesn't need double quotes for \n
This just cleans up a few places where single quotes should have been used instead of double quotes.

Change-Id: I6c53652e71ab96842ed5bb41fb1e0b8c923eb25d
2012-08-07 06:49:44 +02:00
Trevor Parscal 255ce870e2 Puttin' em white-spacers where they aught'a be
function() -> function ()
){ -> ) {

Change-Id: I20a85fcf79d7aec64f7f2559e84c0279550d4eea
2012-08-06 18:52:19 -07:00
Catrope 444961af2f Kill all but one of the Parsoid compat hacks, we don't need them any more
Also remove traces of Parsoid workarounds in tests. Tests are now
passing, yay :)

Change-Id: I8a59fc92c567c3595849e3e9377ce6eb6acde280
2012-08-06 13:56:40 -07:00
Trevor Parscal 13ccb68ae1 Cleanup - all jshint conditions are now met
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
2012-08-03 18:56:04 -07:00
Timo Tijhof 88f6089952 Kranitor #1: On-boarding
'''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
2012-07-27 14:40:00 -07:00
Catrope 7f5dc887ea Do underscore/space conversion in internal link targets
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
2012-07-26 17:30:35 -07:00
Catrope 2b548a002f Get link title from href by stripping article path
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
2012-07-26 16:25:35 -07:00
Catrope 67e11ebbc3 Make VE work again with the link RDFa changes in Parsoid
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
2012-07-26 16:23:03 -07:00
Catrope 1a573480f7 Moved text node test to the correct directory
Change-Id: I6c2146cd051e9e992e6a30a2d5f83d337d454387
2012-07-25 14:49:48 -07:00
Trevor Parscal 76bac7d152 Move test files
Change-Id: Id0a0bd5b4a91f702cad34e9f5e7f2121763abffd
2012-07-25 14:35:49 -07:00