Bug 150298

Summary:

Web Inspector: Pressing Command-S while focused on the styles sidebar should save CSS file

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Nikita Vasilyev <nvasilyev>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

See Also:

https://bugs.webkit.org/show_bug.cgi?id=150357

Attachments:

Description	Flags
[Animated GIF] Current behavior	none
Patch	none
Patch	timothy: review+
Patch	nvasilyev: commit-queue-
Patch	none

Nikita Vasilyev

Reported 2015-10-18 04:11:17 PDT

Created attachment 263415 [details] [Animated GIF] Current behavior index.html: <link rel="stylesheet" href="style.css"> <body>blah</body> style.css: body {font: 24px sans-serif} Steps: 1. Inspect index.html 2. Change the font-size to 32px 3. Press Command-S Expected: A dialog to save style.css is shown. The same dialog as if I were to navigate to style.css resource and press Command-S. Actual: A dialog to save index.html.webarchive is shown. Notes: https://twitter.com/simurai has pointed out that there is no way to save CSS from the styles sidebar, which is a major inconvenience while prototyping. In addition to that, it would be useful to indicate modified CSS resources in the styles sidebar by appending "*", e.d. "styles.css*" when it has unsaved changes.

Attachments
[Animated GIF] Current behavior (155.19 KB, image/gif) 2015-10-18 04:11 PDT, Nikita Vasilyev	no flags	Details
Patch (4.10 KB, patch) 2015-10-20 03:18 PDT, Nikita Vasilyev	no flags	Details Formatted Diff Diff
Patch (3.14 KB, patch) 2015-10-20 03:20 PDT, Nikita Vasilyev	timothy: review+	Details Formatted Diff Diff
Patch (3.14 KB, patch) 2015-10-20 20:26 PDT, Nikita Vasilyev	nvasilyev: commit-queue-	Details Formatted Diff Diff
Patch (3.14 KB, patch) 2015-10-20 20:31 PDT, Nikita Vasilyev	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2015-10-18 04:11:40 PDT

<rdar://problem/23158475>

Devin Rousso

Comment 2 2015-10-18 17:52:15 PDT

I think that this could be pretty risky. Image if the user is focusing a rule for "style1.css" when their cursor is over a rule for "style2.css". If the user hits Cmd+s, which file should it save? I would personally say "style1.css" since it has focus, but it could go either way. I do, however, like the idea of adding a "*" to modified CSS files in the Resources view. I'll work on that this week.

Nikita Vasilyev

Comment 3 2015-10-18 18:08:20 PDT

(In reply to comment #2) > I think that this could be pretty risky. Image if the user is focusing a > rule for "style1.css" when their cursor is over a rule for "style2.css". If > the user hits Cmd+s, which file should it save? I would personally say > "style1.css" since it has focus, but it could go either way. When you say "when their cursor is over a rule for "style2.css"", do you mean the mouse cursor? It should save the file with the focused rule, e.g. "style1.css". By focused, I mean it has a text caret inside of it. > like the idea of adding a "*" to modified CSS files in the Resources view. > I'll work on that this week. 👍

Devin Rousso

Comment 4 2015-10-18 18:10:51 PDT

(In reply to comment #3) > (In reply to comment #2) > When you say "when their cursor is over a rule for "style2.css"", do you > mean the mouse cursor? Yes, I mean the mouse cursor. > It should save the file with the focused rule, e.g. "style1.css". By > focused, I mean it has a text caret inside of it. Right, I would agree, but I am wary that others might not, thats all. > 👍 I did not know that we could put emoticons in comments...😛

Nikita Vasilyev

Comment 5 2015-10-20 03:18:05 PDT

Created attachment 263563 [details] Patch I went ahead and implemented this. It should't break anyone's workflow. I can't imagine somebody making CSS changes and immediately wanting to save a webarchive. Which, by the way, wouldn't include these CSS changes anyway. Here is a test page: http://nv.github.io/webkit-inspector-bugs/external-and-inline-stylesheets/ You'd need to store it locally to be able to save CSS changes, obviously. (In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > When you say "when their cursor is over a rule for "style2.css"", do you > > mean the mouse cursor? > > Yes, I mean the mouse cursor. > > > It should save the file with the focused rule, e.g. "style1.css". By > > focused, I mean it has a text caret inside of it. > > Right, I would agree, but I am wary that others might not, thats all. When I have a few windows opened in any text editor and I press Command-S, it saves the currently focused file, not the one that have my mouse cursor over it. Same goes for split views in any code editor I've seen. No matter where my mouse cursor is, Command-S always saves the currently focused file.

Nikita Vasilyev

Comment 6 2015-10-20 03:20:02 PDT

Created attachment 263564 [details] Patch

Timothy Hatcher

Comment 7 2015-10-20 15:53:54 PDT

Comment on attachment 263564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263564&action=review > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:519 > + if (sourceCode.urlComponents.scheme === "data") { > + // Data URI, e.g. data:text/css;base64,... > + InspectorFrontendHost.beep(); > + return; > + } We should allow saving data URLs. But it sounds like the data is empty? Should file a bug on this. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:525 > + InspectorFrontendHost.save(sourceCode.url, sourceCode.content, false, saveAs); This should use WebInspector.saveDataToFile, which is a wrapper for InspectorFrontendHost.save.

Devin Rousso

Comment 8 2015-10-20 16:41:20 PDT

Comment on attachment 263564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263564&action=review > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:499 > + if (this._style.type !== WebInspector.CSSStyleDeclaration.Type.Rule) { I don't think that this can ever happen, but it may be a good idea to add "|| !this._style.ownerRule" here (to be safe). > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:501 > + InspectorFrontendHost.beep(); Instead of beeping, would it make more sense to just try to save normally (the webarchive)? I would think that if the user modifies a section of HTML (which is a <style> in this case) then they would want to be able to save the HTML from the sidebar just like for a regular rule. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:505 > + console.assert(this._style._ownerRule instanceof WebInspector.CSSRule); _ownerRule has a getter. This should be "this._style.ownerRule" (below too) > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:511 > + InspectorFrontendHost.beep(); See above (line 501) >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:525 >> + InspectorFrontendHost.save(sourceCode.url, sourceCode.content, false, saveAs); > > This should use WebInspector.saveDataToFile, which is a wrapper for InspectorFrontendHost.save. Using this would also let you remove the console.assert on 521-522 as they are checked inside WebInspector.saveDataToFile

Nikita Vasilyev

Comment 9 2015-10-20 20:18:17 PDT

Comment on attachment 263564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263564&action=review >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:499 >> + if (this._style.type !== WebInspector.CSSStyleDeclaration.Type.Rule) { > > I don't think that this can ever happen, but it may be a good idea to add "|| !this._style.ownerRule" here (to be safe). There is an assert for this._style.ownerRule a few lines below. I don't think we should exit silently when there is somethind wrong. >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:501 >> + InspectorFrontendHost.beep(); > > Instead of beeping, would it make more sense to just try to save normally (the webarchive)? I would think that if the user modifies a section of HTML (which is a <style> in this case) then they would want to be able to save the HTML from the sidebar just like for a regular rule. I actually tried that initially and it was pretty annoying. I had to look at the filename extension every time to not accidentally overwrite a CSS file with HTML. I think Command-S in the styles sidebar should consistently save only CSS. Besides, saving HTML doesn't include changes made by Web Inspector (bug 150357). >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:519 >> + } > > We should allow saving data URLs. But it sounds like the data is empty? Should file a bug on this. It turned out InspectorFrontendHost.save didn't work with sourceCode.url. sourceCode.content was fine.

Nikita Vasilyev

Comment 10 2015-10-20 20:26:49 PDT

Created attachment 263653 [details] Patch

WebKit Commit Bot

Comment 11 2015-10-20 20:26:55 PDT

Comment on attachment 263653 [details] Patch Rejecting attachment 263653 [details] from review queue. nvasilyev@apple.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.

Nikita Vasilyev

Comment 12 2015-10-20 20:31:46 PDT

Created attachment 263654 [details] Patch

WebKit Commit Bot

Comment 13 2015-10-20 21:25:43 PDT

Comment on attachment 263654 [details] Patch Clearing flags on attachment: 263654 Committed r191371: <http://trac.webkit.org/changeset/191371>

WebKit Commit Bot

Comment 14 2015-10-20 21:25:48 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.