Bug 229001

Summary: Web Inspector: Do not show contextual documentation popup in the Changes panel
Product: WebKit Reporter: Razvan Caliman <rcaliman>
Component: Web InspectorAssignee: Razvan Caliman <rcaliman>
Status: RESOLVED FIXED    
Severity: Normal CC: drousso, ews-watchlist, inspector-bugzilla-changes, pangle, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 226883    
Bug Blocks:    
Attachments:
Description Flags
Screenshot of issue
none
Patch
none
Patch v2
none
Patch v2.1
none
[fast-cq] Patch v2.2 none

Description Razvan Caliman 2021-08-11 07:02:32 PDT
Created attachment 435343 [details]
Screenshot of issue

The icon to toggle the documentation popup is visible next to every property listed in the Changes panel.
Comment 1 Radar WebKit Bug Importer 2021-08-11 07:03:19 PDT
<rdar://problem/81792379>
Comment 2 Razvan Caliman 2021-08-11 07:44:27 PDT
Created attachment 435345 [details]
Patch
Comment 3 Devin Rousso 2021-08-11 10:56:55 PDT
Comment on attachment 435345 [details]
Patch

I feel like it's a bit odd for it to even be shown in the Changes panel at all 🤔
Comment 4 Razvan Caliman 2021-08-16 09:01:15 PDT
Created attachment 435607 [details]
Patch v2
Comment 5 Patrick Angle 2021-08-16 09:10:03 PDT
Comment on attachment 435607 [details]
Patch v2

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:37
> +        this._readOnly = options.readOnly;
> +        this._showDocumentation = options.showDocumentation;

These probably still need to be `||`-ed and/or `&&`-ed with their default value, since if the caller only provided the `readOnly` option, or only provided the `showDocumentation` option, the other option would still be undefined. This is also why it is preferable for options in an `options` parameter to default to `false`. (e.g. instead of a `showDocumentation` option, a `hideDocumentation` option.)
Comment 6 Razvan Caliman 2021-08-16 10:21:43 PDT
Comment on attachment 435607 [details]
Patch v2

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:37
>> +        this._showDocumentation = options.showDocumentation;
> 
> These probably still need to be `||`-ed and/or `&&`-ed with their default value, since if the caller only provided the `readOnly` option, or only provided the `showDocumentation` option, the other option would still be undefined. This is also why it is preferable for options in an `options` parameter to default to `false`. (e.g. instead of a `showDocumentation` option, a `hideDocumentation` option.)

Oh wow! I had the mistaken expectation that JS merges the two objects, the default and provided one. Today I learned it does not. I'll revert to ` ||`

`hideDocumentation` is a bit weird, since we check this option _before_ generating the DOM for it. 
But I understand the requirement: not to have to update `WI.SpreadSheetStyleProperty` callers elsewhere.
Comment 7 Razvan Caliman 2021-08-16 10:31:07 PDT
Created attachment 435615 [details]
Patch v2.1

Add option to prevent showing contextual documentation button for CSS declarations
Comment 8 Devin Rousso 2021-08-16 11:54:28 PDT
Comment on attachment 435615 [details]
Patch v2.1

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

r=me

> Source/WebInspectorUI/ChangeLog:7
> +        Add config option to WI.SpreadsheetStyleProperty to prevent showing a contextual documentation button.

Alternatively, you maybe could just use CSS to always `display: none` the button.  I don't have a preference as to the approach, so I'll leave it up to you :)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:37
> +        this._hideDocumentation = options.hideDocumentation || false;

NIT: While there's nothing wrong with this, it does have the possibility of keeping alive an object if the caller leverages falsy-conversion (e.g. if someone did this for some reason `new WI.SpreadsheetStyleProperty(delegate, cssProperty, {readOnly: true, hideDocumentation: delegate})` to only hide the documentation if there's a delegate).  For boolean values I usually prefer to `!!options.hideDocumentation` instead as that ensures we don't keep anything alive.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:220
> +        if (!this._hideDocumentation)

NIT: Rather than having this check outside, can we put it inside `_addContextualDocumentationButton` so that we don't have to worry about some future engineer adding it without checking `_hideDocumentation`?
Comment 9 Razvan Caliman 2021-08-17 07:58:55 PDT
Comment on attachment 435615 [details]
Patch v2.1

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

>> Source/WebInspectorUI/ChangeLog:7
>> +        Add config option to WI.SpreadsheetStyleProperty to prevent showing a contextual documentation button.
> 
> Alternatively, you maybe could just use CSS to always `display: none` the button.  I don't have a preference as to the approach, so I'll leave it up to you :)

Ok. Let's not generate the elements at all and make that obvious.
Generating and appending to document, then hiding them with CSS can get confusing during debugging in the future.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:37
>> +        this._hideDocumentation = options.hideDocumentation || false;
> 
> NIT: While there's nothing wrong with this, it does have the possibility of keeping alive an object if the caller leverages falsy-conversion (e.g. if someone did this for some reason `new WI.SpreadsheetStyleProperty(delegate, cssProperty, {readOnly: true, hideDocumentation: delegate})` to only hide the documentation if there's a delegate).  For boolean values I usually prefer to `!!options.hideDocumentation` instead as that ensures we don't keep anything alive.

Good point.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:220
>> +        if (!this._hideDocumentation)
> 
> NIT: Rather than having this check outside, can we put it inside `_addContextualDocumentationButton` so that we don't have to worry about some future engineer adding it without checking `_hideDocumentation`?

Yes, that sounds reasonable.
Comment 10 Razvan Caliman 2021-08-17 08:01:15 PDT
Created attachment 435685 [details]
[fast-cq] Patch v2.2

Address code review comments
Comment 11 EWS 2021-08-17 09:02:06 PDT
Committed r281139 (240593@main): <https://commits.webkit.org/240593@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435685 [details].