.
<rdar://problem/83078713>
Created attachment 438094 [details] Patch
Created attachment 438095 [details] Just for EWS
Created attachment 438144 [details] For EWS
Comment on attachment 438144 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=438144&action=review r=me, neat stuff :) > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1234 > - result.isContentEditable = selection.isContentEditable(); > + result.isContentEditable = selection.hasEditableStyle(); These appear to have different `UserSelectAllTreatment`. Is that a problem? > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1327 > + auto isEditable = frame->selection().selection().hasEditableStyle() ? EditorStateIsContentEditable::Yes : EditorStateIsContentEditable::No; ditto (:1231) > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3561 > m_hasEverFocusedElementDueToUserInteractionSincePageTransition = false; NIT: Shouldn't this really be inside a `HAVE(TOUCH_BAR)`? > Source/WebKit/WebProcess/WebPage/WebPage.cpp:5979 > - m_hasEverFocusedElementDueToUserInteractionSincePageTransition |= m_userIsInteracting; > + if (m_userIsInteracting && m_focusedElement) > + m_hasEverFocusedElementDueToUserInteractionSincePageTransition = true; This seems like a actual functional difference. Is it something we can/should test? > Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:1089 > + if (m_needsFontAttributes) NIT: it'd be nice if we could add comments explaining why these things require computing post layout data
Comment on attachment 438144 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=438144&action=review >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1234 >> + result.isContentEditable = selection.hasEditableStyle(); > > These appear to have different `UserSelectAllTreatment`. Is that a problem? I don't think this will be an issue since we already use this option when grabbing the `rootEditableElement()`, computing editing style, and other adjacent codepaths. (`isContentRichlyEditable()` also uses this option, so it would be consistent with that) >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3561 >> m_hasEverFocusedElementDueToUserInteractionSincePageTransition = false; > > NIT: Shouldn't this really be inside a `HAVE(TOUCH_BAR)`? Moved this into `HAVE(TOUCH_BAR)` instead. >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5979 >> + m_hasEverFocusedElementDueToUserInteractionSincePageTransition = true; > > This seems like a actual functional difference. Is it something we can/should test? It's not actually clear to me what the functional difference would be, since this should only prevent us from setting `m_hasEverFocusedElementDueToUserInteractionSincePageTransition` to true in cases where we wouldn't have tried to show the text editing controls on the Touch Bar anyways. >> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:1089 >> + if (m_needsFontAttributes) > > NIT: it'd be nice if we could add comments explaining why these things require computing post layout data This is because font attributes are plumbed through post-layout data. I'll add comments clarifying this.
Created attachment 438162 [details] For EWS
Committed r282419 (241678@main): <https://commits.webkit.org/241678@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 438162 [details].