- Revision
- 267362
- Author
- [email protected]
- Date
- 2020-09-21 13:23:26 -0700 (Mon, 21 Sep 2020)
Log Message
Selection API: A few more refinements to DOMSelection and VisibleSelection to pass all WPT tests
https://bugs.webkit.org/show_bug.cgi?id=216756
Reviewed by Ryosuke Niwa.
Source/WebCore:
After these changes, we pass all the tests in imported/w3c/web-platform-tests/selection
with no failures except for one due to the rules about absorbing newlines at the start
and end <style> and <script> elements.
However, that's with the live selection range feature enabled, and there are likely issues
with other tests in that mode, so that testing still needs to be done. Then we also have to
decide how we are going to deal with the compatibility risk of changing the behavior to
match the standard.
* editing/VisibleSelection.cpp:
(WebCore::VisibleSelection::VisibleSelection): Updated to rename m_baseIsFirst to
m_anchorIsFirst. Not required for the fix, but helpful for clarity.
(WebCore::VisibleSelection::uncanonicalizedStart const): Ditto.
(WebCore::VisibleSelection::uncanonicalizedEnd const): Ditto.
(WebCore::VisibleSelection::setBaseAndExtentToDeepEquivalents): Compute whether
the anchor is first *before* canonicalization, otherwise we will reverse the two
if their canonical values are equal. Canonicalization is not allowed to change
the ordering other than making two values equal.
(WebCore::VisibleSelection::validate): Updated for name.
(WebCore::VisibleSelection::setWithoutValidation): Ditto, also tweaked other names in the
function and removed an if statement.
(WebCore::VisibleSelection::adjustSelectionToAvoidCrossingShadowBoundaries): Ditto.
(WebCore::VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries): Ditto.
* editing/VisibleSelection.h: Renamed m_baseIsFirst to m_anchorIsFirst and improved the
comment about m_isDirectional.
* page/DOMSelection.cpp:
(WebCore::DOMSelection::anchorPosition const): Removed use of parentAnchoredEquivalent.
The name makes it sound like it would be useful, but really it just triggers some
editing behaviors that don't belong in Position code.
(WebCore::DOMSelection::focusPosition const): Ditto.
(WebCore::DOMSelection::basePosition const): Ditto.
(WebCore::DOMSelection::extentPosition const): Ditto.
(WebCore::DOMSelection::collapse): Reversed the order of the document check and the
check for invalid nodes and offsets. There are some inconsistencies between the
specification and WPT but for now matching WPT seems like the way to go.
(WebCore::DOMSelection::setBaseAndExtent): Ditto.
(WebCore::DOMSelection::extend): Use VisibleSelection::setExtent, which does exactly
what we want, rather than FrameSelection::setExtent, which does not. In the future
we may want to cut down on the number of subtly-different selection functions,
like these two, but for now this fixes this DOM method to work as specified.
LayoutTests:
* editing/inserting/insert-list-in-table-cell-07-expected.txt: Updated results
for a slight change in where the insertion point ends up. This is neither a
progression nor a regression, but this is also a very strange edge case. It
would be better at some point to enhance the editing code so the selection stays
intact, which I think means that the entire table body would be selected, but
the current behavior, before this patch and after, yields an insertion point.
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (267361 => 267362)
--- trunk/LayoutTests/ChangeLog 2020-09-21 20:22:27 UTC (rev 267361)
+++ trunk/LayoutTests/ChangeLog 2020-09-21 20:23:26 UTC (rev 267362)
@@ -1,3 +1,17 @@
+2020-09-20 Darin Adler <[email protected]>
+
+ Selection API: A few more refinements to DOMSelection and VisibleSelection to pass all WPT tests
+ https://bugs.webkit.org/show_bug.cgi?id=216756
+
+ Reviewed by Ryosuke Niwa.
+
+ * editing/inserting/insert-list-in-table-cell-07-expected.txt: Updated results
+ for a slight change in where the insertion point ends up. This is neither a
+ progression nor a regression, but this is also a very strange edge case. It
+ would be better at some point to enhance the editing code so the selection stays
+ intact, which I think means that the entire table body would be selected, but
+ the current behavior, before this patch and after, yields an insertion point.
+
2020-09-21 Chris Dumez <[email protected]>
Remove emphasis/de-emphasis filters from DynamicsCompressor
Modified: trunk/LayoutTests/editing/inserting/insert-list-in-table-cell-07-expected.txt (267361 => 267362)
--- trunk/LayoutTests/editing/inserting/insert-list-in-table-cell-07-expected.txt 2020-09-21 20:22:27 UTC (rev 267361)
+++ trunk/LayoutTests/editing/inserting/insert-list-in-table-cell-07-expected.txt 2020-09-21 20:23:26 UTC (rev 267362)
@@ -8,8 +8,7 @@
| id="element"
| <tr>
| <td>
-| <#selection-anchor>
-| "fsdf"
+| "<#selection-anchor>fsdf"
| <td>
| "fsdf"
| <tr>
@@ -20,7 +19,6 @@
| <tbody>
After:
-| <#selection-caret>
| <table>
| border="1"
| <tbody>
@@ -27,7 +25,8 @@
| id="element"
| <tr>
| <td>
-| "fsdf"
+| "<#selection-caret>fsdf"
+| <br>
| <td>
| "fsdf"
| <tr>
@@ -35,3 +34,4 @@
| "gghfg"
| <td>
| "fsfg"
+| <tbody>
Modified: trunk/Source/WebCore/ChangeLog (267361 => 267362)
--- trunk/Source/WebCore/ChangeLog 2020-09-21 20:22:27 UTC (rev 267361)
+++ trunk/Source/WebCore/ChangeLog 2020-09-21 20:23:26 UTC (rev 267362)
@@ -1,3 +1,53 @@
+2020-09-20 Darin Adler <[email protected]>
+
+ Selection API: A few more refinements to DOMSelection and VisibleSelection to pass all WPT tests
+ https://bugs.webkit.org/show_bug.cgi?id=216756
+
+ Reviewed by Ryosuke Niwa.
+
+ After these changes, we pass all the tests in imported/w3c/web-platform-tests/selection
+ with no failures except for one due to the rules about absorbing newlines at the start
+ and end <style> and <script> elements.
+
+ However, that's with the live selection range feature enabled, and there are likely issues
+ with other tests in that mode, so that testing still needs to be done. Then we also have to
+ decide how we are going to deal with the compatibility risk of changing the behavior to
+ match the standard.
+
+ * editing/VisibleSelection.cpp:
+ (WebCore::VisibleSelection::VisibleSelection): Updated to rename m_baseIsFirst to
+ m_anchorIsFirst. Not required for the fix, but helpful for clarity.
+ (WebCore::VisibleSelection::uncanonicalizedStart const): Ditto.
+ (WebCore::VisibleSelection::uncanonicalizedEnd const): Ditto.
+ (WebCore::VisibleSelection::setBaseAndExtentToDeepEquivalents): Compute whether
+ the anchor is first *before* canonicalization, otherwise we will reverse the two
+ if their canonical values are equal. Canonicalization is not allowed to change
+ the ordering other than making two values equal.
+ (WebCore::VisibleSelection::validate): Updated for name.
+ (WebCore::VisibleSelection::setWithoutValidation): Ditto, also tweaked other names in the
+ function and removed an if statement.
+ (WebCore::VisibleSelection::adjustSelectionToAvoidCrossingShadowBoundaries): Ditto.
+ (WebCore::VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries): Ditto.
+
+ * editing/VisibleSelection.h: Renamed m_baseIsFirst to m_anchorIsFirst and improved the
+ comment about m_isDirectional.
+
+ * page/DOMSelection.cpp:
+ (WebCore::DOMSelection::anchorPosition const): Removed use of parentAnchoredEquivalent.
+ The name makes it sound like it would be useful, but really it just triggers some
+ editing behaviors that don't belong in Position code.
+ (WebCore::DOMSelection::focusPosition const): Ditto.
+ (WebCore::DOMSelection::basePosition const): Ditto.
+ (WebCore::DOMSelection::extentPosition const): Ditto.
+ (WebCore::DOMSelection::collapse): Reversed the order of the document check and the
+ check for invalid nodes and offsets. There are some inconsistencies between the
+ specification and WPT but for now matching WPT seems like the way to go.
+ (WebCore::DOMSelection::setBaseAndExtent): Ditto.
+ (WebCore::DOMSelection::extend): Use VisibleSelection::setExtent, which does exactly
+ what we want, rather than FrameSelection::setExtent, which does not. In the future
+ we may want to cut down on the number of subtly-different selection functions,
+ like these two, but for now this fixes this DOM method to work as specified.
+
2020-09-21 Chris Dumez <[email protected]>
Remove emphasis/de-emphasis filters from DynamicsCompressor
Modified: trunk/Source/WebCore/editing/VisibleSelection.cpp (267361 => 267362)
--- trunk/Source/WebCore/editing/VisibleSelection.cpp 2020-09-21 20:22:27 UTC (rev 267361)
+++ trunk/Source/WebCore/editing/VisibleSelection.cpp 2020-09-21 20:23:26 UTC (rev 267362)
@@ -44,7 +44,7 @@
namespace WebCore {
VisibleSelection::VisibleSelection()
- : m_baseIsFirst(true)
+ : m_anchorIsFirst(true)
, m_isDirectional(false)
{
}
@@ -98,12 +98,12 @@
Position VisibleSelection::uncanonicalizedStart() const
{
- return m_baseIsFirst ? m_anchor : m_focus;
+ return m_anchorIsFirst ? m_anchor : m_focus;
}
Position VisibleSelection::uncanonicalizedEnd() const
{
- return m_baseIsFirst ? m_focus : m_anchor;
+ return m_anchorIsFirst ? m_focus : m_anchor;
}
Optional<SimpleRange> VisibleSelection::range() const
@@ -225,13 +225,13 @@
if (m_focus.isNull())
m_focus = m_anchor;
+ m_anchorIsFirst = m_anchor <= m_focus;
+
m_base = VisiblePosition(m_anchor, m_affinity).deepEquivalent();
if (m_anchor == m_focus)
m_extent = m_base;
else
m_extent = VisiblePosition(m_focus, m_affinity).deepEquivalent();
-
- m_baseIsFirst = m_base <= m_extent;
}
void VisibleSelection::adjustSelectionRespectingGranularity(TextGranularity granularity)
@@ -375,8 +375,8 @@
{
setBaseAndExtentToDeepEquivalents();
- m_start = m_baseIsFirst ? m_base : m_extent;
- m_end = m_baseIsFirst ? m_extent : m_base;
+ m_start = m_anchorIsFirst ? m_base : m_extent;
+ m_end = m_anchorIsFirst ? m_extent : m_base;
auto startBeforeAdjustments = m_start;
auto endBeforeAdjustments = m_end;
@@ -409,38 +409,32 @@
}
if (shouldUpdateAnchor) {
- m_anchor = m_baseIsFirst ? m_start : m_end;
+ m_anchor = m_anchorIsFirst ? m_start : m_end;
m_base = m_anchor;
}
if (shouldUpdateFocus) {
- m_focus = m_baseIsFirst ? m_end : m_start;
+ m_focus = m_anchorIsFirst ? m_end : m_start;
m_extent = m_focus;
}
}
-// FIXME: This function breaks the invariant of this class.
-// But because we use VisibleSelection to store values in editing commands for use when
-// undoing the command, we need to be able to create a selection that while currently
+// Because we use VisibleSelection to store values in editing commands for use when
+// undoing the command, we need to be able to create a selection that, while currently
// invalid, will be valid once the changes are undone. This is a design problem.
-// To fix it we either need to change the invariants of VisibleSelection or create a new
-// class for editing to use that can manipulate selections that are not currently valid.
-void VisibleSelection::setWithoutValidation(const Position& base, const Position& extent)
+// The best fix is likely to get rid of canonicalization from VisibleSelection entirely,
+// and then remove this function.
+void VisibleSelection::setWithoutValidation(const Position& anchor, const Position& focus)
{
- ASSERT(base.isNull() == extent.isNull());
+ ASSERT(anchor.isNull() == focus.isNull());
ASSERT(m_affinity == Affinity::Downstream);
- m_anchor = base;
- m_focus = extent;
- m_base = base;
- m_extent = extent;
- m_baseIsFirst = base <= extent;
- if (m_baseIsFirst) {
- m_start = base;
- m_end = extent;
- } else {
- m_start = extent;
- m_end = base;
- }
- m_type = base == extent ? Type::Caret : Type::Range;
+ m_anchor = anchor;
+ m_focus = focus;
+ m_anchorIsFirst = m_anchor <= m_focus;
+ m_base = anchor;
+ m_extent = focus;
+ m_start = m_anchorIsFirst ? anchor : focus;
+ m_end = m_anchorIsFirst ? focus : anchor;
+ m_type = anchor == focus ? Type::Caret : Type::Range;
}
Position VisibleSelection::adjustPositionForEnd(const Position& currentPosition, Node* startContainerNode)
@@ -512,7 +506,7 @@
}
// Correct the focus if necessary.
- if (m_baseIsFirst) {
+ if (m_anchorIsFirst) {
m_extent = adjustPositionForEnd(m_end, m_start.containerNode());
m_end = m_extent;
} else {
@@ -621,7 +615,7 @@
// Correct the focus if necessary.
if (baseEditableAncestor != lowestEditableAncestor(m_extent.containerNode())) {
- m_extent = m_baseIsFirst ? m_end : m_start;
+ m_extent = m_anchorIsFirst ? m_end : m_start;
m_focus = m_extent;
}
}
Modified: trunk/Source/WebCore/editing/VisibleSelection.h (267361 => 267362)
--- trunk/Source/WebCore/editing/VisibleSelection.h 2020-09-21 20:22:27 UTC (rev 267361)
+++ trunk/Source/WebCore/editing/VisibleSelection.h 2020-09-21 20:23:26 UTC (rev 267362)
@@ -87,7 +87,7 @@
bool isNonOrphanedRange() const { return isRange() && !start().isOrphan() && !end().isOrphan(); }
bool isNoneOrOrphaned() const { return isNone() || start().isOrphan() || end().isOrphan(); }
- bool isBaseFirst() const { return m_baseIsFirst; }
+ bool isBaseFirst() const { return m_anchorIsFirst; }
bool isDirectional() const { return m_isDirectional; }
void setIsDirectional(bool isDirectional) { m_isDirectional = isDirectional; }
@@ -156,8 +156,8 @@
// These are cached, can be recalculated by validate()
enum class Type : uint8_t { None, Caret, Range };
Type m_type { Type::None };
- bool m_baseIsFirst : 1; // True if base is before the extent.
- bool m_isDirectional : 1; // Non-directional ignores m_baseIsFirst and selection always extends on shift + arrow key.
+ bool m_anchorIsFirst : 1; // True if the anchor is before the focus.
+ bool m_isDirectional : 1; // On Mac, Shift-arrow keys move the anchor in a directional selection and moves either end to always extend in a non-directional selection.
};
inline bool operator==(const VisibleSelection& a, const VisibleSelection& b)
Modified: trunk/Source/WebCore/page/DOMSelection.cpp (267361 => 267362)
--- trunk/Source/WebCore/page/DOMSelection.cpp 2020-09-21 20:22:27 UTC (rev 267361)
+++ trunk/Source/WebCore/page/DOMSelection.cpp 2020-09-21 20:23:26 UTC (rev 267362)
@@ -83,7 +83,7 @@
if (!frame)
return { };
if (frame->settings().liveRangeSelectionEnabled())
- return frame->selection().selection().anchor().parentAnchoredEquivalent();
+ return frame->selection().selection().anchor();
auto& selection = frame->selection().selection();
return (selection.isBaseFirst() ? selection.start() : selection.end()).parentAnchoredEquivalent();
}
@@ -94,7 +94,7 @@
if (!frame)
return { };
if (frame->settings().liveRangeSelectionEnabled())
- return frame->selection().selection().focus().parentAnchoredEquivalent();
+ return frame->selection().selection().focus();
auto& selection = frame->selection().selection();
return (selection.isBaseFirst() ? selection.end() : selection.start()).parentAnchoredEquivalent();
}
@@ -106,7 +106,7 @@
if (!frame)
return { };
if (frame->settings().liveRangeSelectionEnabled())
- return frame->selection().selection().anchor().parentAnchoredEquivalent();
+ return frame->selection().selection().anchor();
return frame->selection().selection().base().parentAnchoredEquivalent();
}
@@ -117,7 +117,7 @@
if (!frame)
return { };
if (frame->settings().liveRangeSelectionEnabled())
- return frame->selection().selection().focus().parentAnchoredEquivalent();
+ return frame->selection().selection().focus();
return frame->selection().selection().extent().parentAnchoredEquivalent();
}
@@ -203,11 +203,10 @@
removeAllRanges();
return { };
}
- auto& document = *frame->document();
- if (!document.contains(*node))
- return { };
if (auto result = Range::checkNodeOffsetPair(*node, offset); result.hasException())
return result.releaseException();
+ if (!frame->document()->contains(*node))
+ return { };
} else {
if (!isValidForPosition(node))
return { };
@@ -264,13 +263,13 @@
// FIXME: We should do this by making the arguments non-nullable in the IDL file, once liveRangeSelectionEnabled is always true.
if (!baseNode || !extentNode)
return Exception { TypeError };
- auto& document = *frame->document();
- if (!document.contains(*baseNode) || !document.contains(*extentNode))
- return { };
if (auto result = Range::checkNodeOffsetPair(*baseNode, baseOffset); result.hasException())
return result.releaseException();
if (auto result = Range::checkNodeOffsetPair(*extentNode, extentOffset); result.hasException())
return result.releaseException();
+ auto& document = *frame->document();
+ if (!document.contains(*baseNode) || !document.contains(*extentNode))
+ return { };
} else {
if (!isValidForPosition(baseNode) || !isValidForPosition(extentNode))
return { };
@@ -344,15 +343,18 @@
return { };
if (auto result = Range::checkNodeOffsetPair(node, offset); result.hasException())
return result.releaseException();
+ auto& selection = frame->selection();
+ auto newSelection = selection.selection();
+ newSelection.setExtent(makeContainerOffsetPosition(&node, offset));
+ selection.disassociateLiveRange();
+ selection.setSelection(newSelection);
} else {
if (offset > node.length())
return Exception { IndexSizeError };
if (!isValidForPosition(&node))
return { };
+ frame->selection().setExtent(makeContainerOffsetPosition(&node, offset), Affinity::Downstream);
}
- auto& selection = frame->selection();
- selection.disassociateLiveRange();
- selection.setExtent(makeContainerOffsetPosition(&node, offset), Affinity::Downstream);
return { };
}
