Bug 224471 - Release assert in Document::updateLayout() via HTMLTextAreaElement::childrenChanged
Summary: Release assert in Document::updateLayout() via HTMLTextAreaElement::childrenC...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brandon
URL:
Keywords: InRadar
: 239293 240255 241154 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-04-12 21:50 PDT by Ryosuke Niwa
Modified: 2022-06-07 17:37 PDT (History)
36 users (show)

See Also:


Attachments
WIP (7.33 KB, patch)
2021-04-12 21:53 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Test (278 bytes, text/html)
2021-04-20 19:36 PDT, Ryosuke Niwa
no flags Details
Patch (2.05 KB, patch)
2021-05-19 10:00 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (23.88 KB, patch)
2021-06-07 10:22 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (28.45 KB, patch)
2021-06-25 08:55 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (33.96 KB, patch)
2021-08-25 11:50 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (37.12 KB, patch)
2021-08-26 00:57 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (36.75 KB, patch)
2021-10-06 09:50 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (36.75 KB, patch)
2021-10-06 12:30 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (40.24 KB, patch)
2021-10-07 02:30 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (41.88 KB, patch)
2021-10-08 03:15 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (40.83 KB, patch)
2021-11-02 01:49 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (40.90 KB, patch)
2021-11-02 10:35 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (40.93 KB, patch)
2021-11-15 11:50 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (41.16 KB, patch)
2021-11-17 08:52 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (40.53 KB, patch)
2022-01-19 08:45 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (40.95 KB, patch)
2022-01-20 01:22 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (41.77 KB, patch)
2022-01-20 08:08 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (42.07 KB, patch)
2022-01-31 11:32 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (42.26 KB, patch)
2022-01-31 12:12 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (42.85 KB, patch)
2022-02-01 07:27 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (42.81 KB, patch)
2022-02-01 07:41 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (46.61 KB, patch)
2022-03-09 04:13 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (46.61 KB, patch)
2022-03-09 07:11 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (47.54 KB, patch)
2022-03-22 09:19 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (47.29 KB, patch)
2022-05-11 16:35 PDT, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (46.91 KB, patch)
2022-05-12 10:00 PDT, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (48.26 KB, patch)
2022-05-16 14:21 PDT, Gabriel Nava Marino
rniwa: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2021-04-12 21:50:40 PDT
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>
Comment 1 Ryosuke Niwa 2021-04-12 21:53:40 PDT
Created attachment 425833 [details]
WIP
Comment 2 Ryosuke Niwa 2021-04-20 19:36:54 PDT
Created attachment 426637 [details]
Test
Comment 3 Ryosuke Niwa 2021-04-20 19:37:12 PDT
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)
Comment 4 Sergio Villar Senin 2021-05-19 10:00:27 PDT
Created attachment 429057 [details]
Patch
Comment 5 Sergio Villar Senin 2021-05-19 10:03:18 PDT
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.
Comment 6 Ryosuke Niwa 2021-05-19 14:09:06 PDT
(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.
Comment 7 Ryosuke Niwa 2021-05-19 14:10:03 PDT
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?
Comment 8 Sergio Villar Senin 2021-05-20 02:50:20 PDT
(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.
Comment 9 Ryosuke Niwa 2021-05-20 02:52:50 PDT
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.
Comment 10 Sergio Villar Senin 2021-06-07 10:22:29 PDT
Created attachment 430760 [details]
Patch
Comment 11 Ryosuke Niwa 2021-06-07 15:09:00 PDT
Hm... looks like there are a few test failures?
Comment 12 Sergio Villar Senin 2021-06-11 03:28:53 PDT
(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.
Comment 13 Sergio Villar Senin 2021-06-25 08:55:33 PDT
Created attachment 432276 [details]
Patch

Fixing some a11y tests. Still some of them failing
Comment 14 Sergio Villar Senin 2021-08-25 11:50:58 PDT
Created attachment 436407 [details]
Patch
Comment 15 Sergio Villar Senin 2021-08-26 00:57:19 PDT
Created attachment 436479 [details]
Patch
Comment 16 Ryosuke Niwa 2021-08-28 15:17:53 PDT
Comment on attachment 436479 [details]
Patch

Looks like some tests fare failing?
Comment 17 Sergio Villar Senin 2021-08-30 00:21:21 PDT
(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.
Comment 18 Brady Eidson 2021-10-01 16:40:42 PDT
(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?
Comment 19 James Craig 2021-10-01 16:42:14 PDT
Chris F or Andres G, perhaps.
Comment 20 chris fleizach 2021-10-01 17:02:12 PDT
(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    }
Comment 21 Sergio Villar Senin 2021-10-03 15:39:08 PDT
(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?
Comment 22 Andres Gonzalez 2021-10-04 07:39:13 PDT
(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.
Comment 23 Sergio Villar Senin 2021-10-06 08:13:54 PDT
(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.
Comment 24 Andres Gonzalez 2021-10-06 08:25:54 PDT
(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.
Comment 25 Sergio Villar Senin 2021-10-06 08:57:38 PDT
(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.
Comment 26 Andres Gonzalez 2021-10-06 09:04:03 PDT
(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.
Comment 27 Sergio Villar Senin 2021-10-06 09:50:21 PDT
Created attachment 440381 [details]
Patch
Comment 28 Sergio Villar Senin 2021-10-06 09:51:37 PDT
(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!
Comment 29 Sergio Villar Senin 2021-10-06 12:30:55 PDT
Created attachment 440413 [details]
Patch
Comment 30 Sergio Villar Senin 2021-10-07 02:30:22 PDT
Created attachment 440491 [details]
Patch

Win and WK1 fixes
Comment 31 Sergio Villar Senin 2021-10-08 03:15:29 PDT
Created attachment 440587 [details]
Patch

More mac expectations
Comment 32 Michael Saboff 2021-10-28 14:20:47 PDT
Comment on attachment 440587 [details]
Patch

Looks like there are still some issues with updated tests.
Comment 33 Sergio Villar Senin 2021-11-02 01:49:49 PDT
Created attachment 443070 [details]
Patch
Comment 34 Sergio Villar Senin 2021-11-02 10:35:36 PDT
Created attachment 443106 [details]
Patch
Comment 35 Sergio Villar Senin 2021-11-04 02:00:06 PDT
Gentle ping for reviewers
Comment 36 Sergio Villar Senin 2021-11-10 03:11:10 PST
Ping
Comment 37 Ryosuke Niwa 2021-11-14 11:22:04 PST
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?
Comment 38 Sergio Villar Senin 2021-11-15 11:40:13 PST
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.
Comment 39 Sergio Villar Senin 2021-11-15 11:50:22 PST
Created attachment 444286 [details]
Patch
Comment 40 Sergio Villar Senin 2021-11-16 02:28:40 PST
(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?
Comment 41 Ryosuke Niwa 2021-11-16 13:29:08 PST
(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.
Comment 42 Ryosuke Niwa 2021-11-16 13:31:31 PST
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]);
...
Comment 43 Sergio Villar Senin 2021-11-17 08:43:27 PST
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.
Comment 44 Sergio Villar Senin 2021-11-17 08:49:42 PST
(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.
Comment 45 Sergio Villar Senin 2021-11-17 08:52:51 PST
Created attachment 444528 [details]
Patch
Comment 46 Sergio Villar Senin 2022-01-13 06:42:40 PST
Gentle ping for reviewers
Comment 47 Ryosuke Niwa 2022-01-19 00:24:51 PST
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.
Comment 48 Sergio Villar Senin 2022-01-19 08:45:33 PST
Created attachment 449486 [details]
Patch for landing
Comment 49 Sergio Villar Senin 2022-01-20 01:22:59 PST
Created attachment 449559 [details]
Patch for landing
Comment 50 Sergio Villar Senin 2022-01-20 08:08:13 PST
Created attachment 449575 [details]
Patch for landing
Comment 51 Sergio Villar Senin 2022-01-31 11:32:48 PST
Created attachment 450422 [details]
Patch for landing
Comment 52 Sergio Villar Senin 2022-01-31 12:12:44 PST
Created attachment 450429 [details]
Patch for landing
Comment 53 Ryosuke Niwa 2022-01-31 13:21:10 PST
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?
Comment 54 Sergio Villar Senin 2022-02-01 07:27:24 PST
Created attachment 450525 [details]
Patch
Comment 55 Sergio Villar Senin 2022-02-01 07:32:24 PST
(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?
Comment 56 Sergio Villar Senin 2022-02-01 07:41:16 PST
Created attachment 450526 [details]
Patch
Comment 57 Wenson Hsieh 2022-03-06 16:05:47 PST
(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 {
```
Comment 58 Sergio Villar Senin 2022-03-09 04:13:43 PST
Created attachment 454226 [details]
Patch
Comment 59 Sergio Villar Senin 2022-03-09 07:11:20 PST
Created attachment 454235 [details]
Patch
Comment 60 Sergio Villar Senin 2022-03-22 09:19:17 PDT
Created attachment 455379 [details]
Patch for landing
Comment 61 Sergio Villar Senin 2022-03-24 01:33:52 PDT
Committed r291789 (?): <https://commits.webkit.org/r291789>
Comment 62 Jon Lee 2022-03-24 10:21:19 PDT
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
Comment 63 Matteo Flores 2022-03-24 11:05:36 PDT
(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.
Comment 64 Matteo Flores 2022-03-24 11:16:39 PDT
Reverted r291789 for reason:

This revision caused 65+ new API failures on iOS debug

Committed r291807 (248834@trunk): <https://commits.webkit.org/248834@trunk>
Comment 65 Gabriel Nava Marino 2022-04-29 13:42:32 PDT
*** Bug 239293 has been marked as a duplicate of this bug. ***
Comment 66 Michael Saboff 2022-05-09 08:55:32 PDT
Gabriel will look at the API test crashes caused by the now rolled out patch.
Comment 67 Ryosuke Niwa 2022-05-09 08:58:33 PDT
(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.
Comment 68 Gabriel Nava Marino 2022-05-09 09:08:16 PDT
(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.
Comment 69 Rob Buis 2022-05-11 14:31:14 PDT
*** Bug 240255 has been marked as a duplicate of this bug. ***
Comment 70 Gabriel Nava Marino 2022-05-11 16:35:09 PDT
Created attachment 459188 [details]
Patch
Comment 71 Gabriel Nava Marino 2022-05-11 16:41:05 PDT
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).
Comment 72 Ryosuke Niwa 2022-05-11 22:23:16 PDT
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>
Comment 73 Gabriel Nava Marino 2022-05-12 09:45:43 PDT
(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.
Comment 74 Gabriel Nava Marino 2022-05-12 10:00:19 PDT
Created attachment 459235 [details]
Patch
Comment 75 Ryosuke Niwa 2022-05-12 22:13:24 PDT
Comment on attachment 459235 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> +2022-05-11  Sergio Villar Senin  <svillar@igalia.com>

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
> +2022-05-11  Sergio Villar Senin  <svillar@igalia.com>

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.
Comment 76 Ryosuke Niwa 2022-05-12 22:16:37 PDT
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
Comment 77 Ryosuke Niwa 2022-05-12 22:16:39 PDT
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
Comment 78 Gabriel Nava Marino 2022-05-13 11:35:10 PDT
(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.
Comment 79 Darin Adler 2022-05-13 13:05:39 PDT
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.
Comment 80 Ryosuke Niwa 2022-05-13 21:52:27 PDT
(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
Comment 81 Gabriel Nava Marino 2022-05-16 14:12:18 PDT
(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.
Comment 82 Gabriel Nava Marino 2022-05-16 14:14:19 PDT
(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.
Comment 83 Gabriel Nava Marino 2022-05-16 14:21:17 PDT
Created attachment 459454 [details]
Patch
Comment 84 Brandon 2022-05-18 13:00:37 PDT
Pull request: https://github.com/WebKit/WebKit/pull/742
Comment 85 EWS 2022-05-19 19:15:31 PDT
Committed r294523 (250778@main): <https://commits.webkit.org/250778@main>

Reviewed commits have been landed. Closing PR #742 and removing active labels.
Comment 86 Karl Rackler 2022-05-20 08:39:31 PDT
Reverting  r294523 (250778@main) due to causing iOS Debug 68 API failures and 50 Layout tests crashes.
Comment 87 Karl Rackler 2022-05-20 08:51:23 PDT
Reopen to investigate issue.
Comment 88 Brandon 2022-05-20 11:07:16 PDT
Pull request: https://github.com/WebKit/WebKit/pull/840
Comment 89 Brandon 2022-05-20 11:15:57 PDT
Pull request: https://github.com/WebKit/WebKit/pull/841
Comment 90 EWS 2022-05-20 23:09:50 PDT
Committed r294610 (250836@main): <https://commits.webkit.org/250836@main>

Reviewed commits have been landed. Closing PR #841 and removing active labels.
Comment 91 Rob Buis 2022-06-07 04:56:00 PDT
*** Bug 241154 has been marked as a duplicate of this bug. ***