WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
[Image] After Patch is applied
(72.27 KB, image/png)
2019-11-18 16:46 PST
,
Devin Rousso
no flags
Details
Patch
(27.08 KB, patch)
2019-11-18 19:23 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Devin Rousso
Comment 1
2019-11-18 16:46:05 PST
Created
attachment 383807
[details]
Patch
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
Created
attachment 383833
[details]
Patch
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
<
rdar://problem/57338064
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug