RESOLVED FIXED 224471
Release assert in Document::updateLayout() via HTMLTextAreaElement::childrenChanged
https://bugs.webkit.org/show_bug.cgi?id=224471
Summary Release assert in Document::updateLayout() via HTMLTextAreaElement::childrenC...
Ryosuke Niwa
Reported 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>
Attachments
WIP (7.33 KB, patch)
2021-04-12 21:53 PDT, Ryosuke Niwa
no flags
Test (278 bytes, text/html)
2021-04-20 19:36 PDT, Ryosuke Niwa
no flags
Patch (2.05 KB, patch)
2021-05-19 10:00 PDT, Sergio Villar Senin
no flags
Patch (23.88 KB, patch)
2021-06-07 10:22 PDT, Sergio Villar Senin
no flags
Patch (28.45 KB, patch)
2021-06-25 08:55 PDT, Sergio Villar Senin
no flags
Patch (33.96 KB, patch)
2021-08-25 11:50 PDT, Sergio Villar Senin
no flags
Patch (37.12 KB, patch)
2021-08-26 00:57 PDT, Sergio Villar Senin
no flags
Patch (36.75 KB, patch)
2021-10-06 09:50 PDT, Sergio Villar Senin
no flags
Patch (36.75 KB, patch)
2021-10-06 12:30 PDT, Sergio Villar Senin
no flags
Patch (40.24 KB, patch)
2021-10-07 02:30 PDT, Sergio Villar Senin
no flags
Patch (41.88 KB, patch)
2021-10-08 03:15 PDT, Sergio Villar Senin
no flags
Patch (40.83 KB, patch)
2021-11-02 01:49 PDT, Sergio Villar Senin
no flags
Patch (40.90 KB, patch)
2021-11-02 10:35 PDT, Sergio Villar Senin
no flags
Patch (40.93 KB, patch)
2021-11-15 11:50 PST, Sergio Villar Senin
no flags
Patch (41.16 KB, patch)
2021-11-17 08:52 PST, Sergio Villar Senin
no flags
Patch for landing (40.53 KB, patch)
2022-01-19 08:45 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Patch for landing (40.95 KB, patch)
2022-01-20 01:22 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Patch for landing (41.77 KB, patch)
2022-01-20 08:08 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Patch for landing (42.07 KB, patch)
2022-01-31 11:32 PST, Sergio Villar Senin
no flags
Patch for landing (42.26 KB, patch)
2022-01-31 12:12 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Patch (42.85 KB, patch)
2022-02-01 07:27 PST, Sergio Villar Senin
no flags
Patch (42.81 KB, patch)
2022-02-01 07:41 PST, Sergio Villar Senin
no flags
Patch (46.61 KB, patch)
2022-03-09 04:13 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Patch (46.61 KB, patch)
2022-03-09 07:11 PST, Sergio Villar Senin
ews-feeder: commit-queue-
Patch for landing (47.54 KB, patch)
2022-03-22 09:19 PDT, Sergio Villar Senin
no flags
Patch (47.29 KB, patch)
2022-05-11 16:35 PDT, Gabriel Nava Marino
no flags
Patch (46.91 KB, patch)
2022-05-12 10:00 PDT, Gabriel Nava Marino
no flags
Patch (48.26 KB, patch)
2022-05-16 14:21 PDT, Gabriel Nava Marino
rniwa: review+
ews-feeder: commit-queue-
Ryosuke Niwa
Comment 1 2021-04-12 21:53:40 PDT
Ryosuke Niwa
Comment 2 2021-04-20 19:36:54 PDT
Ryosuke Niwa
Comment 3 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)
Sergio Villar Senin
Comment 4 2021-05-19 10:00:27 PDT
Sergio Villar Senin
Comment 5 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.
Ryosuke Niwa
Comment 6 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.
Ryosuke Niwa
Comment 7 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?
Sergio Villar Senin
Comment 8 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.
Ryosuke Niwa
Comment 9 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.
Sergio Villar Senin
Comment 10 2021-06-07 10:22:29 PDT
Ryosuke Niwa
Comment 11 2021-06-07 15:09:00 PDT
Hm... looks like there are a few test failures?
Sergio Villar Senin
Comment 12 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.
Sergio Villar Senin
Comment 13 2021-06-25 08:55:33 PDT
Created attachment 432276 [details] Patch Fixing some a11y tests. Still some of them failing
Sergio Villar Senin
Comment 14 2021-08-25 11:50:58 PDT
Sergio Villar Senin
Comment 15 2021-08-26 00:57:19 PDT
Ryosuke Niwa
Comment 16 2021-08-28 15:17:53 PDT
Comment on attachment 436479 [details] Patch Looks like some tests fare failing?
Sergio Villar Senin
Comment 17 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.
Brady Eidson
Comment 18 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?
James Craig
Comment 19 2021-10-01 16:42:14 PDT
Chris F or Andres G, perhaps.
chris fleizach
Comment 20 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 }
Sergio Villar Senin
Comment 21 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?
Andres Gonzalez
Comment 22 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.
Sergio Villar Senin
Comment 23 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.
Andres Gonzalez
Comment 24 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.
Sergio Villar Senin
Comment 25 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.
Andres Gonzalez
Comment 26 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.
Sergio Villar Senin
Comment 27 2021-10-06 09:50:21 PDT
Sergio Villar Senin
Comment 28 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!
Sergio Villar Senin
Comment 29 2021-10-06 12:30:55 PDT
Sergio Villar Senin
Comment 30 2021-10-07 02:30:22 PDT
Created attachment 440491 [details] Patch Win and WK1 fixes
Sergio Villar Senin
Comment 31 2021-10-08 03:15:29 PDT
Created attachment 440587 [details] Patch More mac expectations
Michael Saboff
Comment 32 2021-10-28 14:20:47 PDT
Comment on attachment 440587 [details] Patch Looks like there are still some issues with updated tests.
Sergio Villar Senin
Comment 33 2021-11-02 01:49:49 PDT
Sergio Villar Senin
Comment 34 2021-11-02 10:35:36 PDT
Sergio Villar Senin
Comment 35 2021-11-04 02:00:06 PDT
Gentle ping for reviewers
Sergio Villar Senin
Comment 36 2021-11-10 03:11:10 PST
Ping
Ryosuke Niwa
Comment 37 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?
Sergio Villar Senin
Comment 38 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.
Sergio Villar Senin
Comment 39 2021-11-15 11:50:22 PST
Sergio Villar Senin
Comment 40 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?
Ryosuke Niwa
Comment 41 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.
Ryosuke Niwa
Comment 42 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]); ...
Sergio Villar Senin
Comment 43 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.
Sergio Villar Senin
Comment 44 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.
Sergio Villar Senin
Comment 45 2021-11-17 08:52:51 PST
Sergio Villar Senin
Comment 46 2022-01-13 06:42:40 PST
Gentle ping for reviewers
Ryosuke Niwa
Comment 47 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.
Sergio Villar Senin
Comment 48 2022-01-19 08:45:33 PST
Created attachment 449486 [details] Patch for landing
Sergio Villar Senin
Comment 49 2022-01-20 01:22:59 PST
Created attachment 449559 [details] Patch for landing
Sergio Villar Senin
Comment 50 2022-01-20 08:08:13 PST
Created attachment 449575 [details] Patch for landing
Sergio Villar Senin
Comment 51 2022-01-31 11:32:48 PST
Created attachment 450422 [details] Patch for landing
Sergio Villar Senin
Comment 52 2022-01-31 12:12:44 PST
Created attachment 450429 [details] Patch for landing
Ryosuke Niwa
Comment 53 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?
Sergio Villar Senin
Comment 54 2022-02-01 07:27:24 PST
Sergio Villar Senin
Comment 55 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?
Sergio Villar Senin
Comment 56 2022-02-01 07:41:16 PST
Wenson Hsieh
Comment 57 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 { ```
Sergio Villar Senin
Comment 58 2022-03-09 04:13:43 PST
Sergio Villar Senin
Comment 59 2022-03-09 07:11:20 PST
Sergio Villar Senin
Comment 60 2022-03-22 09:19:17 PDT
Created attachment 455379 [details] Patch for landing
Sergio Villar Senin
Comment 61 2022-03-24 01:33:52 PDT
Jon Lee
Comment 62 2022-03-24 10:21:19 PDT
Dawn Morningstar
Comment 63 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.
Dawn Morningstar
Comment 64 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>
Gabriel Nava Marino
Comment 65 2022-04-29 13:42:32 PDT
*** Bug 239293 has been marked as a duplicate of this bug. ***
Michael Saboff
Comment 66 2022-05-09 08:55:32 PDT
Gabriel will look at the API test crashes caused by the now rolled out patch.
Ryosuke Niwa
Comment 67 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.
Gabriel Nava Marino
Comment 68 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.
Rob Buis
Comment 69 2022-05-11 14:31:14 PDT
*** Bug 240255 has been marked as a duplicate of this bug. ***
Gabriel Nava Marino
Comment 70 2022-05-11 16:35:09 PDT
Gabriel Nava Marino
Comment 71 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).
Ryosuke Niwa
Comment 72 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>
Gabriel Nava Marino
Comment 73 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.
Gabriel Nava Marino
Comment 74 2022-05-12 10:00:19 PDT
Ryosuke Niwa
Comment 75 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.
Ryosuke Niwa
Comment 76 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
Ryosuke Niwa
Comment 77 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
Gabriel Nava Marino
Comment 78 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.
Darin Adler
Comment 79 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.
Ryosuke Niwa
Comment 80 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
Gabriel Nava Marino
Comment 81 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.
Gabriel Nava Marino
Comment 82 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.
Gabriel Nava Marino
Comment 83 2022-05-16 14:21:17 PDT
Brandon
Comment 84 2022-05-18 13:00:37 PDT
EWS
Comment 85 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.
Karl Rackler
Comment 86 2022-05-20 08:39:31 PDT
Reverting r294523 (250778@main) due to causing iOS Debug 68 API failures and 50 Layout tests crashes.
Karl Rackler
Comment 87 2022-05-20 08:51:23 PDT
Reopen to investigate issue.
Brandon
Comment 88 2022-05-20 11:07:16 PDT
Brandon
Comment 89 2022-05-20 11:15:57 PDT
EWS
Comment 90 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.
Rob Buis
Comment 91 2022-06-07 04:56:00 PDT
*** Bug 241154 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.