Bug 166909 – Web Inspector: the 'lock' icon for non-editable rules in the Style Rules sidebar lacks a tooltip

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

[Blaze Burg]

Ooops. Probably needs AX attributes as well.

[Radar WebKit Bug Importer]

<rdar://problem/29959406>

[Nikita Vasilyev]

It is currently implemented as a pseudo-element. It should be re-implemented as a regular element so we can add a title attribute.

[Nikita Vasilyev]

What title attribute should we use?

"Style declaration cannot be modified"
"Style declaration is not editable"

[Blaze Burg]

(In reply to comment #3)
> What title attribute should we use?
> 
> "Style declaration cannot be modified"
> "Style declaration is not editable"

It should be a sentence. Maybe, "User Agent styles cannot be modified." The User Agent part should probably be a separate UIString, one for each reason we would add the lock. (What are the other reasons..?)

[Blaze Burg]

(In reply to comment #4)
> (In reply to comment #3)
> > What title attribute should we use?
> > 
> > "Style declaration cannot be modified"
> > "Style declaration is not editable"
> 
> It should be a sentence. Maybe, "User Agent styles cannot be modified." The
> User Agent part should probably be a separate UIString, one for each reason
> we would add the lock. (What are the other reasons..?)

"Style declaration" would be incorrect since the lock applies to the entire rule, not one property declaration.

[Nikita Vasilyev]

(In reply to comment #4)
> (In reply to comment #3)
> > What title attribute should we use?
> > 
> > "Style declaration cannot be modified"
> > "Style declaration is not editable"
> 
> It should be a sentence. Maybe, "User Agent styles cannot be modified." The
> User Agent part should probably be a separate UIString, one for each reason
> we would add the lock. (What are the other reasons..?)

We have a UIString for "User Agent Stylesheet", so how about "User Agent Stylesheet cannot be modified."? I.e.:

    WebInspector.UIString("%s cannot be modified.").format(WebInspector.UIString("User Agent Stylesheet"))

[Devin Rousso]

(In reply to comment #6)
> (In reply to comment #4)
> > It should be a sentence. Maybe, "User Agent styles cannot be modified." The
> > User Agent part should probably be a separate UIString, one for each reason
> > we would add the lock. (What are the other reasons..?)
> 
> We have a UIString for "User Agent Stylesheet", so how about "User Agent
> Stylesheet cannot be modified."? I.e.:
> 
>     WebInspector.UIString("%s cannot be
> modified.").format(WebInspector.UIString("User Agent Stylesheet"))

I don't think this is the only reason for a lock being applied.  From what I can see, it looks like anything that isn't an Author or Inspector rule (not including inline) has the lock applied.

[Nikita Vasilyev]

Created attachment 298613 [details]
Patch

Please remind me, should I still use `git diff --binary` when localizedStrings.js is modified? Without "--binary" it doesn't show up in the diff.

    ⮀file -I Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js
    Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js: text/plain; charset=utf-16be

[Joseph Pecoraro]

(In reply to comment #8)
> Created attachment 298613 [details]
> Patch
> 
> Please remind me, should I still use `git diff --binary` when
> localizedStrings.js is modified? Without "--binary" it doesn't show up in
> the diff.

This means somebody changed the format of the file to binary when it wasn't.

[Nikita Vasilyev]

Created attachment 298621 [details]
Patch

[Matt Baker]

Comment on attachment 298621 [details]
Patch

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

r=me, with suggestions

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:41
> +localizedStrings["%s cannot be modified."] = "%s cannot be modified.";

Tooltips shouldn't end in a period unless there is more than one sentence. I couldn't find any Apple design guidelines backing this up, but it generally seems to be Apple's practice. Xcode follows this convention, and so does most of the Inspector.

I'll file a separate bug to make all tooltip text consistent.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:51
> +            let lockedIconElement = this._headerElement.createChild("img", "locked-icon");

In the past I think it's been recommended that we standardize on createElement/append and avoid using this convenience method.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:55
> +                lockedIconElement.title = WebInspector.UIString("%s cannot be modified.").format(WebInspector.UIString("Style rule"));

This could be rewritten so the UI string "%s cannot be modified" doesn't have to be repeated:

let styleLabel;
if (style.ownerRule && style.ownerRule.type === WebInspector.CSSStyleSheet.Type.UserAgent)
    styleLabel = WebInspector.UIString("User Agent Stylesheet");
else
    styleLabel = WebInspector.UIString("Style rule"));

lockedIconElement.title = WebInspector.UIString("%s cannot be modified.").format(styleLabel);

[Nikita Vasilyev]

Created attachment 298825 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 298825 [details]
Patch

Clearing flags on attachment: 298825

Committed r210756: <http://trac.webkit.org/changeset/210756>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Blaze Burg]

Comment on attachment 298621 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:41
>> +localizedStrings["%s cannot be modified."] = "%s cannot be modified.";
> 
> Tooltips shouldn't end in a period unless there is more than one sentence. I couldn't find any Apple design guidelines backing this up, but it generally seems to be Apple's practice. Xcode follows this convention, and so does most of the Inspector.
> 
> I'll file a separate bug to make all tooltip text consistent.

Here's your missing reference:

https://developer.apple.com/library/content/documentation/UserExperience/Conceptual/OSXHIGuidelines/Assistance.html

[Matt Baker]

(In reply to comment #15)
> Comment on attachment 298621 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298621&action=review
> 
> >> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:41
> >> +localizedStrings["%s cannot be modified."] = "%s cannot be modified.";
> > 
> > Tooltips shouldn't end in a period unless there is more than one sentence. I couldn't find any Apple design guidelines backing this up, but it generally seems to be Apple's practice. Xcode follows this convention, and so does most of the Inspector.
> > 
> > I'll file a separate bug to make all tooltip text consistent.
> 
> Here's your missing reference:
> 
> https://developer.apple.com/library/content/documentation/UserExperience/
> Conceptual/OSXHIGuidelines/Assistance.html

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