Bug 179625 – Web Inspector: ButtonNavigationItem should support image and text button style

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Created attachment 326781 [details]
[Image] Button with image/text

Make ButtonNavigationItem more flexible. Currently it auto-detects the button style (image or text-only) at construction time, based on arguments passed to the constructor. It isn't possible to create a button with both an image and text.

This patch adds a button style enum. Buttons continue to detect the correct style when created (Image or Text), but can also be set to ImageAndText:

    let item = new WI.ButtonNavigationItem("import", WI.UIString("Import"), "Images/Import.svg", 16, 16);
    item.buttonStyle = WI.ButtonNavigationItem.Style.ImageAndText;

[Radar WebKit Bug Importer]

<rdar://problem/35512238>

[Joseph Pecoraro]

Interesting, I like it!

There seems to be a bit of extra padding. ~26px for this one compared to ~13px for the icon only items.

[Matt Baker]

Created attachment 326803 [details]
[Image] Alignment across all button types

Some tweaks were needed to get all three button types aligned properly. Icons should have same vertical alignment, whether they appear in image-only or text-and-image buttons.

[Matt Baker]

(In reply to Joseph Pecoraro from comment #2)
> Interesting, I like it!
> 
> There seems to be a bit of extra padding. ~26px for this one compared to
> ~13px for the icon only items.

Hmm, will investigate.

[Matt Baker]

Created attachment 326810 [details]
Patch

[Matt Baker]

(In reply to Matt Baker from comment #4)
> (In reply to Joseph Pecoraro from comment #2)
> > Interesting, I like it!
> > 
> > There seems to be a bit of extra padding. ~26px for this one compared to
> > ~13px for the icon only items.
> 
> Hmm, will investigate.

Image buttons have a 26px width. In this screenshot, the "refresh" button's icon is 13px wide, leaving 6.5px of padding on each side.

Text buttons have 2px of left/right margin, a 1px border, and 8px of left/right padding, for a total of 11px of padding on each side.

All the margin/padding/border styles are for scope/radio buttons, which use a background color for hover/activated effects.

[Matt Baker]

If we want to support hover effects for all buttons (like the close button in the new Network details view), then we'd want to add margin/padding to image buttons as well and we could further unify button styles.

[Devin Rousso]

Comment on attachment 326810 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:131
> +        for (let key in WI.ButtonNavigationItem.Style)
> +            this.element.classList.remove(WI.ButtonNavigationItem.Style[key]);

You could write this in one line.

    this.element.classList.remove(...Object.values(WI.ButtonNavigationItem.Style));

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:171
> +                this._labelElement = this.element.appendChild(document.createElement("span"));

I realize that `_update()` isn't called that often, but it would be nice if we didn't have to create a new element each time.  Also, it seems like you don't even really need to save this to a member variable, so I'd either rename it to a local variable `labelElement` or add a check to make sure it isn't recreated with each `_update()`.

> Source/WebInspectorUI/UserInterface/Views/CheckboxNavigationItem.js:73
> +    Checked: "checkbox-navigation-item-checked",

This name implies it will only be fired when the checkbox is checked, not when it is unchecked.  I'd rename it to "Update" to match the native event name.

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.css:61
> +.navigation-bar .item.checkbox {
> +    padding: 1px 8px 3px;
> +}
> +
> +.navigation-bar .item.checkbox label {
> +    -webkit-padding-start: 3px;
> +}

We should move this to its own CheckboxNavigationItem.css file.  I find it confusing when I'm looking for the styles of a particular class and there is no corresponding *.css file, and instead everything is in some parent file.

[Matt Baker]

Comment on attachment 326810 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:171
>> +                this._labelElement = this.element.appendChild(document.createElement("span"));
> 
> I realize that `_update()` isn't called that often, but it would be nice if we didn't have to create a new element each time.  Also, it seems like you don't even really need to save this to a member variable, so I'd either rename it to a local variable `labelElement` or add a check to make sure it isn't recreated with each `_update()`.

I'll make labelElement local and ditch the member variable. I think it's okay to recreate it each time, since _update isn't called often.

>> Source/WebInspectorUI/UserInterface/Views/CheckboxNavigationItem.js:73
>> +    Checked: "checkbox-navigation-item-checked",
> 
> This name implies it will only be fired when the checkbox is checked, not when it is unchecked.  I'd rename it to "Update" to match the native event name.

What do you think about CheckedDidChange?

>> Source/WebInspectorUI/UserInterface/Views/NavigationBar.css:61
>> +}
> 
> We should move this to its own CheckboxNavigationItem.css file.  I find it confusing when I'm looking for the styles of a particular class and there is no corresponding *.css file, and instead everything is in some parent file.

Sometimes I'm hesitant to add tiny CSS files, but I agree it can be confusing. Will add.

[Matt Baker]

Created attachment 327140 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 327140 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 327140 [details]
Patch

Clearing flags on attachment: 327140

Committed r224978: <https://trac.webkit.org/changeset/224978>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Hmm, when the button is disabled the icon grays out but not the text. Do you think the text should gray out a little?

[Matt Baker]

(In reply to Joseph Pecoraro from comment #14)
> Hmm, when the button is disabled the icon grays out but not the text. Do you
> think the text should gray out a little?

It definitely should! Will fix.