Bug 169896

Summary: Web Inspector: Editing local storage keys/values starts with truncated strings
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, inspector-bugzilla-changes, mattbaker
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
mattbaker: review-
[Animated GIF] With patch applied
none
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews117 for mac-elcapitan
none
Patch
bburg: review+
Patch none

Description Nikita Vasilyev 2017-03-20 16:15:14 PDT
rdar://problem/14927212

Summary:
When you anticipate a website that needs to use LocalStorage or SessionStorage above a certain amount of characters, the browser saves the content normally in the browser. By accessing the Developer mode and trying to edit the content, what happens is the last part of the saved data is replaced with a "…" character and all other info is lost.
You can no longer edit this data, since most of it is lost and the (web) application is probably broken now.

Steps to Reproduce:
1. Open Safari
2. Visit website that uses local storage and saves a large (over 50 characters or so value) [Better keep Safari in a small viewport]
3. Right click anywhere
4. Click "Inspect Element"
5. Click the "Resources" tab
6. "Click LocalStorage or SessionStorage"
7. Locate a key with a value ending in "…" (chopped to fit screen)
8. Double click the value to edit
9. Click anywhere to save it.

Expected Results:
Safari was supposed to present the full data when the user double clicks to edit the key.

Actual Results:
Safari presents only the beginning of the data, with a "…" instead of the tailing characters. LocalStorage is edited and the web app is probably broken now.

Notes:
When double clicking, restore value to the full key, and not simply allow editing with the "…" in place.
Comment 1 Nikita Vasilyev 2017-03-20 16:17:51 PDT
Created attachment 304963 [details]
Patch
Comment 2 Nikita Vasilyev 2017-03-20 16:21:15 PDT
Created attachment 304965 [details]
[Animated GIF] With patch applied
Comment 3 Matt Baker 2017-03-20 16:37:40 PDT
Nice, I noticed this a while back.
Comment 4 Matt Baker 2017-03-20 16:42:33 PDT
Comment on attachment 304963 [details]
Patch

r-, for some reason `text-overflow: ellipsis` isn't restored after cancel/commit.
Comment 5 Nikita Vasilyev 2017-03-20 17:54:26 PDT
(In reply to comment #4)
> Comment on attachment 304963 [details]
> Patch
> 
> r-, for some reason `text-overflow: ellipsis` isn't restored after
> cancel/commit.

After canceling

<td class="value-column">
    <div class="cell-content">foo</div>
</td>

gets replaced with

<td class="value-column">foo</td>

🤦‍♂️
Comment 6 Nikita Vasilyev 2017-03-21 18:56:07 PDT
Created attachment 305062 [details]
Patch

I've tried making <div class=cell-content> editable instead of <td>, but it appeared to be difficult to style to make it look the way <td class=editing> looks now.

I ended up re-creating <div class=cell-content> after every edit.
Comment 7 Nikita Vasilyev 2017-03-22 17:05:01 PDT
Created attachment 305138 [details]
Patch

As per an offline discussion with Brian, there should be only one code path for creating <div class=cell-content>.
Comment 8 Build Bot 2017-03-22 18:22:05 PDT
Comment on attachment 305138 [details]
Patch

Attachment 305138 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3390911

New failing tests:
media/video-main-content-autoplay.html
Comment 9 Build Bot 2017-03-22 18:22:06 PDT
Created attachment 305149 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 10 Nikita Vasilyev 2017-03-23 09:59:19 PDT
Created attachment 305203 [details]
Patch

Unrelated flaky test. Re-uploading the patch.
Comment 11 BJ Burg 2017-03-23 10:35:10 PDT
Comment on attachment 305203 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:13
> +        since it may get removed while editing.

You should mention that div.cell-content is removed because the <td> itself is contenteditable (using the fancy -webkit-user-modify:read-write-plaintext-only), not the inner div.cell-content.
Comment 12 Nikita Vasilyev 2017-03-23 11:05:58 PDT
Created attachment 305204 [details]
Patch
Comment 13 WebKit Commit Bot 2017-03-23 11:47:52 PDT
Comment on attachment 305204 [details]
Patch

Clearing flags on attachment: 305204

Committed r214308: <http://trac.webkit.org/changeset/214308>
Comment 14 WebKit Commit Bot 2017-03-23 11:47:57 PDT
All reviewed patches have been landed.  Closing bug.