Bug 179625 - Web Inspector: ButtonNavigationItem should support image and text button style
Summary: Web Inspector: ButtonNavigationItem should support image and text button style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-13 12:47 PST by Matt Baker
Modified: 2017-11-17 14:27 PST (History)
5 users (show)

See Also:


Attachments
[Image] Button with image/text (72.93 KB, image/png)
2017-11-13 12:47 PST, Matt Baker
no flags Details
[Image] Alignment across all button types (45.30 KB, image/png)
2017-11-13 14:37 PST, Matt Baker
no flags Details
Patch (11.47 KB, patch)
2017-11-13 15:02 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (13.47 KB, patch)
2017-11-16 18:14 PST, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2017-11-13 12:47:46 PST
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;
Comment 1 Radar WebKit Bug Importer 2017-11-13 12:48:46 PST
<rdar://problem/35512238>
Comment 2 Joseph Pecoraro 2017-11-13 13:40:14 PST
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.
Comment 3 Matt Baker 2017-11-13 14:37:26 PST
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.
Comment 4 Matt Baker 2017-11-13 14:37:57 PST
(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.
Comment 5 Matt Baker 2017-11-13 15:02:34 PST
Created attachment 326810 [details]
Patch
Comment 6 Matt Baker 2017-11-13 15:18:28 PST
(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.
Comment 7 Matt Baker 2017-11-13 15:23:17 PST
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.
Comment 8 Devin Rousso 2017-11-14 20:09:46 PST
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.
Comment 9 Matt Baker 2017-11-16 18:06:52 PST
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.
Comment 10 Matt Baker 2017-11-16 18:14:56 PST
Created attachment 327140 [details]
Patch
Comment 11 Joseph Pecoraro 2017-11-17 12:19:13 PST
Comment on attachment 327140 [details]
Patch

r=me
Comment 12 WebKit Commit Bot 2017-11-17 12:38:30 PST
Comment on attachment 327140 [details]
Patch

Clearing flags on attachment: 327140

Committed r224978: <https://trac.webkit.org/changeset/224978>
Comment 13 WebKit Commit Bot 2017-11-17 12:38:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Joseph Pecoraro 2017-11-17 13:36:39 PST
Hmm, when the button is disabled the icon grays out but not the text. Do you think the text should gray out a little?
Comment 15 Matt Baker 2017-11-17 14:27:19 PST
(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.