Bug 189983 – [macOS] Implement a way for the UI process to request typing attributes at the current selection

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/44648705>

[Wenson Hsieh]

This is for NSInspectorBar support on macOS.

[Wenson Hsieh]

Created attachment 350836 [details]
Patch

[Wenson Hsieh]

Created attachment 350837 [details]
Rebase on trunk

How often is the client going to call into this API? How is it going to know when to call (that is, how is it going to know that the typing attributes at the current selection have changed)? Would it be possible to just deliver the typing attributes along with whatever signal says that they need to be queried again?

[Wenson Hsieh]

(In reply to mitz from comment #4)
> How often is the client going to call into this API? How is it going to know
> when to call (that is, how is it going to know that the typing attributes at
> the current selection have changed)?

- The client (NSInspectorBar, in this case) is going to call into this API when the user changes a value using controls in the inspector bar. It will also call into this API when we tell it to update (i.e. upon selection change) by calling -[NSInspectorBar _update].

> Would it be possible to just deliver the typing attributes along with whatever signal 
> says that they need to be queried again?

I think it's possible to (efficiently) achieve this, but it would require a more complex implementation. We'd certainly want to avoid computing a full typing style and attributed string all the time on every selection change, so we'd need to KVO for the presence of the inspector bar (as well as inspector bar visibility) in the UI process and have the web process know that the inspector bar is visible. Only when the inspector bar is visible would the web process send (1) a selection range, (2) a typing attributes dictionary, and (3) an attributed string if the selection is a range.

[Wenson Hsieh]

Created attachment 350839 [details]
Fix macOS build

(In reply to Wenson Hsieh from comment #5)
> (In reply to mitz from comment #4)
> > How often is the client going to call into this API? How is it going to know
> > when to call (that is, how is it going to know that the typing attributes at
> > the current selection have changed)?
> 
> - The client (NSInspectorBar, in this case) is going to call into this API
> when the user changes a value using controls in the inspector bar. It will
> also call into this API when we tell it to update (i.e. upon selection
> change) by calling -[NSInspectorBar _update].

What happens when the selection doesn’t change but the attributes do change (for example because the use changed a value without using the inspector bar, or perhaps even not using the API, e.g. by something that invokes JavaScript on the page that makes the change that way)?

> 
> > Would it be possible to just deliver the typing attributes along with whatever signal 
> > says that they need to be queried again?
> 
> I think it's possible to (efficiently) achieve this, but it would require a
> more complex implementation. We'd certainly want to avoid computing a full
> typing style and attributed string all the time on every selection change,
> so we'd need to KVO for the presence of the inspector bar (as well as
> inspector bar visibility) in the UI process and have the web process know
> that the inspector bar is visible. Only when the inspector bar is visible
> would the web process send (1) a selection range, (2) a typing attributes
> dictionary, and (3) an attributed string if the selection is a range.

I know that this API is for the inspector bar, but how do we (plan to) make the Fonts panel track the typing style?

[Wenson Hsieh]

(In reply to mitz from comment #7)
> (In reply to Wenson Hsieh from comment #5)
> > (In reply to mitz from comment #4)
> > > How often is the client going to call into this API? How is it going to know
> > > when to call (that is, how is it going to know that the typing attributes at
> > > the current selection have changed)?
> > 
> > - The client (NSInspectorBar, in this case) is going to call into this API
> > when the user changes a value using controls in the inspector bar. It will
> > also call into this API when we tell it to update (i.e. upon selection
> > change) by calling -[NSInspectorBar _update].
> 
> What happens when the selection doesn’t change but the attributes do change
> (for example because the use changed a value without using the inspector
> bar, or perhaps even not using the API, e.g. by something that invokes
> JavaScript on the page that makes the change that way)?

That's a really good point — unfortunately, I don't have a great plan to deal with this. I don't think we handle this either in the various scenarios currently where we have state in the UI process that's derived from the web process (e.g., some information in EditorState and AssistedNodeInformation come to mind). I'll think about this some more...

> 
> > 
> > > Would it be possible to just deliver the typing attributes along with whatever signal 
> > > says that they need to be queried again?
> > 
> > I think it's possible to (efficiently) achieve this, but it would require a
> > more complex implementation. We'd certainly want to avoid computing a full
> > typing style and attributed string all the time on every selection change,
> > so we'd need to KVO for the presence of the inspector bar (as well as
> > inspector bar visibility) in the UI process and have the web process know
> > that the inspector bar is visible. Only when the inspector bar is visible
> > would the web process send (1) a selection range, (2) a typing attributes
> > dictionary, and (3) an attributed string if the selection is a range.
> 
> I know that this API is for the inspector bar, but how do we (plan to) make
> the Fonts panel track the typing style?

TBH, not 100% sure. The (naive) approach I was considering is to request typing attributes using the same machinery as we would for the inspector bar. Then, to avoid doing excessive work computing typing attributes multiple times on every selection change, I'd implement some kind of batching mechanism so that when a typing attribute request is made while another typing attribute request is already in-flight, we'll just wait for the response to the former typing attribute request and invoke both callbacks then.

That being said, this isn't a viable strategy if we want to preserve the ability for clients to do things like: (1) request typing attributes, (2) immediately perform some editing operation, (3) request typing attributes again, and expect the typing attributes fetched in (3) to reflect the editing done in (2).

Alternately, we can cache computed FontAttributes in the web process instead (invalidated on selection change), so the only added cost of multiple calls to request typing attributes is a bit of IPC encoding/decoding.

(In reply to Wenson Hsieh from comment #8)
> (In reply to mitz from comment #7)
> > (In reply to Wenson Hsieh from comment #5)
> > > (In reply to mitz from comment #4)
> > > > How often is the client going to call into this API? How is it going to know
> > > > when to call (that is, how is it going to know that the typing attributes at
> > > > the current selection have changed)?
> > > 
> > > - The client (NSInspectorBar, in this case) is going to call into this API
> > > when the user changes a value using controls in the inspector bar. It will
> > > also call into this API when we tell it to update (i.e. upon selection
> > > change) by calling -[NSInspectorBar _update].
> > 
> > What happens when the selection doesn’t change but the attributes do change
> > (for example because the use changed a value without using the inspector
> > bar, or perhaps even not using the API, e.g. by something that invokes
> > JavaScript on the page that makes the change that way)?
> 
> That's a really good point — unfortunately, I don't have a great plan to
> deal with this. I don't think we handle this either in the various scenarios
> currently where we have state in the UI process that's derived from the web
> process (e.g., some information in EditorState and AssistedNodeInformation
> come to mind). I'll think about this some more...

How is this handled for the Touch Bar?

This may be naïve, but if we handle these things correctly in a legacy WebView, it means that we are willing to do the work required to computed those things whenever they change and there’s an interested observer, so for WKWebView, we should be able to create proxies for those UI-process observers in the Web Content process and push the information of interest to them.

> 
> > 
> > > 
> > > > Would it be possible to just deliver the typing attributes along with whatever signal 
> > > > says that they need to be queried again?
> > > 
> > > I think it's possible to (efficiently) achieve this, but it would require a
> > > more complex implementation. We'd certainly want to avoid computing a full
> > > typing style and attributed string all the time on every selection change,
> > > so we'd need to KVO for the presence of the inspector bar (as well as
> > > inspector bar visibility) in the UI process and have the web process know
> > > that the inspector bar is visible. Only when the inspector bar is visible
> > > would the web process send (1) a selection range, (2) a typing attributes
> > > dictionary, and (3) an attributed string if the selection is a range.
> > 
> > I know that this API is for the inspector bar, but how do we (plan to) make
> > the Fonts panel track the typing style?
> 
> TBH, not 100% sure. The (naive) approach I was considering is to request
> typing attributes using the same machinery as we would for the inspector
> bar. Then, to avoid doing excessive work computing typing attributes
> multiple times on every selection change, I'd implement some kind of
> batching mechanism so that when a typing attribute request is made while
> another typing attribute request is already in-flight, we'll just wait for
> the response to the former typing attribute request and invoke both
> callbacks then.
> 
> That being said, this isn't a viable strategy if we want to preserve the
> ability for clients to do things like: (1) request typing attributes, (2)
> immediately perform some editing operation, (3) request typing attributes
> again, and expect the typing attributes fetched in (3) to reflect the
> editing done in (2).
> 
> Alternately, we can cache computed FontAttributes in the web process instead
> (invalidated on selection change), so the only added cost of multiple calls
> to request typing attributes is a bit of IPC encoding/decoding.

[Wenson Hsieh]

> > > What happens when the selection doesn’t change but the attributes do change
> > > (for example because the use changed a value without using the inspector
> > > bar, or perhaps even not using the API, e.g. by something that invokes
> > > JavaScript on the page that makes the change that way)?
> …
> How is this handled for the Touch Bar?

Currently, it is not (both in legacy and modern WebKit). I determined this by adding and removing styles via web inspector and checking to see whether the inspector bar/Touch Bar/font panel updated to reflect the new state, but it looks like all three don't update until the next selection change.

> This may be naïve, but if we handle these things correctly in a legacy
> WebView, it means that we are willing to do the work required to computed
> those things whenever they change and there’s an interested observer, so for
> WKWebView, we should be able to create proxies for those UI-process
> observers in the Web Content process and push the information of interest to
> them.

[Wenson Hsieh]

> This may be naïve, but if we handle these things correctly in a legacy
> WebView, it means that we are willing to do the work required to computed
> those things whenever they change and there’s an interested observer, so for
> WKWebView, we should be able to create proxies for those UI-process
> observers in the Web Content process and push the information of interest to
> them.

Currently, we handle updating the inspector bar alongside the font panel in legacy WebKit correctly, albeit inefficiently since we compute typing attributes twice when the font panel is visible. The first call is from WebKit updating the font panel when the selection changes...

1   0x11315995f WebCore::Editor::fontAttributesAtSelectionStart() const
2   0x1104d3769 -[WebHTMLView(WebInternal) _updateFontPanel]
3   0x102508bdc
4   0x110460af8 updateFontPanel(WebView*)
5   0x110460f30 WebEditorClient::respondToChangedContents()
6   0x1131466e4 WebCore::Editor::respondToChangedContents(WebCore::VisibleSelection const&)
7   0x113149b3d WebCore::Editor::appliedEditing(WebCore::CompositeEditCommand&)

...and this second call is from AppKit asking WebKit for typing attribute information when calling -updateSelectedAttributes to update the inspector bar.

1   0x11315995f WebCore::Editor::fontAttributesAtSelectionStart() const
2   0x1105b52bf -[WebView(WebViewEditingActions) typingAttributes]
3   0x103dc47fa -[__NSInspectorBarItemController _updateSelectedAttributesForSyncClient]
4   0x103dc3d30 -[__NSInspectorBarItemController updateSelectedAttributes]
5   0x103db7274 -[NSInspectorBar _update]
6   0x102508c20
7   0x110460af8 updateFontPanel(WebView*)
8   0x110460f30 WebEditorClient::respondToChangedContents()
9   0x1131466e4 WebCore::Editor::respondToChangedContents(WebCore::VisibleSelection const&)
10  0x113149b3d WebCore::Editor::appliedEditing(WebCore::CompositeEditCommand&)

[Wenson Hsieh]

Dan and I chatted in person about this. (I think) we agree that computing and sending all font attribute information on selection change is too costly, and it's not straightforward how to limit this work to only when the inspector bar is present (and also visible, and the web view is also the first responder). We had briefly considered SPI for Mail to opt into computing and sending typing attributes whenever the selection changes, but a more straightforward solution is to simply cache typing attributes so that subsequent typing attribute requests are cheap, if the selection hasn't changed.

[Wenson Hsieh]

Created attachment 350889 [details]
Patch

[Wenson Hsieh]

Created attachment 351336 [details]
Rebase on trunk

[EWS Watchlist]

Comment on attachment 351336 [details]
Rebase on trunk

Attachment 351336 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9420746

New failing tests:
css3/filters/backdrop/add-remove-add-backdrop-filter.html

[EWS Watchlist]

Created attachment 351338 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Wenson Hsieh]

Comment on attachment 351336 [details]
Rebase on trunk

Thanks for the review!

The macOS failure (css3/filters/backdrop/add-remove-add-backdrop-filter.html) does not seem related.

[WebKit Commit Bot]

Comment on attachment 351336 [details]
Rebase on trunk

Rejecting attachment 351336 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 351336, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=351336&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=189983&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 351336 from bug 189983.
Fetching: https://bugs.webkit.org/attachment.cgi?id=351336
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	Tools/TestWebKitAPI/Tests/TestWebKitAPI/mac/AppKitSPI.h => Source/WebKit/Platform/spi/mac/AppKitSPI.h
	C	Tools/TestWebKitAPI/Tests/TestWebKitAPI/mac/AppKitSPI.h => Tools/TestWebKitAPI/mac/TestInspectorBar.h
	A	Tools/TestWebKitAPI/mac/TestInspectorBar.mm
	M	Source/WebKit/ChangeLog
	M	Source/WebKit/Shared/WebCoreArgumentCoders.cpp
	M	Source/WebKit/Shared/WebCoreArgumentCoders.h
	M	Source/WebKit/Shared/mac/ArgumentCodersMac.h
	M	Source/WebKit/Shared/mac/ArgumentCodersMac.mm
	M	Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
	M	Source/WebKit/UIProcess/Cocoa/WebViewImpl.h
	M	Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm
	M	Source/WebKit/UIProcess/WebPageProxy.cpp
	M	Source/WebKit/UIProcess/WebPageProxy.h
	M	Source/WebKit/UIProcess/WebPageProxy.messages.in
	M	Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm
	M	Source/WebKit/UIProcess/mac/WebPageProxyMac.mm
	M	Source/WebKit/WebKit.xcodeproj/project.pbxproj
	M	Source/WebKit/WebProcess/WebPage/WebPage.cpp
	M	Source/WebKit/WebProcess/WebPage/WebPage.h
	M	Source/WebKit/WebProcess/WebPage/WebPage.messages.in
	M	Tools/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Tools/ChangeLog' is out of date
W: d22035f10124afcda117e9691900939b7d04f94b and refs/remotes/origin/master differ, using rebase:
:040000 040000 e5c7f27896302a6307b0f76ea077f9defb08d31a 118259e4a045b5b61292f0dd43f3f259bfe06cad M	Source
:040000 040000 e5c58c40d0a83d716ce8b28c0e16d59dc1d9eb89 a40c4f91c303cb6acc517a7e7375a15c93c0f2ac M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	Tools/TestWebKitAPI/Tests/TestWebKitAPI/mac/AppKitSPI.h => Source/WebKit/Platform/spi/mac/AppKitSPI.h
	C	Tools/TestWebKitAPI/Tests/TestWebKitAPI/mac/AppKitSPI.h => Tools/TestWebKitAPI/mac/TestInspectorBar.h
	A	Tools/TestWebKitAPI/mac/TestInspectorBar.mm
	M	Source/WebKit/ChangeLog
	M	Source/WebKit/Shared/WebCoreArgumentCoders.cpp
	M	Source/WebKit/Shared/WebCoreArgumentCoders.h
	M	Source/WebKit/Shared/mac/ArgumentCodersMac.h
	M	Source/WebKit/Shared/mac/ArgumentCodersMac.mm
	M	Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
	M	Source/WebKit/UIProcess/Cocoa/WebViewImpl.h
	M	Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm
	M	Source/WebKit/UIProcess/WebPageProxy.cpp
	M	Source/WebKit/UIProcess/WebPageProxy.h
	M	Source/WebKit/UIProcess/WebPageProxy.messages.in
	M	Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm
	M	Source/WebKit/UIProcess/mac/WebPageProxyMac.mm
	M	Source/WebKit/WebKit.xcodeproj/project.pbxproj
	M	Source/WebKit/WebProcess/WebPage/WebPage.cpp
	M	Source/WebKit/WebProcess/WebPage/WebPage.h
	M	Source/WebKit/WebProcess/WebPage/WebPage.messages.in
	M	Tools/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Tools/ChangeLog' is out of date
W: d22035f10124afcda117e9691900939b7d04f94b and refs/remotes/origin/master differ, using rebase:
:040000 040000 e5c7f27896302a6307b0f76ea077f9defb08d31a 118259e4a045b5b61292f0dd43f3f259bfe06cad M	Source
:040000 040000 e5c58c40d0a83d716ce8b28c0e16d59dc1d9eb89 a40c4f91c303cb6acc517a7e7375a15c93c0f2ac M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From https://git.webkit.org/git/WebKit
   3806d14af8d..00eb7df9a03  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 236717 = 3806d14af8d1323fd4234c8690a373333a0f23ee
r236718 = 385fdfffc4a162ae799834b66ca5fd35e9edb8c3
r236719 = 34c762bb8a8c6a44b74d5f7aa94a81985e4a4839
r236720 = 0c9cd2d1fd5c44e0f7363ee0cdbbb51e09553811
r236721 = 00eb7df9a03bb56c911c7ede2483a9a914cdc3a0
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: https://webkit-queues.webkit.org/results/9423873

[Wenson Hsieh]

Created attachment 351363 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 351363 [details]
Patch for landing

Clearing flags on attachment: 351363

Committed r236724: <https://trac.webkit.org/changeset/236724>