Bug 229001 – Web Inspector: Do not show contextual documentation popup in the Changes panel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Razvan Caliman]

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.

[Radar WebKit Bug Importer]

<rdar://problem/81792379>

[Razvan Caliman]

Created attachment 435345 [details]
Patch

[Devin Rousso]

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 🤔

[Razvan Caliman]

Created attachment 435607 [details]
Patch v2

[Patrick Angle]

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.)

[Razvan Caliman]

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.

[Razvan Caliman]

Created attachment 435615 [details]
Patch v2.1

Add option to prevent showing contextual documentation button for CSS declarations

[Devin Rousso]

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`?

[Razvan Caliman]

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.

[Razvan Caliman]

Created attachment 435685 [details]
[fast-cq] Patch v2.2

Address code review comments

[EWS]

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].