Summary: Cookies table needs copy keyboard shortcut and context menu support. DataGrid supports `handleCopyEvent` and provides "Copy Row" & "Copy Table" context menu items as needed. Table doesn't need to provide this level of support, but at the very least clients need to query column visibility, so the clipboard text can reflect what is shown in the table. CookieStorageContentView can reuse the formatting from tablePopulateCell to generate clipboard text.
<rdar://problem/45953002>
Created attachment 354865 [details] Patch
Created attachment 355127 [details] Patch
Comment on attachment 355127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355127&action=review r-, due to the `emDash` and `zeroWidthSpace` concerns. The logic looks fine. > Source/WebInspectorUI/ChangeLog:20 > + (WI.CookieStorageContentView): NIT: this should go first in the ChangeLog > Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:352 > + return values.join("\t"); Is the reason you aren't using a CSV format because some of the `WI.Cookie` values might have commas? In that case, perhaps we should expose this as a cookie string instead (e.g. `${key}=${value}` joined by ";")? > Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:368 > + return cookie.domain || emDash; We shouldn't expose `emDash` as clipboard content. That's really just a formatting tool for WebInspector. See (352) for an alternative > Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:370 > + return cookie.path || emDash; Ditto (368) > Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:376 > + return cookie.secure ? checkmark : zeroWidthSpace; We shouldn't expose `zeroWidthSpace` as clipboard content. That's really just a formatting tool for WebInspector. See (352) for an alternative > Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:378 > + return cookie.httpOnly ? checkmark : zeroWidthSpace; Ditto (376) > Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:380 > + return cookie.sameSite === WI.Cookie.SameSiteType.None ? emDash : WI.Cookie.displayNameForSameSiteType(cookie.sameSite); Ditto (368) > Source/WebInspectorUI/UserInterface/Views/Table.js:244 > + return this._columnSpecs.values(); This should probably just call `Array.from` by default. I don't think we ever expose an iterator unless it's via a `Symbol.iterator` function.
Comment on attachment 355127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355127&action=review >> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:352 >> + return values.join("\t"); > > Is the reason you aren't using a CSV format because some of the `WI.Cookie` values might have commas? In that case, perhaps we should expose this as a cookie string instead (e.g. `${key}=${value}` joined by ";")? This matches the original DataGrid behavior. I haven't found many native apps with tables to use for comparison, but the Instruments app (which doesn't allow multiple selection) does use \t to delimit fields. We could always add other copy options if there is a need. >> Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:368 >> + return cookie.domain || emDash; > > We shouldn't expose `emDash` as clipboard content. That's really just a formatting tool for WebInspector. See (352) for an alternative I'll make the content depend on a `formatForClipboard` flag (or something like that).
Created attachment 355612 [details] Patch
Comment on attachment 355612 [details] Patch r=me
Comment on attachment 355612 [details] Patch Clearing flags on attachment: 355612 Committed r238569: <https://trac.webkit.org/changeset/238569>
All reviewed patches have been landed. Closing bug.