Bug 216325 – Selection API: Return live range synchronized with selection from getRangeAt and throw errors as specified

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Alex Christensen]

Make Selection throw same errors as other browsers and return same Range object from getRangeAt like other browsers

[Alex Christensen]

Created attachment 408368 [details]
Patch

[Darin Adler]

Comment on attachment 408368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408368&action=review

> Source/WebCore/page/DOMSelection.cpp:323
> +        if (auto shadowAncestor = selectionShadowAncestor(frame))
> +            m_cachedRange = createLiveRange(makeSimpleRange(*makeBoundaryPointBeforeNode(*shadowAncestor)));
> +        else
> +            m_cachedRange = createLiveRange(*frame.selection().selection().firstRange());

This is not right. What invalidates this when the selection changes? The DOMSelection class’s functions aren’t the only way to change the selection.

If we need to cache a Range and re-use it we will have to check that it’s still the same and create a new one if it’s not. Or add notification from the actual selection mechanism to tell the DOMSelection when to generate a new Range.

Easiest is probably to always recalculate the range and then only reuse m_cachedRange if it’s identical to what’s computed.

But since a Range is a mutable object, I’d like to understand what the semantics are if the caller changes the value of the Range after receiving it. Will we modify the selection if that is done (I doubt it)? Do we need to keep returning the same Range even though its value is now incorrect?

[Ryosuke Niwa]

Comment on attachment 408368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408368&action=review

> Source/WebCore/page/DOMSelection.cpp:228
> +    if (!extentNode || !baseNode)
> +        return Exception { TypeError };

We've never thrown these exceptions in the past.
I don't think we can make this kind of backwards incompatible change.

>> Source/WebCore/page/DOMSelection.cpp:323
>> +            m_cachedRange = createLiveRange(*frame.selection().selection().firstRange());
> 
> This is not right. What invalidates this when the selection changes? The DOMSelection class’s functions aren’t the only way to change the selection.
> 
> If we need to cache a Range and re-use it we will have to check that it’s still the same and create a new one if it’s not. Or add notification from the actual selection mechanism to tell the DOMSelection when to generate a new Range.
> 
> Easiest is probably to always recalculate the range and then only reuse m_cachedRange if it’s identical to what’s computed.
> 
> But since a Range is a mutable object, I’d like to understand what the semantics are if the caller changes the value of the Range after receiving it. Will we modify the selection if that is done (I doubt it)? Do we need to keep returning the same Range even though its value is now incorrect?

When the range is modified, we'd have to update the selection as well.

I think this "feature" needs to be done via runtime flag since this poses a pretty serious compatibility risk.

[Ryosuke Niwa]

By the way, according to the spec, we shouldn't be creating a new range whenever selection is changed either. We should be updating the one Range object we returned instead.

[Darin Adler]

The idea that DOMSelection returns a range with contents that dynamically update seems really strange given there can be multiple selected ranges in other browsers. What happens to the range when there is no selection? Or the second range when the selection is only a single range? Is there a specification covering this?

If that is correct behavior, it should be easy to implement. We just need to update the outstanding Range, if any, each time the selection changes. Code in FrameSelection would need to do it, I think.

As far as staging our implementation, I suspect it’s OK to return a single Range object that keeps getting updated without yet implementing the "changing the Range changes the selection" behavior; that can be done separately afterward.

And the other direction shouldn’t be too tricky either. We can either modify WebCore::Range to have virtual functions that are called any time a range is updated, and return a range of a special subclass, or to be more efficient but more poorly encapsulated we can put a boolean in WebCore::Range and teach it to call "up" to FrameSelection.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #5)
> 
> And the other direction shouldn’t be too tricky either. We can either modify
> WebCore::Range to have virtual functions that are called any time a range is
> updated, and return a range of a special subclass, or to be more efficient
> but more poorly encapsulated we can put a boolean in WebCore::Range and
> teach it to call "up" to FrameSelection.

That won’t quite work because addRange is supposed to keep whatever range given by the script and we’re supposed to respect future changes made to that Range and also update that range wherever selection is updated.

[Darin Adler]

I’m really having trouble understanding the model. Is there a specification somewhere I can look at.

If someone deselects, what happens to the Range? Does that "detach" it and make it inert? Is it supposed to be modified in some way as a side effect? Maybe collapsed or something?

[Darin Adler]

(In reply to Ryosuke Niwa from comment #6)
> (In reply to Darin Adler from comment #5)
> > 
> > And the other direction shouldn’t be too tricky either. We can either modify
> > WebCore::Range to have virtual functions that are called any time a range is
> > updated, and return a range of a special subclass, or to be more efficient
> > but more poorly encapsulated we can put a boolean in WebCore::Range and
> > teach it to call "up" to FrameSelection.
> 
> That won’t quite work because addRange is supposed to keep whatever range
> given by the script and we’re supposed to respect future changes made to
> that Range and also update that range wherever selection is updated.

The boolean flag would work, then. We would set it on the passed-in Range, and clear it on the old range, I guess.

[Alex Christensen]

https://www.w3.org/TR/selection-api/

[Darin Adler]

I see now, the specification prohibits implementing multiple ranges.

[Darin Adler]

This is easy to implement, then.

At any given time, at most one live range in a given document is the selection. Modifying it modifies the selection. DOMSelection::getRangeAt(0) always returns either that already-associated range, or a newly created one. We can use a pointer to the Range in the document instead of a boolean if we like to say which is this special Range, but I don’t think we should keep the pointer too far away from the Range, like it might not be good if it was in the FrameSelection and every Range modification involved going all the way to the FrameSelection or DOMSelection object to find if it’s the special one.

It seems that when you clear the selection, not using the DOMSelection API, that the documentation is not clear about what to do. It seems to prohibit disassociating the live range because it says "if the DOM changes in a way that changes the range's boundary points, or a script modifies the boundary points of the range, the same range object must continue to be associated with the selection", but doesn’t say what the range should be. And it mentions the DOM and a script, but not the user!

[Ryosuke Niwa]

(In reply to Darin Adler from comment #8)
> (In reply to Ryosuke Niwa from comment #6)
> > (In reply to Darin Adler from comment #5)
> > > 
> > > And the other direction shouldn’t be too tricky either. We can either modify
> > > WebCore::Range to have virtual functions that are called any time a range is
> > > updated, and return a range of a special subclass, or to be more efficient
> > > but more poorly encapsulated we can put a boolean in WebCore::Range and
> > > teach it to call "up" to FrameSelection.
> > 
> > That won’t quite work because addRange is supposed to keep whatever range
> > given by the script and we’re supposed to respect future changes made to
> > that Range and also update that range wherever selection is updated.
> 
> The boolean flag would work, then. We would set it on the passed-in Range,
> and clear it on the old range, I guess.

Right.

(In reply to Darin Adler from comment #11)
> This is easy to implement, then.
> 
> At any given time, at most one live range in a given document is the
> selection. Modifying it modifies the selection. DOMSelection::getRangeAt(0)
> always returns either that already-associated range, or a newly created one.

Right.

> We can use a pointer to the Range in the document instead of a boolean if we
> like to say which is this special Range, but I don’t think we should keep
> the pointer too far away from the Range, like it might not be good if it was
> in the FrameSelection and every Range modification involved going all the
> way to the FrameSelection or DOMSelection object to find if it’s the special
> one.

I think boolean works better. Then we can have a reference to that Range from FrameSelection.

We just need to remember to clear it so that we don't leak Document.

> It seems that when you clear the selection, not using the DOMSelection API,
> that the documentation is not clear about what to do. It seems to prohibit
> disassociating the live range because it says "if the DOM changes in a way
> that changes the range's boundary points, or a script modifies the boundary
> points of the range, the same range object must continue to be associated
> with the selection", but doesn’t say what the range should be. And it
> mentions the DOM and a script, but not the user!

See https://www.w3.org/TR/selection-api/#user-interactions

[Darin Adler]

Comment on attachment 408368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408368&action=review

>> Source/WebCore/page/DOMSelection.cpp:228
>> +        return Exception { TypeError };
> 
> We've never thrown these exceptions in the past.
> I don't think we can make this kind of backwards incompatible change.

The Selection API specification, which you are editor of, specifies this. If you don’t think we should do it, I suggest we make a change to that specification.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #13)
> Comment on attachment 408368 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408368&action=review
> 
> >> Source/WebCore/page/DOMSelection.cpp:228
> >> +        return Exception { TypeError };
> > 
> > We've never thrown these exceptions in the past.
> > I don't think we can make this kind of backwards incompatible change.
> 
> The Selection API specification, which you are editor of, specifies this. If
> you don’t think we should do it, I suggest we make a change to that
> specification.

We probably should. I didn’t realize this difference existed. I should probably file an issue about this.

[Darin Adler]

I’ve got a patch ready that implement's this (no change log yet). The part I don’t yet have done is the feature flag, right now it’s just a function that always returns false. I have not added a feature flag before and not sure exactly how. Also wondering how to best take advantage of the Web Platform Tests when the feature flag is still off by default.

[Darin Adler]

Created attachment 408643 [details]
Patch

[Darin Adler]

Created attachment 408663 [details]
Patch

[Darin Adler]

Created attachment 408673 [details]
Patch

[Alex Christensen]

(In reply to Darin Adler from comment #15)
> Also wondering how to best take advantage of the Web Platform
> Tests when the feature flag is still off by default.

In such a case we usually make WebKitTestRunner and DumpRenderTree explicitly turn the flag on.

[Darin Adler]

(In reply to Alex Christensen from comment #19)
> (In reply to Darin Adler from comment #15)
> > Also wondering how to best take advantage of the Web Platform
> > Tests when the feature flag is still off by default.
> 
> In such a case we usually make WebKitTestRunner and DumpRenderTree
> explicitly turn the flag on.

Thanks!

I am aware of that option. But for something like this where the feature is a big *change*, not simple a new *addition*, is it safe to only test with the feature flag on? Seems unlikely we can do without regression tests for the mode we are all still shipping.

I’d think for this kind of behavior change development we’d want a system where we could identify a set of tests to run in both modes, but that could be quite unwieldy, and would make our tests take even longer.

[Alex Christensen]

We do something similar for tests in http/tests/resourceLoadStatistics where if it ends with -database.html we use a database storage, otherwise it uses a plist storage.  This certainly isn't great, and we're about to remove the plist storage code, but that is an option.

[Ryosuke Niwa]

Comment on attachment 408673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408673&action=review

We need to modify Range.idl and keep it alive while it's associated with selection.
i.e. JSRange::hasOpaqueRoot must return true while Range::m_isAssociatedWithSelection is true.

> Source/WebCore/dom/Range.cpp:99
> +        m_ownerDocument->selection().clear();
> +        m_isAssociatedWithSelection = false;

Hm... this is https://github.com/w3c/selection-api/issues/79
I need to figure out where in DOM spec I can hook into to fix this.

> Source/WebCore/dom/Range.h:103
> +    void wasAssociatedWithSelection() { m_isAssociatedWithSelection = true; }
> +    void wasDisassociatedFromSelection() { m_isAssociatedWithSelection = false; }

I'd prefer didAssociate/didDiassociate since wasAssociate sounds like a question about whether it was previously associated with selection or not.

> Source/WebCore/editing/FrameSelection.cpp:371
> +    updateAssociatedLiveRange();

We also need to call this in FrameSelection::respondToNodeModification:
https://trac.webkit.org/browser/trunk/Source/WebCore/editing/FrameSelection.cpp#L525

That's what gets called when DOM tree is mutated.

> Source/WebCore/editing/FrameSelection.cpp:2833
> +    if (!isRootNode(m_document, range))
> +        disassociateLiveRange();

How could this ever happen? Given we'd be running updateFromAssociatedLiveRange()
whenever Range is modified, this condition seems impossible to hit.
Why don't we just RELEASE_ASSERT instead?

> Source/WebCore/editing/FrameSelection.cpp:2838
> +        // Tell the live range it's not associated while modifying it so it doesn't call updateFromAssociatedLiveRange.
> +        m_associatedLiveRange->wasDisassociatedFromSelection();
> +        *m_associatedLiveRange = *range;
> +        m_associatedLiveRange->wasAssociatedWithSelection();

Instead of adding a comment like this, why don't we just add a member function for this? e.g.
m_associatedLiveRange->updateFromSelection(*range);

> Source/WebCore/editing/FrameSelection.h:339
> +    RefPtr<Range> m_associatedLiveRange;

Please clear this in willBeRemovedFromFrame.

[Ryosuke Niwa]

Darin's patch will also fix https://bugs.webkit.org/show_bug.cgi?id=145212.

[Darin Adler]

Comment on attachment 408673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408673&action=review

Thanks for looking this over.

>> Source/WebCore/dom/Range.h:103
>> +    void wasDisassociatedFromSelection() { m_isAssociatedWithSelection = false; }
> 
> I'd prefer didAssociate/didDiassociate since wasAssociate sounds like a question about whether it was previously associated with selection or not.

OK.

>> Source/WebCore/editing/FrameSelection.cpp:371
>> +    updateAssociatedLiveRange();
> 
> We also need to call this in FrameSelection::respondToNodeModification:
> https://trac.webkit.org/browser/trunk/Source/WebCore/editing/FrameSelection.cpp#L525
> 
> That's what gets called when DOM tree is mutated.

No, I’m not sure we do that. Live ranges already get updated when the DOM tree is mutated, so I would expect that the associated live range will already be correctly updated.

I think we need to make test cases to demonstrate if that is necessary, or if there are subtle differences between how the selection live range should be updated and how others are updated.

>> Source/WebCore/editing/FrameSelection.cpp:2833
>> +        disassociateLiveRange();
> 
> How could this ever happen? Given we'd be running updateFromAssociatedLiveRange()
> whenever Range is modified, this condition seems impossible to hit.
> Why don't we just RELEASE_ASSERT instead?

This happens in two different ways:

1) The selection is cleared.
2) The selection is moved into a shadow tree.

I used to have special cases for those two; both are covered by a single check.

>> Source/WebCore/editing/FrameSelection.cpp:2838
>> +        m_associatedLiveRange->wasAssociatedWithSelection();
> 
> Instead of adding a comment like this, why don't we just add a member function for this? e.g.
> m_associatedLiveRange->updateFromSelection(*range);

OK.

>> Source/WebCore/editing/FrameSelection.h:339
>> +    RefPtr<Range> m_associatedLiveRange;
> 
> Please clear this in willBeRemovedFromFrame.

It does get cleared there.

Because willBeRemovedFromFrame calls setSelectionWithoutUpdatingAppearance with an empty selection. The setSelectionWithoutUpdatingAppearance that calls updateAssociatedLiveRange after setting m_selection. Then updateAssociatedLiveRange calls disassociateLiveRange since isRootNode returns false.

I don’t want to add additional code since this is a pretty strong guarantee. But maybe I should add an assertion if you think this is fragile in some way.

[Darin Adler]

(In reply to Ryosuke Niwa from comment #22)
> We need to modify Range.idl and keep it alive while it's associated with
> selection.
> i.e. JSRange::hasOpaqueRoot must return true while
> Range::m_isAssociatedWithSelection is true.

Don’t we already have a mechanism that keeps the wrapper alive in that case, as we do for DOM nodes? Can you help me construct a test to check? I am happy to add the code, but I really want to check with a test. Maybe this really is the first API that holds a live range and returns it a second time.

[Darin Adler]

Created attachment 408846 [details]
Patch

[Ryosuke Niwa]

(In reply to Darin Adler from comment #24)
> Comment on attachment 408673 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408673&action=review
> 
> Thanks for looking this over.
>
> >> Source/WebCore/editing/FrameSelection.cpp:371
> >> +    updateAssociatedLiveRange();
> > 
> > We also need to call this in FrameSelection::respondToNodeModification:
> > https://trac.webkit.org/browser/trunk/Source/WebCore/editing/FrameSelection.cpp#L525
> > 
> > That's what gets called when DOM tree is mutated.
> 
> No, I’m not sure we do that. Live ranges already get updated when the DOM
> tree is mutated, so I would expect that the associated live range will
> already be correctly updated.
> 
> I think we need to make test cases to demonstrate if that is necessary, or
> if there are subtle differences between how the selection live range should
> be updated and how others are updated.

I'm pretty sure there is a subtle difference there. I recall I purposefully did something differently when nodes are removed before/after the boundary points when I wrote respondToNodeModification. I don't recall the exact difference tough.

> >> Source/WebCore/editing/FrameSelection.cpp:2833
> >> +        disassociateLiveRange();
> > 
> > How could this ever happen? Given we'd be running updateFromAssociatedLiveRange()
> > whenever Range is modified, this condition seems impossible to hit.
> > Why don't we just RELEASE_ASSERT instead?
> 
> This happens in two different ways:
> 
> 1) The selection is cleared.
> 2) The selection is moved into a shadow tree.
> 
> I used to have special cases for those two; both are covered by a single
> check.

Oh I see. That's somewhat subtle. Maybe add a comment there?

> >> Source/WebCore/editing/FrameSelection.h:339
> >> +    RefPtr<Range> m_associatedLiveRange;
> > 
> > Please clear this in willBeRemovedFromFrame.
> 
> It does get cleared there.
> 
> Because willBeRemovedFromFrame calls setSelectionWithoutUpdatingAppearance
> with an empty selection. The setSelectionWithoutUpdatingAppearance that
> calls updateAssociatedLiveRange after setting m_selection. Then
> updateAssociatedLiveRange calls disassociateLiveRange since isRootNode
> returns false.
> 
> I don’t want to add additional code since this is a pretty strong guarantee.
> But maybe I should add an assertion if you think this is fragile in some way.

Got it.

(In reply to Darin Adler from comment #25)
> (In reply to Ryosuke Niwa from comment #22)
> > We need to modify Range.idl and keep it alive while it's associated with
> > selection.
> > i.e. JSRange::hasOpaqueRoot must return true while
> > Range::m_isAssociatedWithSelection is true.
> 
> Don’t we already have a mechanism that keeps the wrapper alive in that case,
> as we do for DOM nodes? Can you help me construct a test to check? I am
> happy to add the code, but I really want to check with a test. Maybe this
> really is the first API that holds a live range and returns it a second time.

I don't think so. GC doesn't know that JSRange needs to be kept alive in this case. The way we achieve that is either visitChildren of some other live object like JSDOSelection visiting this JSRange or isReachableFromOpaqueRoot returns true for this object while it's kept alive by JSDOMWindow. It's possible there are other cases where we hold onto Range for a later use but I'm pretty sure that any such code has the same bug I'm pointing out here.

So I think you can write a test like this:

function setRangeAlive() {
    getSelection().getRangeAt(0).alive = true;
}

for (let i = 0; i < 20; ++i) {
    setRangeAlive();
    GCController.collect();
    shouldBeTrue('getSelection().getRangeAt(0).alive');
}

[Darin Adler]

Created attachment 408852 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/69015762>

[Darin Adler]

Created attachment 408979 [details]
Patch

[Darin Adler]

This new patch that I uploaded has most of the implementation. Here’s what I know it lacks:

1) Haven’t updated all the tests to run with the feature on, and rebased all the expectations accordingly. Or maybe tests turn all experimental features on? If so, then a *lot* are going to fail.

2) Haven’t done the work to make sure that arbitrary JavaScript properties attached to the range are preserved.

3) Haven’t added support to FrameSelection for preserving the endpoints, not canonicalizing them and throwing away the pre-canonical ones.

Need at least do all 3 of those before turning on the feature flag for real.

[Darin Adler]

Created attachment 408996 [details]
Patch

[Darin Adler]

Created attachment 408997 [details]
Patch

[Darin Adler]

Created attachment 409003 [details]
Patch

[Darin Adler]

I think I’d like to land this patch, then do (2), then do (3), then run tests locally and make more fixes, then perhaps change the feature into an experimental feature (that step is a bit risky because then we’d not be testing the legacy code in modern WebKit regression tests).

[Alex Christensen]

Comment on attachment 409003 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409003&action=review

> Source/WebCore/ChangeLog:3
> +        Make Selection throw same errors as other browsers and return same Range object from getRangeAt like other browsers

This should definitely have a different title.  "Introduce off-by-default LiveRangeSelectionEnabled"

> Source/WebCore/editing/FrameSelection.h:338
> +    Document* m_document { nullptr };

As long as we're changing this we may as well make it WeakPtr<Document> to avoid UAFs.

> Source/WebCore/page/DOMSelection.idl:71
> +    undefined modify(optional DOMString alter, optional DOMString direction, optional DOMString granularity);

Do we really want to modify (pun intended) the default values of this old nonstandard function?

[Darin Adler]

Comment on attachment 409003 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409003&action=review

>> Source/WebCore/ChangeLog:3
>> +        Make Selection throw same errors as other browsers and return same Range object from getRangeAt like other browsers
> 
> This should definitely have a different title.  "Introduce off-by-default LiveRangeSelectionEnabled"

Yes. I guess I will either retitle this bug or move this patch to another.

>> Source/WebCore/editing/FrameSelection.h:338
>> +    Document* m_document { nullptr };
> 
> As long as we're changing this we may as well make it WeakPtr<Document> to avoid UAFs.

OK.

>> Source/WebCore/page/DOMSelection.idl:71
>> +    undefined modify(optional DOMString alter, optional DOMString direction, optional DOMString granularity);
> 
> Do we really want to modify (pun intended) the default values of this old nonstandard function?

Yes. It’s a trivial, harmless cleanup. All three of these strings are treated as enumerations by the function, checking for specific values. Any unknown value, including null string, empty string, and "undefined", is treated the same.

I didn’t remove "optional" because *that* is a trivial cleanup that could cause an exception with misuse of this method, rather than the current behavior of harmlessly doing nothing, so that requires a tiny bit more research and thought. Would also be nice to actually use IDL binding enumerations for this instead of literally using strings. But none of that is important to do right now so I left this otherwise alone. Seemed irritating to still have the "undefined" comment and values, though, and there was no real value to having them.

[Sam Weinig]

Comment on attachment 409003 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409003&action=review

> Source/WebCore/ChangeLog:7
> +

I think this could use a brief explanation/overview of the changes, especially why a setting is being added.

> Source/WebCore/editing/FrameSelection.cpp:2783
> +static bool isRootNode(Document* document, const Optional<SimpleRange>& range)

This name is confusing to me. It seems like it should include the word contains somewhere.

> LayoutTests/ChangeLog:7
> +

What's the plan for tests here? I assume this should start passing a bunch of the WPT suite with LiveRangeSelectionEnabled turned on, no?

[Darin Adler]

Comment on attachment 409003 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409003&action=review

>> Source/WebCore/ChangeLog:7
>> +
> 
> I think this could use a brief explanation/overview of the changes, especially why a setting is being added.

OK, will do. Going to use a different bug to post the final patch.

>> Source/WebCore/editing/FrameSelection.cpp:2783
>> +static bool isRootNode(Document* document, const Optional<SimpleRange>& range)
> 
> This name is confusing to me. It seems like it should include the word contains somewhere.

Could just be named "contains". Really this is "is in this document's tree, not in a shadow tree, nor disconnected, nor in another document". That’s what document->contains(x) checks for a Node x. So I can just call it contains. However, the Selection API uses the "document is the root node of the node" terminology.

>> LayoutTests/ChangeLog:7
>> +
> 
> What's the plan for tests here? I assume this should start passing a bunch of the WPT suite with LiveRangeSelectionEnabled turned on, no?

Yes, when we turn this on, it affects tons of test results. We may want to add some new tests too, but at first, just passing the many, many affected tests covers a *lot* of what is needed.

I’ll probably add a at least one test to check "don’t lose properties when GC" and perhaps more.

All of that will come with the later patch that moves this to "experimental" or turns it on unconditionally, or whatever. I don’t understand how to test both the old code path and the new one responsibly, which is why right now this is testing only the old path.

[Darin Adler]

This is at an interesting stage. The new implementation checked in under a feature flag now passes all the Web Platform Tests, but there are tons of effects on existing regression tests. Many of those are unimportant changes or slight progressions, but it’s going to take time to work through all of them.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #40)
> This is at an interesting stage. The new implementation checked in under a
> feature flag now passes all the Web Platform Tests, but there are tons of
> effects on existing regression tests. Many of those are unimportant changes
> or slight progressions, but it’s going to take time to work through all of
> them.

Hm... I wonder if it makes sense to turn on this feature in WPT directory so that both code paths will be tested. I think our existing layout tests is probably sufficient to cover the old behavior.

[Darin Adler]

(In reply to Ryosuke Niwa from comment #41)
> Hm... I wonder if it makes sense to turn on this feature in WPT directory so
> that both code paths will be tested. I think our existing layout tests is
> probably sufficient to cover the old behavior.

I am not sure that our other regression tests cover the old behavior well, but despite that sure, I’d be OK doing that. Any tips on how?

[Ryosuke Niwa]

(In reply to Darin Adler from comment #42)
> (In reply to Ryosuke Niwa from comment #41)
> > Hm... I wonder if it makes sense to turn on this feature in WPT directory so
> > that both code paths will be tested. I think our existing layout tests is
> > probably sufficient to cover the old behavior.
> 
> I am not sure that our other regression tests cover the old behavior well,
> but despite that sure, I’d be OK doing that. Any tips on how?

I think you can turn it into an experimental feature although this would mean that we'd have to do something about WK1.

Sam (Weinig) has been doing some work in area. Perhaps Sam can advise us on what to do here.

[Darin Adler]

(In reply to Ryosuke Niwa from comment #43)
> (In reply to Darin Adler from comment #42)
> > (In reply to Ryosuke Niwa from comment #41)
> > > Hm... I wonder if it makes sense to turn on this feature in WPT directory so
> > > that both code paths will be tested. I think our existing layout tests is
> > > probably sufficient to cover the old behavior.
> > 
> > I am not sure that our other regression tests cover the old behavior well,
> > but despite that sure, I’d be OK doing that. Any tips on how?
> 
> I think you can turn it into an experimental feature although this would
> mean that we'd have to do something about WK1.

Turning it into an experimental feature would turn it on by default for all tests in Modern WebKit. I know this because I made this an experimental feature at first (when I didn’t know what an experimental feature was).

I don’t think that’s helpful if our goal is to have it turned on for a single WPT directory and off for all other tests.

[Sam Weinig]

(In reply to Darin Adler from comment #44)
> (In reply to Ryosuke Niwa from comment #43)
> > (In reply to Darin Adler from comment #42)
> > > (In reply to Ryosuke Niwa from comment #41)
> > > > Hm... I wonder if it makes sense to turn on this feature in WPT directory so
> > > > that both code paths will be tested. I think our existing layout tests is
> > > > probably sufficient to cover the old behavior.
> > > 
> > > I am not sure that our other regression tests cover the old behavior well,
> > > but despite that sure, I’d be OK doing that. Any tips on how?
> > 
> > I think you can turn it into an experimental feature although this would
> > mean that we'd have to do something about WK1.
> 
> Turning it into an experimental feature would turn it on by default for all
> tests in Modern WebKit. I know this because I made this an experimental
> feature at first (when I didn’t know what an experimental feature was).
> 
> I don’t think that’s helpful if our goal is to have it turned on for a
> single WPT directory and off for all other tests.

I commented in a bit more detail in https://bugs.webkit.org/show_bug.cgi?id=217042, but I think we should extend the current "test header / comment" system we have to also support specifying preferences from a per-directory manifest file, so that we can use that concept for things like WPT where we don't want to modify the tests themselves. Right now, adding things to the "test header / comment" system is tedious, but once I get a little further into generation, I think we could easily make it work for any preference.

[Ryosuke Niwa]

Created attachment 463689 [details]
WIP

editing/deleting/backspace-delete-last-char-in-table.html: Initial selection has changed.
editing/deleting/delete-4038408-fix.html: Ditto.
editing/deleting/delete-image-001.html: Editing delegate callback change.
editing/deleting/delete-image-002.html: Ditto.
editing/deleting/delete-image-003.html: Ditto.
editing/deleting/delete-image-004.html: Ditto.
editing/deleting/delete-line-001.html: Ditto.
editing/deleting/delete-line-003.html: Ditto.
editing/deleting/delete-line-005.html: Ditto.
editing/deleting/delete-list-items-in-table-cell-1.html: Initial selection has changed.
editing/deleting/delete-list-items-in-table-cell-2.html: Ditto.
editing/deleting/delete-list-items-in-table-cell-5.html: Ditto.
editing/deleting/delete-list-items-in-table-cell-7.html: Ditto.
editing/deleting/deleting-relative-positioned-special-element.html: Ditto.
editing/deleting/forward-delete-last-char-in-table.html: Ditto.
editing/deleting/merge-paragraph-contatining-noneditable.html: Ditto.
editing/editability/ignored-content.html: Visually identical selection.
editing/execCommand/button.html: Ditto.
editing/execCommand/change-list-type.html: Initial selection has changed.
editing/execCommand/contenteditable-justify-next-paragraph.html: Final selection has changed but it's visually identical.
editing/execCommand/format-block-multiple-paragraphs-in-pre.html: Initial selection has changed.
editing/execCommand/format-block-multiple-paragraphs.html: Ditto.
editing/execCommand/format-block-table.html: Ditto.
editing/execCommand/insertHorizontalRule.html: Progression. Caret is between "foo" and "bar" as expected.
editing/execCommand/indent-pre-list.html: Initial selection has changed.
editing/execCommand/indent-pre-paragraphs.html: Ditto.
editing/execCommand/insert-paragraph-twice-at-end-of-block-styled-listitem.html: Ditto.
editing/execCommand/insert-paragraph-twice-at-end-of-custom-listitem.html: Ditto.
editing/execCommand/unindent-nested-blockquote-with-inner-div.html: Ditto.
editing/inserting/insert-list-in-table-cell-01.html: Ditto.
editing/inserting/insert-list-in-table-cell-02.html: Ditto.
editing/inserting/insert-list-in-table-cell-03.html: Ditto.
editing/inserting/insert-list-in-table-cell-04.html: Ditto.
editing/inserting/insert-list-in-table-cell-05.html: Ditto.
editing/inserting/insert-list-in-table-cell-06.html: Ditto.
editing/inserting/insert-list-in-table-cell-07.html: Ditto.
editing/inserting/insert-list-in-table-cell-08.html: Ditto.
editing/pasteboard/copy-paste-with-important-rules.html: Ditto.
editing/pasteboard/copy-text-with-backgroundcolor.html: Ditto.
editing/pasteboard/display-block-on-spans.html: Ditto.
editing/pasteboard/paste-table-cells.html: Ditto.
editing/pasteboard/paste-table-with-unrendered-text-nodes.html: Ditto.
editing/pasteboard/paste-text-005.html: Visually identical selection.
editing/pasteboard/paste-text-006.html: Ditto.
editing/pasteboard/paste-text-007.html: Ditto.
editing/pasteboard/paste-text-with-style.html: Initial selection has changed.
editing/pasteboard/select-element-1.html: Editing delegate callback change.
editing/pasteboard/testcase-9507.html: Initial selection has changed.
editing/selection/containsNode-expected.txt: Rebselined.
editing/selection/containsNode.html: Changed the expectation for partially selected text node when partialContainment flag
is not set. When live range was disabled, we'd always behave as if partial containment is set to true.
editing/selection/dump-as-markup.html: Initial selection has changed.
editing/selection/extend-after-mouse-selection.html: Visually identical selection.
editing/selection/extend-selection-backward-at-beginning-of-line-by-sentence-granularity.html: Dutto,
editing/selection/extend-selection-backward-at-beginning-of-line-by-word-granularity.html: Ditto.
editing/selection/focus-contenteditable-iframe.html: Initial selection has changed.
editing/selection/getRangeAt.html: Visually identical selection.
editing/selection/move-by-word-visually-across-object-element-1.html: Initial selection has changed.
editing/selection/move-by-word-visually-across-object-element-2.html: Ditto.
editing/selection/move-by-word-visually-across-object-element-3.html: Ditto.
editing/selection/node-removal-1.html: Visually identical selection.
editing/selection/select-out-of-floated-non-editable-02.html: Ditto.
editing/selection/select-out-of-floated-non-editable-07.html: Ditto.
editing/selection/select-out-of-floated-non-editable-10.html: Ditto.
editing/selection/shift-click-includes-existing-selection.html: Initial selection has changed.
editing/selection/toString-1.html: Visually identical selection.
editing/selection/user-select-all-selection.html: Ditto.
editing/selection/user-select-all-with-shift.html.html: Ditto.
editing/selection/user-select-all-with-single-click.html: Ditto.
editing/selection/user-select-js-property.html: Changed the expectation of the first test case. New expected result matches
Chrome and Firefox.
editing/style/change-text-direction-crash.html: Visually identical selection.
editing/style/fore-color-by-name.html: Ditto.
editing/style/preserve-selection-direction.html: Ditto.
editing/style/remove-underline-from-stylesheet.html: Ditto.
editing/style/style-boundary-001.html: Ditto.
editing/style/style-text-node-without-editable-parent.html: Ditto.
editing/undo/redo-style.html: Ditto.
editing/unsupported-content/list-delete-001.html: Ditto.
editing/unsupported-content/list-delete-003.html: Ditto.


7 failures remain under LayoutTests/editing:
editing/deleting/delete-start-block.html
editing/deleting/forward-delete-empty-table-cell.html
editing/execCommand/insert-list-nested-with-orphaned.html
editing/execCommand/insert-nested-lists-in-table.html
editing/execCommand/reset-direction-after-breaking-blockquote.html
editing/selection/editable-table-cell-selection.html
editing/selection/shift-click.html

[Ryosuke Niwa]

Created attachment 463945 [details]
WIP

[Ryosuke Niwa]

Created attachment 463974 [details]
WIP2

[Ryosuke Niwa]

Created attachment 464082 [details]
WIP3

[Ryosuke Niwa]

*** Bug 220514 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

*** Bug 23600 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Created attachment 464110 [details]
WIP4

[Ryosuke Niwa]

Created attachment 464323 [details]
WIP5

[Ryosuke Niwa]

Created attachment 464343 [details]
WIP6

[Ryosuke Niwa]

Pull request: https://github.com/WebKit/WebKit/pull/8267

[EWS]

Committed 259904@main (1226d38de5c7): <https://commits.webkit.org/259904@main>

Reviewed commits have been landed. Closing PR #8267 and removing active labels.

[Ryosuke Niwa]

*** Bug 145212 has been marked as a duplicate of this bug. ***