Bug 95654 – Web Inspector: Remove toolbar icons by default, and add settings options to show them

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

By default, only show text labels in the toolbar, but add a settings option to show the icons.

Created attachment 161880 [details]
Patch

[WebKit Review Bot]

Attachment 161880 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 161880 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Please attach screenshots with changes to the UI.

> Source/WebCore/inspector/front-end/Settings.js:47
> +    showUseToolbarIcons: true

I don't think we need this.

> Source/WebCore/inspector/front-end/SettingsScreen.js:254
> +        p.appendChild(this._createCheckboxSetting(WebInspector.UIString("Show toolbar icons"), WebInspector.settings.useToolbarIcons));

Strings should go to the WebCore/English.lproj/localizedStrings.js

> Source/WebCore/inspector/front-end/Toolbar.js:167
> +    _useToolbarIconsSettingChanged: function(event) {

{ on the next line.

> Source/WebCore/inspector/front-end/Toolbar.js:171
> +    _setIconsVisible: function(visible) {

ditto

> Source/WebCore/inspector/front-end/Toolbar.js:249
> +            if (toolbarItems[i].offsetTop > 1)

Should we compare things with 1 pixel? What is the reason behind this change?

> Source/WebCore/inspector/front-end/inspector.css:68
> +#toolbar.with-icons + #main {

We don't use sibling selectors.

> Source/WebCore/inspector/front-end/inspector.css:99
> +body.compact #toolbar.with-icons + #main {

ditto

> Source/WebCore/inspector/front-end/inspector.css:208
> +    /* A line height of 0 allows precise text positioning using padding. */

I was hoping that this change would be simpler. Is there a way to optimize the styles here?

Created attachment 161881 [details]
Screenshot showing the new UI in the different states

(In reply to comment #3)
> (From update of attachment 161880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161880&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        Reviewed by NOBODY (OOPS!).
> 
> Please attach screenshots with changes to the UI.

Done.
 
> > Source/WebCore/inspector/front-end/Settings.js:47
> > +    showUseToolbarIcons: true
> 
> I don't think we need this.

Ok, I'll remove it.

> > Source/WebCore/inspector/front-end/SettingsScreen.js:254
> > +        p.appendChild(this._createCheckboxSetting(WebInspector.UIString("Show toolbar icons"), WebInspector.settings.useToolbarIcons));
> 
> Strings should go to the WebCore/English.lproj/localizedStrings.js
>
> > Source/WebCore/inspector/front-end/Toolbar.js:167
> > +    _useToolbarIconsSettingChanged: function(event) {
> 
> { on the next line.
> 
> > Source/WebCore/inspector/front-end/Toolbar.js:171
> > +    _setIconsVisible: function(visible) {
> 
> ditto
> 
> > Source/WebCore/inspector/front-end/Toolbar.js:249
> > +            if (toolbarItems[i].offsetTop > 1)
> 
> Should we compare things with 1 pixel? What is the reason behind this change?

Since the body now has a 1px border when attached, the toolbar items will all have an offsetTop of 1.
 
> > Source/WebCore/inspector/front-end/inspector.css:68
> > +#toolbar.with-icons + #main {
> 
> We don't use sibling selectors.
> 
> > Source/WebCore/inspector/front-end/inspector.css:99
> > +body.compact #toolbar.with-icons + #main {
> 
> ditto
> 
> > Source/WebCore/inspector/front-end/inspector.css:208
> > +    /* A line height of 0 allows precise text positioning using padding. */
> 
> I was hoping that this change would be simpler. Is there a way to optimize the styles here?

In the current UI, there are several places where things are not centered. I decided to fix those in addition to removing the toolbar icons. Would it be better to separate it into two patches?

[Pavel Feldman]

> In the current UI, there are several places where things are not centered. I decided to fix those in addition to removing the toolbar icons. Would it be better to separate it into two patches?

Yes please! It actually confused me...

[Pavel Feldman]

Created attachment 178359 [details]
[Patch] Rebaselined, made the mode opt-in, removed compact mode

[Vsevolod Vlasov]

Comment on attachment 178359 [details]
[Patch] Rebaselined, made the mode opt-in, removed compact mode

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

> Source/WebCore/ChangeLog:8
> +        - removes compact mode in favor or dock-to-bottom

of

[Pavel Feldman]

Committed r137133: <http://trac.webkit.org/changeset/137133>

[Vivek Galatage]

(In reply to comment #0)
> By default, only show text labels in the toolbar, but add a settings option to show the icons.

Should it be enabled by default? I updated to TOT and to my surprise things started looking differently. I had to trace back from the trac.webkit.org to find this change. Only when I stumbled upon here I came to know that there exists the option to change this behavior. I am sure many developers will face this. Just my thoughts.

[Pavel Feldman]

> Should it be enabled by default? I updated to TOT and to my surprise things started looking differently. I had to trace back from the trac.webkit.org to find this change. Only when I stumbled upon here I came to know that there exists the option to change this behavior. I am sure many developers will face this. Just my thoughts.

Yes, it should. Sorry for making it look confusing in the changelog - was a last minute decision. Iconless looks is where we headed long term, so I thought that making it default would be ok.