RESOLVED FIXED 204330
Web Inspector: Local Overrides: the placeholder for the MIME type, status code, and status text is the same as the placeholder URL
https://bugs.webkit.org/show_bug.cgi?id=204330
Summary Web Inspector: Local Overrides: the placeholder for the MIME type, status cod...
Devin Rousso
Reported 2019-11-18 16:34:07 PST
# STEPS TO REPRODUCE: 1. inspect any page 2. click the + in the bottom of the navigation sidebar of the Sources Tab 3. select "Local Override..." 4. delete the contents of the MIME type, status code, or status text inputs => URL is shown, instead of the default value for those inputs
Attachments
Patch (23.95 KB, patch)
2019-11-18 16:46 PST, Devin Rousso
no flags
[Image] After Patch is applied (72.27 KB, image/png)
2019-11-18 16:46 PST, Devin Rousso
no flags
Patch (27.08 KB, patch)
2019-11-18 19:23 PST, Devin Rousso
no flags
Devin Rousso
Comment 1 2019-11-18 16:46:05 PST
Devin Rousso
Comment 2 2019-11-18 16:46:42 PST
Created attachment 383808 [details] [Image] After Patch is applied
Joseph Pecoraro
Comment 3 2019-11-18 17:27:02 PST
Comment on attachment 383807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383807&action=review Some questions. > Source/WebInspectorUI/ChangeLog:14 > + (.popover .local-resource-override-popover-content .data-grid tr.header-content-type > :matches(.name-column, .value-column)): Added. > + Replace the hardcoded `placeholder` with an optional `options` object that can include a The user experience of only placeholders seems weird. I'd prefer it include values where it has values. > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:52 > - let url = this._urlCodeMirror.getValue(); > + let url = this._urlCodeMirror.getValue() || this._urlCodeMirror.getOption("placeholder"); > if (!url) > return null; Not allowing a path and having it fallback to the placeholder seems bad. In the other cases it seems fine because those are reasonable default values, but this one ending in "/path" seems dumb. I think we should continue to disallow an empty url. > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:119 > if (!mimeType) > - mimeType = "text/javascript"; > + mimeType = "text/plain"; I'd rather we prefer the default be "text/javascript" since text files are very rare on web pages. > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:336 > - value, > + ...options, Why did we get rid of the initial value? value: value Unless I'm missing something, this doesn't seem preferable to setting this value because you can't edit / copy the placeholder. If you want, maybe the URL field could start with an empty value but have a placeholder to force the user to type in a URL, but that seems weird because it requires typing a full URL path when we could have pre-generated part of it.
Devin Rousso
Comment 4 2019-11-18 17:32:03 PST
Comment on attachment 383807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383807&action=review >> Source/WebInspectorUI/ChangeLog:14 >> + Replace the hardcoded `placeholder` with an optional `options` object that can include a > > The user experience of only placeholders seems weird. I'd prefer it include values where it has values. I think we should do this if we're given a resource to copy/override, but not if we're creating an "empty" local override. >> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:52 >> return null; > > Not allowing a path and having it fallback to the placeholder seems bad. In the other cases it seems fine because those are reasonable default values, but this one ending in "/path" seems dumb. > > I think we should continue to disallow an empty url. That makes sense. Agreed. >> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:119 >> + mimeType = "text/plain"; > > I'd rather we prefer the default be "text/javascript" since text files are very rare on web pages. I feel like that's a bit "biased" towards users mostly using this to override JavaScript vs other types (e.g. CSS, HTML, etc.), but I don't particularly care that much. >> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:336 >> + ...options, > > Why did we get rid of the initial value? > > value: value > > Unless I'm missing something, this doesn't seem preferable to setting this value because you can't edit / copy the placeholder. > > If you want, maybe the URL field could start with an empty value but have a placeholder to force the user to type in a URL, but that seems weird because it requires typing a full URL path when we could have pre-generated part of it. We didn't so much "get rid of it" as we allow the caller to decide whether they want to supply it or not. As I wrote above, I think it makes a lot of sense to set a value when we know there is a value (e.g. overriding an existing resource), but if we're creating a brand new local override I think having them be placeholders is more "inviting" in that it encourages the user to fill in their own value.
Devin Rousso
Comment 5 2019-11-18 19:23:28 PST
Joseph Pecoraro
Comment 6 2019-11-19 12:01:21 PST
Comment on attachment 383833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383833&action=review rs=me > Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:37 > + this._selectable = selectable !== undefined ? selectable : true; > + this._copyable = copyable !== undefined ? copyable : true; > + this._editable = editable !== undefined ? editable : true; Can we use nullish operators now? selectable ?? true
Devin Rousso
Comment 7 2019-11-19 14:09:58 PST
Comment on attachment 383833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383833&action=review >> Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:37 >> + this._editable = editable !== undefined ? editable : true; > > Can we use nullish operators now? > > selectable ?? true I'd rather wait for nullish coalescing to reach Stage 4 before starting to use it. This would be a perfect use case though :)
WebKit Commit Bot
Comment 8 2019-11-19 14:55:11 PST
Comment on attachment 383833 [details] Patch Clearing flags on attachment: 383833 Committed r252652: <https://trac.webkit.org/changeset/252652>
WebKit Commit Bot
Comment 9 2019-11-19 14:55:13 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 10 2019-11-19 14:56:17 PST
Note You need to log in before you can comment on or make changes to this bug.