This repository has been archived by the owner on Jun 15, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 245
Fix breaking out of <p> when deleting across paragraphs in FF #97
Merged
Merged
Changes from 20 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
def0e57
Cleanup code
theefer 794fd59
Remove custom insertHTML command, cleanup to be done in formatter
theefer 7b16a23
Drop empty-when-deleting patch, to be replace with formatter
theefer d09d6b5
WIP enforce p elements
theefer d7a4a4a
Move things in their right place
theefer 2034146
Merge branch 'master' of github.com:guardian/scribe into sc-ff-backsp…
theefer 528487b
Merge branch 'master' of github.com:guardian/scribe into sc-ff-backsp…
theefer b658a57
Bump selenium-webdriver version
theefer 30d1987
Upgrade selenium everywhere necessary
theefer 8bd25fa
More tests around cross-paragraph deletion
theefer 76fee17
Cleanup code
theefer 8c4df39
Ensure P and LI are selectable
theefer 83a05c4
Describe broken smart list plugin tests for now
theefer 068ff58
Add missing files
theefer d0c6a98
More explicit versioning
theefer 3cf299c
Clearer comment
theefer d9b8363
Move formatters into their directory
theefer 10e6253
Describe skipped tests
theefer 7b5c973
Syntax
theefer 049413e
Better test description
theefer ef70dda
Use lodash util
theefer a5a7fa5
Indentation
theefer 79f06d3
Arbitrarily bypass more tests
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
define([ | ||
'lodash-modern/arrays/flatten' | ||
], function ( | ||
flatten | ||
) { | ||
|
||
function observeDomChanges(el, callback) { | ||
function notEmptyTextNode(node) { | ||
return ! (node.nodeType === Node.TEXT_NODE && node.textContent === ''); | ||
} | ||
|
||
function notSelectionMarkerNode(node) { | ||
return ! (node.nodeType === Node.ELEMENT_NODE && node.className === 'scribe-marker'); | ||
} | ||
|
||
function includeRealMutations(mutations) { | ||
var allChangedNodes = flatten(mutations.map(function(mutation) { | ||
var added = Array.prototype.slice.call(mutation.addedNodes); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have Lodash, so maybe use |
||
var removed = Array.prototype.slice.call(mutation.removedNodes); | ||
return added.concat(removed); | ||
})); | ||
|
||
var realChangedNodes = allChangedNodes. | ||
filter(notEmptyTextNode). | ||
filter(notSelectionMarkerNode); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strange indentation? |
||
|
||
return realChangedNodes.length > 0; | ||
} | ||
|
||
|
||
// Flag to avoid running recursively | ||
var runningPostMutation = false; | ||
var observer = new MutationObserver(function(mutations) { | ||
if (! runningPostMutation && includeRealMutations(mutations)) { | ||
runningPostMutation = true; | ||
|
||
callback(); | ||
|
||
// We must yield to let any mutation we caused be triggered | ||
// in the next cycle | ||
setTimeout(function() { | ||
runningPostMutation = false; | ||
}, 0); | ||
} | ||
}); | ||
|
||
observer.observe(el, { | ||
attributes: true, | ||
childList: true, | ||
subtree: true | ||
}); | ||
|
||
return observer; | ||
} | ||
|
||
return observeDomChanges; | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
define([ | ||
'lodash-modern/arrays/last', | ||
'lodash-modern/collections/contains' | ||
], function ( | ||
last, | ||
contains | ||
) { | ||
|
||
/** | ||
* Chrome and Firefox: Upon pressing backspace inside of a P, the | ||
* browser deletes the paragraph element, leaving the caret (and any | ||
* content) outside of any P. | ||
* | ||
* Firefox: Erasing across multiple paragraphs, or outside of a | ||
* whole paragraph (e.g. by ‘Select All’) will leave content outside | ||
* of any P. | ||
* | ||
* Entering a new line in a pristine state state will insert | ||
* `<div>`s (in Chrome) or `<br>`s (in Firefox) where previously we | ||
* had `<p>`'s. This patches the behaviour of delete/backspace so | ||
* that we do not end up in a pristine state. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
|
||
// TODO: not exhaustive? | ||
var blockElementNames = ['P', 'LI', 'DIV', 'BLOCKQUOTE', 'UL', 'OL', 'H2']; | ||
function isBlockElement(node) { | ||
return contains(blockElementNames, node.nodeName); | ||
} | ||
|
||
/** | ||
* Wrap consecutive inline elements and text nodes in a P element. | ||
*/ | ||
function wrapChildNodes(parentNode) { | ||
var groups = Array.prototype.reduce.call(parentNode.childNodes, | ||
function (accumulator, binChildNode) { | ||
var group = last(accumulator); | ||
if (! group) { | ||
startNewGroup(); | ||
} else { | ||
var isBlockGroup = isBlockElement(group[0]); | ||
if (isBlockGroup === isBlockElement(binChildNode)) { | ||
group.push(binChildNode); | ||
} else { | ||
startNewGroup(); | ||
} | ||
} | ||
|
||
return accumulator; | ||
|
||
function startNewGroup() { | ||
var newGroup = [binChildNode]; | ||
accumulator.push(newGroup); | ||
} | ||
}, []); | ||
|
||
var consecutiveInlineElementsAndTextNodes = groups.filter(function (group) { | ||
var isBlockGroup = isBlockElement(group[0]); | ||
return ! isBlockGroup; | ||
}); | ||
|
||
consecutiveInlineElementsAndTextNodes.forEach(function (nodes) { | ||
var pElement = document.createElement('p'); | ||
nodes[0].parentNode.insertBefore(pElement, nodes[0]); | ||
nodes.forEach(function (node) { | ||
pElement.appendChild(node); | ||
}); | ||
}); | ||
|
||
parentNode._isWrapped = true; | ||
} | ||
|
||
// Traverse the tree, wrapping child nodes as we go. | ||
function traverse(parentNode) { | ||
var treeWalker = document.createTreeWalker(parentNode, NodeFilter.SHOW_ELEMENT); | ||
var node = treeWalker.firstChild(); | ||
|
||
// FIXME: does this recurse down? | ||
|
||
while (node) { | ||
// TODO: At the moment we only support BLOCKQUOTEs. See failing | ||
// tests. | ||
if (node.nodeName === 'BLOCKQUOTE' && ! node._isWrapped) { | ||
wrapChildNodes(node); | ||
traverse(parentNode); | ||
break; | ||
} | ||
node = treeWalker.nextSibling(); | ||
} | ||
} | ||
|
||
|
||
return function () { | ||
return function (scribe) { | ||
|
||
scribe.htmlFormatter.formatters.push(function (html) { | ||
/** | ||
* Ensure P mode. | ||
* | ||
* Wrap any orphan text nodes in a P element. | ||
*/ | ||
// TODO: This should be configurable and also correct markup such as | ||
// `<ul>1</ul>` to <ul><li>2</li></ul>`. See skipped tests. | ||
// TODO: This should probably be a part of HTML Janitor, or some other | ||
// formatter. | ||
var bin = document.createElement('div'); | ||
bin.innerHTML = html; | ||
|
||
wrapChildNodes(bin); | ||
traverse(bin); | ||
|
||
return bin.innerHTML; | ||
}); | ||
|
||
}; | ||
}; | ||
|
||
}); |
51 changes: 51 additions & 0 deletions
51
src/plugins/core/formatters/html/ensure-selectable-containers.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
define(function () { | ||
|
||
/** | ||
* Chrome and Firefox: Block-level elements like `<p>` or `<li>` | ||
* need to contain either text or a `<br>` to remain selectable. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
function containsChild(node, elementType) { | ||
// FIXME: do we need to recurse further down? | ||
for (var n = node.firstChild; n; n = n.nextSibling) { | ||
if (n.tagName === elementType) { | ||
return true; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
|
||
function traverse(parentNode) { | ||
var treeWalker = document.createTreeWalker(parentNode, NodeFilter.SHOW_ELEMENT); | ||
var node = treeWalker.firstChild(); | ||
|
||
while (node) { | ||
// Find any block-level container that contains neither text nor a <br> | ||
if ((node.nodeName === 'P' || node.nodeName === 'LI') && | ||
(node.textContent === '') && | ||
(! containsChild(node, 'BR'))) { | ||
node.appendChild(document.createElement('br')); | ||
} | ||
node = treeWalker.nextSibling(); | ||
} | ||
} | ||
|
||
return function () { | ||
return function (scribe) { | ||
|
||
scribe.htmlFormatter.formatters.push(function (html) { | ||
var bin = document.createElement('div'); | ||
bin.innerHTML = html; | ||
|
||
traverse(bin); | ||
|
||
return bin.innerHTML; | ||
}); | ||
|
||
}; | ||
}; | ||
|
||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://fredkschott.com/post/2014/02/npm-no-longer-defaults-to-tildes/