⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 267362 in webkit


Ignore:
Timestamp:
Sep 21, 2020, 1:23:26 PM (6 years ago)
Author:
Darin Adler
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.

Location:
trunk
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r267361 r267362  
     12020-09-20  Darin Adler  <darin@apple.com>
     2
     3        Selection API: A few more refinements to DOMSelection and VisibleSelection to pass all WPT tests
     4        https://bugs.webkit.org/show_bug.cgi?id=216756
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        * editing/inserting/insert-list-in-table-cell-07-expected.txt: Updated results
     9        for a slight change in where the insertion point ends up. This is neither a
     10        progression nor a regression, but this is also a very strange edge case. It
     11        would be better at some point to enhance the editing code so the selection stays
     12        intact, which I think means that the entire table body would be selected, but
     13        the current behavior, before this patch and after, yields an insertion point.
     14
    1152020-09-21  Chris Dumez  <cdumez@apple.com>
    216

  • trunk/LayoutTests/editing/inserting/insert-list-in-table-cell-07-expected.txt

    r260207 r267362  
    99|     <tr>
    1010|       <td>
    11 |         <#selection-anchor>
    12 |         "fsdf"
     11|         "<#selection-anchor>fsdf"
    1312|       <td>
    1413|         "fsdf"
    … …  
    2120
    2221After:
    23 | <#selection-caret>
    2422| <table>
    2523|   border="1"
    … …  
    2826|     <tr>
    2927|       <td>
    30 |         "fsdf"
     28|         "<#selection-caret>fsdf"
     29|         <br>
    3130|       <td>
    3231|         "fsdf"
    … …  
    3635|       <td>
    3736|         "fsfg"
     37|   <tbody>

  • trunk/Source/WebCore/ChangeLog

    r267361 r267362  
     12020-09-20  Darin Adler  <darin@apple.com>
     2
     3        Selection API: A few more refinements to DOMSelection and VisibleSelection to pass all WPT tests
     4        https://bugs.webkit.org/show_bug.cgi?id=216756
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        After these changes, we pass all the tests in imported/w3c/web-platform-tests/selection
     9        with no failures except for one due to the rules about absorbing newlines at the start
     10        and end <style> and <script> elements.
     11
     12        However, that's with the live selection range feature enabled, and there are likely issues
     13        with other tests in that mode, so that testing still needs to be done. Then we also have to
     14        decide how we are going to deal with the compatibility risk of changing the behavior to
     15        match the standard.
     16
     17        * editing/VisibleSelection.cpp:
     18        (WebCore::VisibleSelection::VisibleSelection): Updated to rename m_baseIsFirst to
     19        m_anchorIsFirst. Not required for the fix, but helpful for clarity.
     20        (WebCore::VisibleSelection::uncanonicalizedStart const): Ditto.
     21        (WebCore::VisibleSelection::uncanonicalizedEnd const): Ditto.
     22        (WebCore::VisibleSelection::setBaseAndExtentToDeepEquivalents): Compute whether
     23        the anchor is first *before* canonicalization, otherwise we will reverse the two
     24        if their canonical values are equal. Canonicalization is not allowed to change
     25        the ordering other than making two values equal.
     26        (WebCore::VisibleSelection::validate): Updated for name.
     27        (WebCore::VisibleSelection::setWithoutValidation): Ditto, also tweaked other names in the
     28        function and removed an if statement.
     29        (WebCore::VisibleSelection::adjustSelectionToAvoidCrossingShadowBoundaries): Ditto.
     30        (WebCore::VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries): Ditto.
     31
     32        * editing/VisibleSelection.h: Renamed m_baseIsFirst to m_anchorIsFirst and improved the
     33        comment about m_isDirectional.
     34
     35        * page/DOMSelection.cpp:
     36        (WebCore::DOMSelection::anchorPosition const): Removed use of parentAnchoredEquivalent.
     37        The name makes it sound like it would be useful, but really it just triggers some
     38        editing behaviors that don't belong in Position code.
     39        (WebCore::DOMSelection::focusPosition const): Ditto.
     40        (WebCore::DOMSelection::basePosition const): Ditto.
     41        (WebCore::DOMSelection::extentPosition const): Ditto.
     42        (WebCore::DOMSelection::collapse): Reversed the order of the document check and the
     43        check for invalid nodes and offsets. There are some inconsistencies between the
     44        specification and WPT but for now matching WPT seems like the way to go.
     45        (WebCore::DOMSelection::setBaseAndExtent): Ditto.
     46        (WebCore::DOMSelection::extend): Use VisibleSelection::setExtent, which does exactly
     47        what we want, rather than FrameSelection::setExtent, which does not. In the future
     48        we may want to cut down on the number of subtly-different selection functions,
     49        like these two, but for now this fixes this DOM method to work as specified.
     50
    1512020-09-21  Chris Dumez  <cdumez@apple.com>
    252

  • trunk/Source/WebCore/editing/VisibleSelection.cpp

    r267329 r267362  
    4545
    4646VisibleSelection::VisibleSelection()
    47     : m_baseIsFirst(true)
     47    : m_anchorIsFirst(true)
    4848    , m_isDirectional(false)
    4949{
    … …  
    9999Position VisibleSelection::uncanonicalizedStart() const
    100100{
    101     return m_baseIsFirst ? m_anchor : m_focus;
     101    return m_anchorIsFirst ? m_anchor : m_focus;
    102102}
    103103
    104104Position VisibleSelection::uncanonicalizedEnd() const
    105105{
    106     return m_baseIsFirst ? m_focus : m_anchor;
     106    return m_anchorIsFirst ? m_focus : m_anchor;
    107107}
    108108
    … …  
    226226        m_focus = m_anchor;
    227227
     228    m_anchorIsFirst = m_anchor <= m_focus;
     229
    228230    m_base = VisiblePosition(m_anchor, m_affinity).deepEquivalent();
    229231    if (m_anchor == m_focus)
    … …  
    231233    else
    232234        m_extent = VisiblePosition(m_focus, m_affinity).deepEquivalent();
    233 
    234     m_baseIsFirst = m_base <= m_extent;
    235235}
    236236
    … …  
    376376    setBaseAndExtentToDeepEquivalents();
    377377
    378     m_start = m_baseIsFirst ? m_base : m_extent;
    379     m_end = m_baseIsFirst ? m_extent : m_base;
     378    m_start = m_anchorIsFirst ? m_base : m_extent;
     379    m_end = m_anchorIsFirst ? m_extent : m_base;
    380380
    381381    auto startBeforeAdjustments = m_start;
    … …  
    410410
    411411    if (shouldUpdateAnchor) {
    412         m_anchor = m_baseIsFirst ? m_start : m_end;
     412        m_anchor = m_anchorIsFirst ? m_start : m_end;
    413413        m_base = m_anchor;
    414414    }
    415415    if (shouldUpdateFocus) {
    416         m_focus = m_baseIsFirst ? m_end : m_start;
     416        m_focus = m_anchorIsFirst ? m_end : m_start;
    417417        m_extent = m_focus;
    418418    }
    419419}
    420420
    421 // FIXME: This function breaks the invariant of this class.
    422 // But because we use VisibleSelection to store values in editing commands for use when
    423 // undoing the command, we need to be able to create a selection that while currently
     421// Because we use VisibleSelection to store values in editing commands for use when
     422// undoing the command, we need to be able to create a selection that, while currently
    424423// invalid, will be valid once the changes are undone. This is a design problem.
    425 // To fix it we either need to change the invariants of VisibleSelection or create a new
    426 // class for editing to use that can manipulate selections that are not currently valid.
    427 void VisibleSelection::setWithoutValidation(const Position& base, const Position& extent)
    428 {
    429     ASSERT(base.isNull() == extent.isNull());
     424// The best fix is likely to get rid of canonicalization from VisibleSelection entirely,
     425// and then remove this function.
     426void VisibleSelection::setWithoutValidation(const Position& anchor, const Position& focus)
     427{
     428    ASSERT(anchor.isNull() == focus.isNull());
    430429    ASSERT(m_affinity == Affinity::Downstream);
    431     m_anchor = base;
    432     m_focus = extent;
    433     m_base = base;
    434     m_extent = extent;
    435     m_baseIsFirst = base <= extent;
    436     if (m_baseIsFirst) {
    437         m_start = base;
    438         m_end = extent;
    439     } else {
    440         m_start = extent;
    441         m_end = base;
    442     }
    443     m_type = base == extent ? Type::Caret : Type::Range;
     430    m_anchor = anchor;
     431    m_focus = focus;
     432    m_anchorIsFirst = m_anchor <= m_focus;
     433    m_base = anchor;
     434    m_extent = focus;
     435    m_start = m_anchorIsFirst ? anchor : focus;
     436    m_end = m_anchorIsFirst ? focus : anchor;
     437    m_type = anchor == focus ? Type::Caret : Type::Range;
    444438}
    445439
    … …  
    513507
    514508    // Correct the focus if necessary.
    515     if (m_baseIsFirst) {
     509    if (m_anchorIsFirst) {
    516510        m_extent = adjustPositionForEnd(m_end, m_start.containerNode());
    517511        m_end = m_extent;
    … …  
    622616    // Correct the focus if necessary.
    623617    if (baseEditableAncestor != lowestEditableAncestor(m_extent.containerNode())) {
    624         m_extent = m_baseIsFirst ? m_end : m_start;
     618        m_extent = m_anchorIsFirst ? m_end : m_start;
    625619        m_focus = m_extent;
    626620    }

  • trunk/Source/WebCore/editing/VisibleSelection.h

    r267329 r267362  
    8888    bool isNoneOrOrphaned() const { return isNone() || start().isOrphan() || end().isOrphan(); }
    8989
    90     bool isBaseFirst() const { return m_baseIsFirst; }
     90    bool isBaseFirst() const { return m_anchorIsFirst; }
    9191    bool isDirectional() const { return m_isDirectional; }
    9292    void setIsDirectional(bool isDirectional) { m_isDirectional = isDirectional; }
    … …  
    157157    enum class Type : uint8_t { None, Caret, Range };
    158158    Type m_type { Type::None };
    159     bool m_baseIsFirst : 1; // True if base is before the extent.
    160     bool m_isDirectional : 1; // Non-directional ignores m_baseIsFirst and selection always extends on shift + arrow key.
     159    bool m_anchorIsFirst : 1; // True if the anchor is before the focus.
     160    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.
    161161};
    162162

  • trunk/Source/WebCore/page/DOMSelection.cpp

    r267329 r267362  
    8484        return { };
    8585    if (frame->settings().liveRangeSelectionEnabled())
    86         return frame->selection().selection().anchor().parentAnchoredEquivalent();
     86        return frame->selection().selection().anchor();
    8787    auto& selection = frame->selection().selection();
    8888    return (selection.isBaseFirst() ? selection.start() : selection.end()).parentAnchoredEquivalent();
    … …  
    9595        return { };
    9696    if (frame->settings().liveRangeSelectionEnabled())
    97         return frame->selection().selection().focus().parentAnchoredEquivalent();
     97        return frame->selection().selection().focus();
    9898    auto& selection = frame->selection().selection();
    9999    return (selection.isBaseFirst() ? selection.end() : selection.start()).parentAnchoredEquivalent();
    … …  
    107107        return { };
    108108    if (frame->settings().liveRangeSelectionEnabled())
    109         return frame->selection().selection().anchor().parentAnchoredEquivalent();
     109        return frame->selection().selection().anchor();
    110110    return frame->selection().selection().base().parentAnchoredEquivalent();
    111111}
    … …  
    118118        return { };
    119119    if (frame->settings().liveRangeSelectionEnabled())
    120         return frame->selection().selection().focus().parentAnchoredEquivalent();
     120        return frame->selection().selection().focus();
    121121    return frame->selection().selection().extent().parentAnchoredEquivalent();
    122122}
    … …  
    204204            return { };
    205205        }
    206         auto& document = *frame->document();
    207         if (!document.contains(*node))
    208             return { };
    209206        if (auto result = Range::checkNodeOffsetPair(*node, offset); result.hasException())
    210207            return result.releaseException();
     208        if (!frame->document()->contains(*node))
     209            return { };
    211210    } else {
    212211        if (!isValidForPosition(node))
    … …  
    265264        if (!baseNode || !extentNode)
    266265            return Exception { TypeError };
    267         auto& document = *frame->document();
    268         if (!document.contains(*baseNode) || !document.contains(*extentNode))
    269             return { };
    270266        if (auto result = Range::checkNodeOffsetPair(*baseNode, baseOffset); result.hasException())
    271267            return result.releaseException();
    272268        if (auto result = Range::checkNodeOffsetPair(*extentNode, extentOffset); result.hasException())
    273269            return result.releaseException();
     270        auto& document = *frame->document();
     271        if (!document.contains(*baseNode) || !document.contains(*extentNode))
     272            return { };
    274273    } else {
    275274        if (!isValidForPosition(baseNode) || !isValidForPosition(extentNode))
    … …  
    345344        if (auto result = Range::checkNodeOffsetPair(node, offset); result.hasException())
    346345            return result.releaseException();
     346        auto& selection = frame->selection();
     347        auto newSelection = selection.selection();
     348        newSelection.setExtent(makeContainerOffsetPosition(&node, offset));
     349        selection.disassociateLiveRange();
     350        selection.setSelection(newSelection);
    347351    } else {
    348352        if (offset > node.length())
    … …  
    350354        if (!isValidForPosition(&node))
    351355            return { };
    352     }
    353     auto& selection = frame->selection();
    354     selection.disassociateLiveRange();
    355     selection.setExtent(makeContainerOffsetPosition(&node, offset), Affinity::Downstream);
     356        frame->selection().setExtent(makeContainerOffsetPosition(&node, offset), Affinity::Downstream);
     357    }
    356358    return { };
    357359}
Note: See TracChangeset for help on using the changeset viewer.