Release assert in Document::updateLayout() via HTMLTextAreaElement::childrenChanged

[Ryosuke Niwa]

e.g.

ASSERTION FAILED: isSafeToUpdateStyleOrLayout(*this)
./dom/Document.cpp(2174) : void WebCore::Document::updateLayout()
1   0x6a2323d49 WTFCrash
2   0x6a2323d69 WTFCrashWithSecurityImplication
3   0x699daa33d WebCore::Document::updateLayout()
4   0x699dabc4e WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks)
5   0x69a02e2ee WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&)
6   0x69a02e966 WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity)
7   0x699fbd864 WebCore::FrameSelection::recomputeCaretRect()
8   0x699fb79b2 WebCore::FrameSelection::updateAppearance()
9   0x699fb76fc WebCore::FrameSelection::updateAndRevealSelection(WebCore::AXTextStateChangeIntent const&)
10  0x699f8c3fd WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>, WebCore::AXTextStateChangeIntent, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)
11  0x699fb4f06 WebCore::FrameSelection::moveWithoutValidationTo(WebCore::Position const&, WebCore::Position const&, bool, bool, WebCore::SelectionRevealMode, WebCore::AXTextStateChangeIntent const&)
12  0x69a1ca3c0 WebCore::HTMLTextFormControlElement::setSelectionRange(int, int, WebCore::TextFieldSelectionDirection, WebCore::SelectionRevealMode, WebCore::AXTextStateChangeIntent const&)
13  0x69a1cb9c5 WebCore::HTMLTextAreaElement::setValueCommon(WTF::String const&)
14  0x69a1c84b0 WebCore::HTMLTextAreaElement::childrenChanged(WebCore::ContainerNode::ChildChange const&)
15  0x699d6b201 WebCore::ContainerNode::removeAllChildrenWithScriptAssertion(WebCore::ContainerNode::ChildChange::Source, WebCore::ContainerNode::DeferChildrenChanged)

<rdar://76389334>

[Ryosuke Niwa]

Created attachment 425833 [details]
WIP

[Ryosuke Niwa]

Created attachment 426637 [details]
Test

[Ryosuke Niwa]

e.g.
ASSERTION FAILED: isSafeToUpdateStyleOrLayout(*this)
./dom/Document.cpp(2174) : void WebCore::Document::updateLayout()
1   0x6a2323d49 WTFCrash
2   0x6a2323d69 WTFCrashWithSecurityImplication
3   0x699daa33d WebCore::Document::updateLayout()
4   0x699dabc4e WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks)
5   0x69a02e2ee WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&)
6   0x69a02e966 WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity)
7   0x699fbd864 WebCore::FrameSelection::recomputeCaretRect()
8   0x699fb79b2 WebCore::FrameSelection::updateAppearance()
9   0x699fb76fc WebCore::FrameSelection::updateAndRevealSelection(WebCore::AXTextStateChangeIntent const&)
10  0x699f8c3fd WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>, WebCore::AXTextStateChangeIntent, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)
11  0x699fb4f06 WebCore::FrameSelection::moveWithoutValidationTo(WebCore::Position const&, WebCore::Position const&, bool, bool, WebCore::SelectionRevealMode, WebCore::AXTextStateChangeIntent const&)
12  0x69a1ca3c0 WebCore::HTMLTextFormControlElement::setSelectionRange(int, int, WebCore::TextFieldSelectionDirection, WebCore::SelectionRevealMode, WebCore::AXTextStateChangeIntent const&)
13  0x69a1cb9c5 WebCore::HTMLTextAreaElement::setValueCommon(WTF::String const&)
14  0x69a1c84b0 WebCore::HTMLTextAreaElement::childrenChanged(WebCore::ContainerNode::ChildChange const&)
15  0x699d6b201 WebCore::ContainerNode::removeAllChildrenWithScriptAssertion(WebCore::ContainerNode::ChildChange::Source, WebCore::ContainerNode::DeferChildrenChanged)

[Sergio Villar Senin]

Created attachment 429057 [details]
Patch

[Sergio Villar Senin]

I haven't added a test case because the current one is pretty good and minimal.

Note that the bug cannot be reproduced if the leaf node (n1) has at least one child. The reason why it does not fail is because when the child is added then setValueCommon() is called and value() is initialized to the empty string. Later when replaceChildren() is called the setValueCommon() call finds that the new value (empty string) is equal to value() (also the empty string set before) and then it bails out gracefully.

[Ryosuke Niwa]

(In reply to Sergio Villar Senin from comment #5)
> I haven't added a test case because the current one is pretty good and
> minimal.
> 
> Note that the bug cannot be reproduced if the leaf node (n1) has at least
> one child. The reason why it does not fail is because when the child is
> added then setValueCommon() is called and value() is initialized to the
> empty string. Later when replaceChildren() is called the setValueCommon()
> call finds that the new value (empty string) is equal to value() (also the
> empty string set before) and then it bails out gracefully.

Hm... can we include the test in the patch since this isn't a security bug? The fact we're hitting release assertion would mean that we'd simply crash.

[Ryosuke Niwa]

Comment on attachment 429057 [details]
Patch

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

> Source/WebCore/html/HTMLTextAreaElement.cpp:397
> -    if (normalizedValue == value())
> +    if (normalizedValue == value() || (normalizedValue.isEmpty() && value().isNull()))

Are you sure this works all the time even when the textarea has some child nodes?

[Sergio Villar Senin]

(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 429057 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429057&action=review
> 
> > Source/WebCore/html/HTMLTextAreaElement.cpp:397
> > -    if (normalizedValue == value())
> > +    if (normalizedValue == value() || (normalizedValue.isEmpty() && value().isNull()))
> 
> Are you sure this works all the time even when the textarea has some child
> nodes?

When the text area has some child nodes it does indeed early return as well and thus it does not ever call all the other following methods which is perhaps undesirable. I'll give it an extra thought.

[Ryosuke Niwa]

Comment on attachment 429057 [details]
Patch

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

>>> Source/WebCore/html/HTMLTextAreaElement.cpp:397
>>> +    if (normalizedValue == value() || (normalizedValue.isEmpty() && value().isNull()))
>> 
>> Are you sure this works all the time even when the textarea has some child nodes?
> 
> When the text area has some child nodes it does indeed early return as well and thus it does not ever call all the other following methods which is perhaps undesirable. I'll give it an extra thought.

ok.

[Sergio Villar Senin]

Created attachment 430760 [details]
Patch

[Ryosuke Niwa]

Hm... looks like there are a few test failures?

[Sergio Villar Senin]

(In reply to Ryosuke Niwa from comment #11)
> Hm... looks like there are a few test failures?

I'm working on those. Several tests assume that changes in the selection via JS are synchronous so that's why they're failing. I was a bit worried about the a11y ones because I am not sure if we're breaking any assumption in mac a11y code. However the patch does not change the behaviour in case the change is user-triggered so I guess we should be safe on that regard.

[Sergio Villar Senin]

Created attachment 432276 [details]
Patch

Fixing some a11y tests. Still some of them failing

[Sergio Villar Senin]

Created attachment 436407 [details]
Patch

[Sergio Villar Senin]

Created attachment 436479 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 436479 [details]
Patch

Looks like some tests fare failing?

[Sergio Villar Senin]

(In reply to Ryosuke Niwa from comment #16)
> Comment on attachment 436479 [details]
> Patch
> 
> Looks like some tests fare failing?

Yeah, both

   accessibility/mac/selection-boundary-userinfo.html
   fast/repaint/4776765.html

work fine in my local mac machine. So perhaps there are timing issues there. WRT to 

   accessibility/mac/selection-sync.html

I'd need some assistance from a a11y expert, because I'm having a hard time trying to make it work. I applied the same pattern used to make the other tests work (i.e. let the main thread kick in so we could asynchronously get notified) but it didn't work in the end.

[Brady Eidson]

(In reply to Sergio Villar Senin from comment #17)
> 
> I'd need some assistance from a a11y expert, because I'm having a hard time
> trying to make it work. I applied the same pattern used to make the other
> tests work (i.e. let the main thread kick in so we could asynchronously get
> notified) but it didn't work in the end.

James, can you help, or know someone who can?

[James Craig]

Chris F or Andres G, perhaps.

[chris fleizach]

(In reply to Brady Eidson from comment #18)
> (In reply to Sergio Villar Senin from comment #17)
> > 
> > I'd need some assistance from a a11y expert, because I'm having a hard time
> > trying to make it work. I applied the same pattern used to make the other
> > tests work (i.e. let the main thread kick in so we could asynchronously get
> > notified) but it didn't work in the end.
> 
> James, can you help, or know someone who can?

I think the problem is that you're returning early before notifying accessibility of changes

    if (!(options & IsUserTriggered)) {
 456        scheduleAppearanceUpdateAfterStyleChange();
 457        return;
 458    }

[Sergio Villar Senin]

(In reply to chris fleizach from comment #20)
> (In reply to Brady Eidson from comment #18)
> > (In reply to Sergio Villar Senin from comment #17)
> > > 
> > > I'd need some assistance from a a11y expert, because I'm having a hard time
> > > trying to make it work. I applied the same pattern used to make the other
> > > tests work (i.e. let the main thread kick in so we could asynchronously get
> > > notified) but it didn't work in the end.
> > 
> > James, can you help, or know someone who can?
> 
> I think the problem is that you're returning early before notifying
> accessibility of changes
> 
>     if (!(options & IsUserTriggered)) {
>  456        scheduleAppearanceUpdateAfterStyleChange();
>  457        return;
>  458    }

My a11y knowledge is very limited honestly, what's the expected way to do that?

[Andres Gonzalez]

(In reply to Sergio Villar Senin from comment #21)
> (In reply to chris fleizach from comment #20)
> > (In reply to Brady Eidson from comment #18)
> > > (In reply to Sergio Villar Senin from comment #17)
> > > > 
> > > > I'd need some assistance from a a11y expert, because I'm having a hard time
> > > > trying to make it work. I applied the same pattern used to make the other
> > > > tests work (i.e. let the main thread kick in so we could asynchronously get
> > > > notified) but it didn't work in the end.
> > > 
> > > James, can you help, or know someone who can?
> > 
> > I think the problem is that you're returning early before notifying
> > accessibility of changes
> > 
> >     if (!(options & IsUserTriggered)) {
> >  456        scheduleAppearanceUpdateAfterStyleChange();
> >  457        return;
> >  458    }
> 
> My a11y knowledge is very limited honestly, what's the expected way to do
> that?

As Chris pointed out, since you are returning in FrameSelection::setSelection before calling updateAndRevealSelection, notifyAccessibilityForSelectionChange is never called, and thus accessibility clients are not notified of selection changes. You may try calling notifyAccessibilityForSelectionChange before that early return. Not sure that that would work though without first calling the rest of updateAndRevealSelection, but worth the try.

[Sergio Villar Senin]

(In reply to Andres Gonzalez from comment #22)
> (In reply to Sergio Villar Senin from comment #21)
> > (In reply to chris fleizach from comment #20)
> > > (In reply to Brady Eidson from comment #18)
> > > > (In reply to Sergio Villar Senin from comment #17)
> > > > > 
> > > > > I'd need some assistance from a a11y expert, because I'm having a hard time
> > > > > trying to make it work. I applied the same pattern used to make the other
> > > > > tests work (i.e. let the main thread kick in so we could asynchronously get
> > > > > notified) but it didn't work in the end.
> > > > 
> > > > James, can you help, or know someone who can?
> > > 
> > > I think the problem is that you're returning early before notifying
> > > accessibility of changes
> > > 
> > >     if (!(options & IsUserTriggered)) {
> > >  456        scheduleAppearanceUpdateAfterStyleChange();
> > >  457        return;
> > >  458    }
> > 
> > My a11y knowledge is very limited honestly, what's the expected way to do
> > that?
> 
> As Chris pointed out, since you are returning in
> FrameSelection::setSelection before calling updateAndRevealSelection,
> notifyAccessibilityForSelectionChange is never called, and thus
> accessibility clients are not notified of selection changes. You may try
> calling notifyAccessibilityForSelectionChange before that early return. Not
> sure that that would work though without first calling the rest of
> updateAndRevealSelection, but worth the try.


Well, notifyAccessibilityForSelectionChange will be eventually called if you follow the call path:
scheduleAppearanceUpdateAfterStyleChang->appearanceUpdateTimerFired->updateAppearanceAfterLayout->updateAppearanceAfterLayoutOrStyleChange->updateAndRevealSelection->notifyAccessibilityForSelectionChange

The thing is that the update is not synchronous as it was before the path (note all the changes I had to make to test cases). As I mentioned here the problem I have is with the following tesst accessibility/mac/selection-sync.html.

Both the test contents and the name suggest that a11y expect those selection change events to be synchronous. I was wondering whether that's a requirement or the test could be skipped/adapted.

[Andres Gonzalez]

(In reply to Sergio Villar Senin from comment #23)
> (In reply to Andres Gonzalez from comment #22)
> > (In reply to Sergio Villar Senin from comment #21)
> > > (In reply to chris fleizach from comment #20)
> > > > (In reply to Brady Eidson from comment #18)
> > > > > (In reply to Sergio Villar Senin from comment #17)
> > > > > > 
> > > > > > I'd need some assistance from a a11y expert, because I'm having a hard time
> > > > > > trying to make it work. I applied the same pattern used to make the other
> > > > > > tests work (i.e. let the main thread kick in so we could asynchronously get
> > > > > > notified) but it didn't work in the end.
> > > > > 
> > > > > James, can you help, or know someone who can?
> > > > 
> > > > I think the problem is that you're returning early before notifying
> > > > accessibility of changes
> > > > 
> > > >     if (!(options & IsUserTriggered)) {
> > > >  456        scheduleAppearanceUpdateAfterStyleChange();
> > > >  457        return;
> > > >  458    }
> > > 
> > > My a11y knowledge is very limited honestly, what's the expected way to do
> > > that?
> > 
> > As Chris pointed out, since you are returning in
> > FrameSelection::setSelection before calling updateAndRevealSelection,
> > notifyAccessibilityForSelectionChange is never called, and thus
> > accessibility clients are not notified of selection changes. You may try
> > calling notifyAccessibilityForSelectionChange before that early return. Not
> > sure that that would work though without first calling the rest of
> > updateAndRevealSelection, but worth the try.
> 
> 
> Well, notifyAccessibilityForSelectionChange will be eventually called if you
> follow the call path:
> scheduleAppearanceUpdateAfterStyleChang->appearanceUpdateTimerFired-
> >updateAppearanceAfterLayout->updateAppearanceAfterLayoutOrStyleChange-
> >updateAndRevealSelection->notifyAccessibilityForSelectionChange
> 
> The thing is that the update is not synchronous as it was before the path
> (note all the changes I had to make to test cases). As I mentioned here the
> problem I have is with the following tesst
> accessibility/mac/selection-sync.html.
> 
> Both the test contents and the name suggest that a11y expect those selection
> change events to be synchronous. I was wondering whether that's a
> requirement or the test could be skipped/adapted.

Thanks Sergio for the clarification. Could you please mark accessibility/mac/selection-sync.html to be skipped, and file a bug assigned to me? It would be easier to fix this test once I can build with your patch, tried to apply it but ran into build issues.

[Sergio Villar Senin]

(In reply to Andres Gonzalez from comment #24)
> > > As Chris pointed out, since you are returning in
> > > FrameSelection::setSelection before calling updateAndRevealSelection,
> > > notifyAccessibilityForSelectionChange is never called, and thus
> > > accessibility clients are not notified of selection changes. You may try
> > > calling notifyAccessibilityForSelectionChange before that early return. Not
> > > sure that that would work though without first calling the rest of
> > > updateAndRevealSelection, but worth the try.
> > 
> > 
> > Well, notifyAccessibilityForSelectionChange will be eventually called if you
> > follow the call path:
> > scheduleAppearanceUpdateAfterStyleChang->appearanceUpdateTimerFired-
> > >updateAppearanceAfterLayout->updateAppearanceAfterLayoutOrStyleChange-
> > >updateAndRevealSelection->notifyAccessibilityForSelectionChange
> > 
> > The thing is that the update is not synchronous as it was before the path
> > (note all the changes I had to make to test cases). As I mentioned here the
> > problem I have is with the following tesst
> > accessibility/mac/selection-sync.html.
> > 
> > Both the test contents and the name suggest that a11y expect those selection
> > change events to be synchronous. I was wondering whether that's a
> > requirement or the test could be skipped/adapted.
> 
> Thanks Sergio for the clarification. Could you please mark
> accessibility/mac/selection-sync.html to be skipped, and file a bug assigned
> to me? It would be easier to fix this test once I can build with your patch,
> tried to apply it but ran into build issues.

Sure thing. My main concern here is whether this kind of change could break a11y stuff in the mac because as you can see in the test changes, the way a11y events are emitted changes completely (it's async).

Anyway I'll update the patch.

[Andres Gonzalez]

(In reply to Sergio Villar Senin from comment #25)
> (In reply to Andres Gonzalez from comment #24)
> > > > As Chris pointed out, since you are returning in
> > > > FrameSelection::setSelection before calling updateAndRevealSelection,
> > > > notifyAccessibilityForSelectionChange is never called, and thus
> > > > accessibility clients are not notified of selection changes. You may try
> > > > calling notifyAccessibilityForSelectionChange before that early return. Not
> > > > sure that that would work though without first calling the rest of
> > > > updateAndRevealSelection, but worth the try.
> > > 
> > > 
> > > Well, notifyAccessibilityForSelectionChange will be eventually called if you
> > > follow the call path:
> > > scheduleAppearanceUpdateAfterStyleChang->appearanceUpdateTimerFired-
> > > >updateAppearanceAfterLayout->updateAppearanceAfterLayoutOrStyleChange-
> > > >updateAndRevealSelection->notifyAccessibilityForSelectionChange
> > > 
> > > The thing is that the update is not synchronous as it was before the path
> > > (note all the changes I had to make to test cases). As I mentioned here the
> > > problem I have is with the following tesst
> > > accessibility/mac/selection-sync.html.
> > > 
> > > Both the test contents and the name suggest that a11y expect those selection
> > > change events to be synchronous. I was wondering whether that's a
> > > requirement or the test could be skipped/adapted.
> > 
> > Thanks Sergio for the clarification. Could you please mark
> > accessibility/mac/selection-sync.html to be skipped, and file a bug assigned
> > to me? It would be easier to fix this test once I can build with your patch,
> > tried to apply it but ran into build issues.
> 
> Sure thing. My main concern here is whether this kind of change could break
> a11y stuff in the mac because as you can see in the test changes, the way
> a11y events are emitted changes completely (it's async).
> 
> Anyway I'll update the patch.

We should handle async notifications in accessibility. And we are moving towards fully async change updates, especially with the introduction of the isolated tree mode where accessibility requests are handled off the main thread. So this should be inline with what we are doing.

[Sergio Villar Senin]

Created attachment 440381 [details]
Patch

[Sergio Villar Senin]

(In reply to Andres Gonzalez from comment #26)
> > Sure thing. My main concern here is whether this kind of change could break
> > a11y stuff in the mac because as you can see in the test changes, the way
> > a11y events are emitted changes completely (it's async).
> > 
> > Anyway I'll update the patch.
> 
> We should handle async notifications in accessibility. And we are moving
> towards fully async change updates, especially with the introduction of the
> isolated tree mode where accessibility requests are handled off the main
> thread. So this should be inline with what we are doing.

Sounds great, thanks for your assistance!

[Sergio Villar Senin]

Created attachment 440413 [details]
Patch

[Sergio Villar Senin]

Created attachment 440491 [details]
Patch

Win and WK1 fixes

[Sergio Villar Senin]

Created attachment 440587 [details]
Patch

More mac expectations

[Michael Saboff]

Comment on attachment 440587 [details]
Patch

Looks like there are still some issues with updated tests.

[Sergio Villar Senin]

Created attachment 443070 [details]
Patch

[Sergio Villar Senin]

Created attachment 443106 [details]
Patch

[Sergio Villar Senin]

Gentle ping for reviewers

[Sergio Villar Senin]

Ping

[Ryosuke Niwa]

Comment on attachment 443106 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:470
> +    auto document = Ref { *m_document };

Can't we just do: Ref document = *m_document?

> Source/WebCore/editing/FrameSelection.cpp:2515
> +    m_document->updateLayoutIgnorePendingStylesheets();
> +    updateAppearanceAfterLayout();

Why can't we call updateSelectionAppearanceNow instead? Please explain in the change log.

> Source/WebCore/html/HTMLFormControlElement.cpp:495
> +    document().selection().updateAppearanceIfRevealingSelectionIsNeeded();

Do: Ref { document().selection() }->updateAppearanceIfRevealingSelectionIsNeeded()
so that there is no change of the document getting removed
even if this form control element is adopted to another document.

> LayoutTests/ChangeLog:13
> +        Same situation for other tests that do not involve repaint rects but a11y notifications. We

Nit: but *trigger accessibility tree updates*?

> LayoutTests/accessibility/mac/selection-boundary-userinfo.html:165
> +        function yieldMainThread() {

waitForAnimationFrame? Also { should be on new line per WebKit style guideline.

> LayoutTests/accessibility/mac/selection-boundary-userinfo.html:167
> +            requestAnimationFrame(() => { resolve(); });

You can just do this: requestAnimationFrame(resolve);

> LayoutTests/accessibility/mac/selection-boundary-userinfo.html:187
> +        focusElement("textbox")
> +          .then(() => moveAsync(true))
> +          .then(() => focusElement("input"))
> +          .then(() => moveAsync(false))
> +          .then(() => focusElement("textarea"))
> +          .then(() => moveAsync(true));

Add async keyword to the outer function and do:
await moveAsync(true);
await focusElement("input");
...

> LayoutTests/accessibility/mac/selection-change-userinfo.html:240
> +        focusTextBox()
> +          .then(() => resetPosition())

Please use async & await here. This is so much boilerplate!

> LayoutTests/fast/repaint/selection-gap-absolute-child.html:16
> +        setTimeout(function() {

Why we don't need to wait for RAF? Please explain in the change log.

> LayoutTests/fast/repaint/selection-gap-flipped-absolute-child.html:16
> +        setTimeout(function() {

Ditto.

> LayoutTests/fast/repaint/selection-gap-transformed-absolute-child.html:16
> +        setTimeout(function() {

Ditto.

> LayoutTests/fast/repaint/selection-gap-transformed-fixed-child.html:16
> +        setTimeout(function() {

Ditto.

> LayoutTests/fast/repaint/selection-ruby-rl.html:23
> +    setTimeout(function() {

Ditto.

> LayoutTests/fast/repaint/text-selection-overflow-hidden.html:25
> +        setTimeout(function() {

Ditto.

> LayoutTests/platform/mac/TestExpectations:2450
> +
> +webkit.org/b/231298 accessibility/mac/selection-sync.html [ Skip ] # Timeout

We're losing test coverage here. Can we fix the test before we land this patch?

> LayoutTests/platform/win/fast/repaint/selection-gap-fixed-child-expected.txt:-7
> -(repaint rects
> -  (rect 0 0 100 100)
> -)

Seems like this test is broken?

[Sergio Villar Senin]

Comment on attachment 443106 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/editing/FrameSelection.cpp:470
>> +    auto document = Ref { *m_document };
> 
> Can't we just do: Ref document = *m_document?

Sure, didn't know that was preferred.

>> Source/WebCore/editing/FrameSelection.cpp:2515
>> +    updateAppearanceAfterLayout();
> 
> Why can't we call updateSelectionAppearanceNow instead? Please explain in the change log.

I guess we can indeed, I'll switch to that.

>> Source/WebCore/html/HTMLFormControlElement.cpp:495
>> +    document().selection().updateAppearanceIfRevealingSelectionIsNeeded();
> 
> Do: Ref { document().selection() }->updateAppearanceIfRevealingSelectionIsNeeded()
> so that there is no change of the document getting removed
> even if this form control element is adopted to another document.

I guess you mean Ref { document() }->selection(). The FrameSelection is not ref counted.

>> LayoutTests/ChangeLog:13
>> +        Same situation for other tests that do not involve repaint rects but a11y notifications. We
> 
> Nit: but *trigger accessibility tree updates*?

Yep, much more precise indeed.

>> LayoutTests/accessibility/mac/selection-boundary-userinfo.html:165
>> +        function yieldMainThread() {
> 
> waitForAnimationFrame? Also { should be on new line per WebKit style guideline.

OK will rename it.

>> LayoutTests/accessibility/mac/selection-boundary-userinfo.html:167
>> +            requestAnimationFrame(() => { resolve(); });
> 
> You can just do this: requestAnimationFrame(resolve);

OK.

>> LayoutTests/accessibility/mac/selection-boundary-userinfo.html:187
>> +          .then(() => moveAsync(true));
> 
> Add async keyword to the outer function and do:
> await moveAsync(true);
> await focusElement("input");
> ...

There is not really an "outer" function in this case, just a chain of alternating calls. I tried the async/await approach but never managed to get the right order in the calls.

>> LayoutTests/accessibility/mac/selection-change-userinfo.html:240
>> +          .then(() => resetPosition())
> 
> Please use async & await here. This is so much boilerplate!

Same reply than above, this was the only way I could make it work.

>> LayoutTests/fast/repaint/selection-gap-absolute-child.html:16
>> +        setTimeout(function() {
> 
> Why we don't need to wait for RAF? Please explain in the change log.

I could use any of those. I'll switch to RAF. Ditto for the comments below.

>> LayoutTests/platform/mac/TestExpectations:2450
>> +webkit.org/b/231298 accessibility/mac/selection-sync.html [ Skip ] # Timeout
> 
> We're losing test coverage here. Can we fix the test before we land this patch?

Not sure, Andres Gonzalez asked me to file the bug. He mentioned he'd take care.

>> LayoutTests/platform/win/fast/repaint/selection-gap-fixed-child-expected.txt:-7
>> -)
> 
> Seems like this test is broken?

I am honestly not sure. When I checked the expectations, this change make win match the mac-wk2 results so I assumed it was a step in the right direction.

[Sergio Villar Senin]

Created attachment 444286 [details]
Patch

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #39)
> Created attachment 444286 [details]
> Patch

After checking the EWS results it looks like using setTimeout() as in the original patch is safer than RAF which does produce flaky results in under stress conditions. Any thoughts on that Ryosuke?

[Ryosuke Niwa]

(In reply to Sergio Villar Senin from comment #40)
> (In reply to Sergio Villar Senin from comment #39)
> > Created attachment 444286 [details]
> > Patch
> 
> After checking the EWS results it looks like using setTimeout() as in the
> original patch is safer than RAF which does produce flaky results in under
> stress conditions. Any thoughts on that Ryosuke?

Okay, please mention that in the change log or in a comment so that people will know why we're using setTimeout in the future.

[Ryosuke Niwa]

Comment on attachment 444286 [details]
Patch

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

> LayoutTests/accessibility/mac/selection-boundary-userinfo.html:187
> +        focusElement("textbox")
> +          .then(() => moveAsync(true))
> +          .then(() => focusElement("input"))
> +          .then(() => moveAsync(false))
> +          .then(() => focusElement("textarea"))
> +          .then(() => moveAsync(true));

Wrap this in an anonymous function like so:
(async function () {
    await focusElement("textbox");
    await moveAsync(true);
    ...
})();

> LayoutTests/accessibility/mac/selection-change-userinfo.html:211
> +	function moveSelection(direction, granularity) {

Make this function async so that:
async function moveSelection(direction, granularity)
{

> LayoutTests/accessibility/mac/selection-change-userinfo.html:246
> +        focusTextBox()
> +          .then(() => resetPosition())
> +          .then(() => moveSelection("forward", gran[0]))
> +          .then(() => moveSelection("backward", gran[0]))
> +          .then(() => moveSelection("forward", gran[1]))
> +          .then(() => moveSelection("backward", gran[1]))
> +          .then(() => moveSelection("forward", gran[2]))
> +          .then(() => moveSelection("backward", gran[2]))

Then you should be able to do this here:
await focusTextBox();
await resetPosition();
await moveSelection("forward", gran[0]);
...

[Sergio Villar Senin]

Comment on attachment 444286 [details]
Patch

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

>> LayoutTests/accessibility/mac/selection-boundary-userinfo.html:187
>> +          .then(() => moveAsync(true));
> 
> Wrap this in an anonymous function like so:
> (async function () {
>     await focusElement("textbox");
>     await moveAsync(true);
>     ...
> })();

Right, completely forgot about your previous comment. Will replace it.

>> LayoutTests/accessibility/mac/selection-change-userinfo.html:211
>> +	function moveSelection(direction, granularity) {
> 
> Make this function async so that:
> async function moveSelection(direction, granularity)
> {

As it returns a Promise I think I can leave it as is and...

>> LayoutTests/accessibility/mac/selection-change-userinfo.html:246
>> +          .then(() => moveSelection("backward", gran[2]))
> 
> Then you should be able to do this here:
> await focusTextBox();
> await resetPosition();
> await moveSelection("forward", gran[0]);
> ...

Apply the same pattern than in the other file with the anonymous async function using await to wait for the results.

[Sergio Villar Senin]

(In reply to Ryosuke Niwa from comment #41)
> (In reply to Sergio Villar Senin from comment #40)
> > (In reply to Sergio Villar Senin from comment #39)
> > > Created attachment 444286 [details]
> > > Patch
> > 
> > After checking the EWS results it looks like using setTimeout() as in the
> > original patch is safer than RAF which does produce flaky results in under
> > stress conditions. Any thoughts on that Ryosuke?
> 
> Okay, please mention that in the change log or in a comment so that people
> will know why we're using setTimeout in the future.

Sure. I've also fixed the indentation which was kind of weird in the previous patch I posted.

[Sergio Villar Senin]

Created attachment 444528 [details]
Patch

[Sergio Villar Senin]

Gentle ping for reviewers

[Ryosuke Niwa]

Comment on attachment 444528 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:2515
> +    m_document->updateLayoutIgnorePendingStylesheets();

Why do we need this separate layout update in addition to calling updateSelectionAppearanceNow below?
Please explain it in the change log or even add a comment here.

> Source/WebCore/html/HTMLFormControlElement.cpp:496
>      focus();
> +    Ref { document() }->selection().updateAppearanceIfRevealingSelectionIsNeeded();

Why is calling focus() not enough here? Please explain it in the change log and/or add a comment here.

[Sergio Villar Senin]

Created attachment 449486 [details]
Patch for landing

[Sergio Villar Senin]

Created attachment 449559 [details]
Patch for landing

[Sergio Villar Senin]

Created attachment 449575 [details]
Patch for landing

[Sergio Villar Senin]

Created attachment 450422 [details]
Patch for landing

[Sergio Villar Senin]

Created attachment 450429 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 450429 [details]
Patch for landing

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

> Source/WebCore/html/HTMLFormControlElement.cpp:35
> +#include "FrameSelection.h"

Is this new include needed?

[Sergio Villar Senin]

Created attachment 450525 [details]
Patch

[Sergio Villar Senin]

(In reply to Ryosuke Niwa from comment #53)
> Comment on attachment 450429 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450429&action=review
> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:35
> > +#include "FrameSelection.h"
> 
> Is this new include needed?

Not really, it was automatically added by my IDE (it compiles units individually).

BTW, if we land this I guess I should send something to the webkit-dev list mentioning the behaviour change, because people might develop tests in which they expect that setting/extending a selection is a synchronous operation. I've checked the specs (both the HTML5 and the selection one) and didn't see any reference to whether the operation should be sync/async so I guess we're safe on that side, but still I guess a clarification does not hurt. Ryosuke WDYT?

[Sergio Villar Senin]

Created attachment 450526 [details]
Patch

[Wenson Hsieh]

(In reply to Sergio Villar Senin from comment #56)
> Created attachment 450526 [details]
> Patch

…

> {
>     "Skipped": [],
>     "Failed": [],
>     "Crashed": [],
>     "Timedout": [
>         {
>             "name": "TestWebKitAPI.AppHighlights.AppHighlightCreateAndRestoreAndScroll",
>             "output": "2022-02-01 10:11:04.246 TestWebKitAPI[26200:292609] Writing analzed variants.\n2022-02-01 10:11:05.778 TestWebKitAPI[26200:292609] Writing analzed variants.\n2022-02-01 10:11:08.339 TestWebKitAPI[26200:292609] Expected Highlights to be populated and the page to scroll.\n"
>         }
>     ]
> }

I looked into the failing API test in the latest iteration of the patch, and it looks like a legitimate bug: what's happening here is that App Highlight restoration code (i.e., when restoring and scrolling to reveal a Quick Note) uses TemporarySelectionChange to select and scroll to reveal the highlight range; currently, we assume that this scrolling happens synchronously, since we revert the selection to the original range at the end of `AppHighlightStorage::attemptToRestoreHighlightAndScroll` when the TemporarySelectionChange falls out of scope.

However, since this isn't a user-triggered selection change, scrolling no longer happens immediately after this patch. Instead, no scrolling occurs at all, since we end up only scheduling the appearance update timer before setting the selection back to the original state with `SelectionRevealMode::DoNotReveal`.

I think it's *probably* sensible to handle this like a user triggered selection change though, since this only happens when the user interacts with a Quick Note from within the Notes app on iOS and macOS. Something like this should be sufficient to fix the failing test (by making the app highlight scrolling user-triggered):

```
diff --git a/Source/WebCore/Modules/highlight/AppHighlightStorage.cpp b/Source/WebCore/Modules/highlight/AppHighlightStorage.cpp
index 7b2e15b0bac6..a2636e092ec6 100644
--- a/Source/WebCore/Modules/highlight/AppHighlightStorage.cpp
+++ b/Source/WebCore/Modules/highlight/AppHighlightStorage.cpp
@@ -272,7 +272,7 @@ bool AppHighlightStorage::attemptToRestoreHighlightAndScroll(AppHighlightRangeDa
         if (textIndicator)
             m_document->page()->chrome().client().setTextIndicator(textIndicator->data());

-        TemporarySelectionChange selectionChange(*strongDocument, { range.value() }, { TemporarySelectionOption::DelegateMainFrameScroll, TemporarySelectionOption::SmoothScroll, TemporarySelectionOption::RevealSelectionBounds });
+        TemporarySelectionChange selectionChange(*strongDocument, { *range }, { TemporarySelectionOption::DelegateMainFrameScroll, TemporarySelectionOption::SmoothScroll, TemporarySelectionOption::RevealSelectionBounds, TemporarySelectionOption::UserTriggered });
     }

     return true;
diff --git a/Source/WebCore/editing/Editor.cpp b/Source/WebCore/editing/Editor.cpp
index 344645c34bd2..496ff0fedb89 100644
--- a/Source/WebCore/editing/Editor.cpp
+++ b/Source/WebCore/editing/Editor.cpp
@@ -252,7 +252,7 @@ TemporarySelectionChange::~TemporarySelectionChange()

 void TemporarySelectionChange::setSelection(const VisibleSelection& selection, IsTemporarySelection isTemporarySelection)
 {
-    auto options = FrameSelection::defaultSetSelectionOptions();
+    auto options = FrameSelection::defaultSetSelectionOptions(m_options.contains(TemporarySelectionOption::UserTriggered) ? UserTriggered : NotUserTriggered);
     if (m_options & TemporarySelectionOption::DoNotSetFocus)
         options.add(FrameSelection::DoNotSetFocus);

diff --git a/Source/WebCore/editing/Editor.h b/Source/WebCore/editing/Editor.h
index 9f726fd27312..a0eb032e767b 100644
--- a/Source/WebCore/editing/Editor.h
+++ b/Source/WebCore/editing/Editor.h
@@ -125,6 +125,7 @@ enum class TemporarySelectionOption : uint8_t {
     DelegateMainFrameScroll = 1 << 5,

     RevealSelectionBounds = 1 << 6,
+    UserTriggered = 1 << 7,
 };

 class TemporarySelectionChange {
```

[Sergio Villar Senin]

Created attachment 454226 [details]
Patch

[Sergio Villar Senin]

Created attachment 454235 [details]
Patch

[Sergio Villar Senin]

Created attachment 455379 [details]
Patch for landing

[Sergio Villar Senin]

Committed r291789 (?): <https://commits.webkit.org/r291789>

[Jon Lee]

This is causing debug assertions on the bots
https://build.webkit.org/results/Apple-iOS-15-Simulator-Debug-WK2-Tests/r291791%20(2056)/results.html

[Dawn Flores]

(In reply to Jon Lee from comment #62)
> This is causing debug assertions on the bots
> https://build.webkit.org/results/Apple-iOS-15-Simulator-Debug-WK2-Tests/
> r291791%20(2056)/results.html

Indeed it is. Reverted for now. Caused 65+ API failures on iOS Debug.

[Dawn Flores]

Reverted r291789 for reason:

This revision caused 65+ new API failures on iOS debug

Committed r291807 (248834@trunk): <https://commits.webkit.org/248834@trunk>

[Gabriel Nava Marino]

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

[Michael Saboff]

Gabriel will look at the API test crashes caused by the now rolled out patch.

[Ryosuke Niwa]

(In reply to Michael Saboff from comment #66)
> Gabriel will look at the API test crashes caused by the now rolled out patch.

I've already looked into it. The patch (and the approach) doesn't work.
We need to make this selection update async.
Talk to Wenson about it.

[Gabriel Nava Marino]

(In reply to Ryosuke Niwa from comment #67)
> (In reply to Michael Saboff from comment #66)
> > Gabriel will look at the API test crashes caused by the now rolled out patch.
> 
> I've already looked into it. The patch (and the approach) doesn't work.
> We need to make this selection update async.
> Talk to Wenson about it.

My understanding is that Sergio's patch above will make this selection update async? See ChangeLog:

> In order to fix that, we can switch to a model in which we update the selection asynchronously
> in the case of having a non-user triggered change. That way we don't do it inside
> a restricted layout scope.

This is the patch Wenson pointed me to after asking about Bug 239293.

[Rob Buis]

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

[Gabriel Nava Marino]

Created attachment 459188 [details]
Patch

[Gabriel Nava Marino]

Comment on attachment 459188 [details]
Patch

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

I've re-uploaded Sergio's patch as originally landed, with two changes.

> Source/WebCore/editing/FrameSelection.cpp:472
> +    if (!m_document)

This is added to the original patch to prevent the crash causing the iOS test failures.

> LayoutTests/editing/selection-with-absolute-positioned-empty-content.html:23
> +requestAnimationFrame(() => {

Changed setTimeout to requestAnimationFrame as setTimeout was causing the test to fail unless the delay was set to a non-zero value (i.e. 1ms).

[Ryosuke Niwa]

Comment on attachment 459188 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:2549
> +void FrameSelection::updateAppearanceIfRevealingSelectionIsNeeded()

Where is this ever called??

>> LayoutTests/editing/selection-with-absolute-positioned-empty-content.html:23
>> +requestAnimationFrame(() => {
> 
> Changed setTimeout to requestAnimationFrame as setTimeout was causing the test to fail unless the delay was set to a non-zero value (i.e. 1ms).

Try this instead:
<script>
if (window.testRunner) {
    testRunner.dumpAsText();
    testRunner.waitUntilDone();
}

function timerAfterRAF(callback) {
    return requestAnimationFrame(() => setTimeout(callback, 0));
}

timerAfterRAF(() => {
    if (window.internals)
        internals.startTrackingRepaints();
    window.getSelection().selectAllChildren(foobar);
    timerAfterRAF(() => {
        if (window.internals) {
            result.innerText = internals.repaintRectsAsText();
            internals.stopTrackingRepaints();
        }
        if (window.testRunner)
            testRunner.notifyDone();
    });
});
</script>

[Gabriel Nava Marino]

(In reply to Ryosuke Niwa from comment #72)
> Comment on attachment 459188 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459188&action=review
> 
> > Source/WebCore/editing/FrameSelection.cpp:2549
> > +void FrameSelection::updateAppearanceIfRevealingSelectionIsNeeded()
> 
> Where is this ever called??

Good catch! Looks like this was used in an older patch, but was subsequently removed https://bugs.webkit.org/attachment.cgi?id=449559&action=prettypatch.

I will clean it up.

> 
> >> LayoutTests/editing/selection-with-absolute-positioned-empty-content.html:23
> >> +requestAnimationFrame(() => {
> > 
> > Changed setTimeout to requestAnimationFrame as setTimeout was causing the test to fail unless the delay was set to a non-zero value (i.e. 1ms).
> 
> Try this instead:
> <script>
> if (window.testRunner) {
>     testRunner.dumpAsText();
>     testRunner.waitUntilDone();
> }
> 
> function timerAfterRAF(callback) {
>     return requestAnimationFrame(() => setTimeout(callback, 0));
> }
> 
> timerAfterRAF(() => {
>     if (window.internals)
>         internals.startTrackingRepaints();
>     window.getSelection().selectAllChildren(foobar);
>     timerAfterRAF(() => {
>         if (window.internals) {
>             result.innerText = internals.repaintRectsAsText();
>             internals.stopTrackingRepaints();
>         }
>         if (window.testRunner)
>             testRunner.notifyDone();
>     });
> });
> </script>

Thank you! I will try this approach.

[Gabriel Nava Marino]

Created attachment 459235 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 459235 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> [phone]  Sergio Villar Senin  <[e-mail]>

Let's not use another person's name as the author since you're posting this patch now.
Mention that this patch is based on another patch made by Sergio Villar Senin instead.

> LayoutTests/ChangeLog:1
> [phone]  Sergio Villar Senin  <[e-mail]>

Ditto.

> LayoutTests/platform/win/fast/repaint/selection-gap-fixed-child-expected.txt:5
> -(repaint rects
> -  (rect 0 0 100 100)
> -)
>  

This looks like a regression.
This test probably also needs fixing.

[Ryosuke Niwa]

Comment on attachment 459235 [details]
Patch

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

>> LayoutTests/platform/win/fast/repaint/selection-gap-fixed-child-expected.txt:5
>>  
> 
> This looks like a regression.
> This test probably also needs fixing.

This needs the same fix as other tests that uses text-based-repaint.js

[Ryosuke Niwa]

Comment on attachment 459235 [details]
Patch

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

>> LayoutTests/platform/win/fast/repaint/selection-gap-fixed-child-expected.txt:5
>>  
> 
> This looks like a regression.
> This test probably also needs fixing.

This needs the same fix as other tests that uses text-based-repaint.js

[Gabriel Nava Marino]

(In reply to Ryosuke Niwa from comment #77)
> Comment on attachment 459235 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459235&action=review
> 
> >> LayoutTests/platform/win/fast/repaint/selection-gap-fixed-child-expected.txt:5
> >>  
> > 
> > This looks like a regression.
> > This test probably also needs fixing.
> 
> This needs the same fix as other tests that uses text-based-repaint.js

Earlier Sergio mentioned this was inline now with mac-wk2 results. Is this no longer the case? See below:

> >> LayoutTests/platform/win/fast/repaint/selection-gap-fixed-child-expected.txt:-7
> >> -)
> > 
> > Seems like this test is broken?


> I am honestly not sure. When I checked the expectations, this change make win match the mac-wk2 results so I assumed it was a step in the right direction.

[Darin Adler]

Comment on attachment 459235 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:2380
> +    const_cast<FrameSelection&>(*this).updateSelectionAppearanceNow();

If this function can have a side effect of updating selection appearance, maybe we should make it non-const to signal that rather than adding the const_cast. Of course, we have to check if that ripples through other code in an unpleasant way.

> LayoutTests/fast/repaint/selection-ruby-rl.html:23
> +    // requestAnimationFrame produces flacky results.

The word is "flaky", not "flacky".

Also, this is a mysterious comment. Why do we have to wait at all? Is there a good reason this *should* be using requestAnimationFrame? Do we know why it produces flaky results? Why is setTimeout an acceptable alternative? I honestly would be OK with no comment at all. But if we do have a comment I’d like to understand it better.

[Ryosuke Niwa]

(In reply to Gabriel Nava Marino from comment #78)
> (In reply to Ryosuke Niwa from comment #77)
> > Comment on attachment 459235 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=459235&action=review
>
> > >> LayoutTests/platform/win/fast/repaint/selection-gap-fixed-child-expected.txt:-7
> > >> -)
> > > 
> > > Seems like this test is broken?
> 
> 
> > I am honestly not sure. When I checked the expectations, this change make win match the mac-wk2 results so I assumed it was a step in the right direction.

I don't think so. Check out the original expected result here:
https://trac.webkit.org/browser/trunk/LayoutTests/fast/repaint/selection-gap-fixed-child-expected.txt

The test was added to validate that selection gaps are repainted. If we're missing rects, then we're either not testing it right or not repainting. This test needs the same fix as other tests that use text-based-repaint.js like LayoutTests/fast/repaint/selection-gap-absolute-child.html

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #79)
> Comment on attachment 459235 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459235&action=review
> 
> > Source/WebCore/editing/FrameSelection.cpp:2380
> > +    const_cast<FrameSelection&>(*this).updateSelectionAppearanceNow();
> 
> If this function can have a side effect of updating selection appearance,
> maybe we should make it non-const to signal that rather than adding the
> const_cast. Of course, we have to check if that ripples through other code
> in an unpleasant way.

I have made it non-const and there were no unpleasant side-effects.

> 
> > LayoutTests/fast/repaint/selection-ruby-rl.html:23
> > +    // requestAnimationFrame produces flacky results.
> 
> The word is "flaky", not "flacky".
> 
> Also, this is a mysterious comment. Why do we have to wait at all? Is there
> a good reason this *should* be using requestAnimationFrame? Do we know why
> it produces flaky results? Why is setTimeout an acceptable alternative? I
> honestly would be OK with no comment at all. But if we do have a comment I’d
> like to understand it better.

I don't believe so, other than "the results with the latter were not as reliable under stress/debug conditions" as mentioned in the LayoutTests/ChangeLog and so I have removed this comment in all tests where it was added.

[Gabriel Nava Marino]

(In reply to Ryosuke Niwa from comment #80)
> (In reply to Gabriel Nava Marino from comment #78)
> > (In reply to Ryosuke Niwa from comment #77)
> > > Comment on attachment 459235 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=459235&action=review
> >
> > > >> LayoutTests/platform/win/fast/repaint/selection-gap-fixed-child-expected.txt:-7
> > > >> -)
> > > > 
> > > > Seems like this test is broken?
> > 
> > 
> > > I am honestly not sure. When I checked the expectations, this change make win match the mac-wk2 results so I assumed it was a step in the right direction.
> 
> I don't think so. Check out the original expected result here:
> https://trac.webkit.org/browser/trunk/LayoutTests/fast/repaint/selection-gap-
> fixed-child-expected.txt
> 
> The test was added to validate that selection gaps are repainted. If we're
> missing rects, then we're either not testing it right or not repainting.
> This test needs the same fix as other tests that use text-based-repaint.js
> like LayoutTests/fast/repaint/selection-gap-absolute-child.html

Thank you for the clarification Ryosuke! I have updated fast/repaint/selection-gap-fixed-child.html to have the same fix as other tests that use text-based-repaint.js and reverted the changes to LayoutTests/platform/win/fast/repaint/selection-gap-fixed-child-expected.txt.

[Gabriel Nava Marino]

Created attachment 459454 [details]
Patch

[Brandon]

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

[EWS]

Committed r294523 (250778@main): <https://commits.webkit.org/250778@main>

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

[Karl Rackler]

Reverting  r294523 (250778@main) due to causing iOS Debug 68 API failures and 50 Layout tests crashes.

[Karl Rackler]

Reopen to investigate issue.

[Brandon]

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

[Brandon]

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

[EWS]

Committed r294610 (250836@main): <https://commits.webkit.org/250836@main>

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

[Rob Buis]

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