Bug 200212 – Web Inspector: Resources: Display outline around images when viewing image collections

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 375053 [details]
[Image] Before

I love how we display all images when selecting "Images" folder in the navigation sidebar.

Many websites use image sprites. google.com is one of those websites. When selecting "Images" folder,
it's hard to guess how many image resources I'm looking at.

[Nikita Vasilyev]

Created attachment 375054 [details]
[Image] After

[Nikita Vasilyev]

Created attachment 375055 [details]
Patch

[Devin Rousso]

Comment on attachment 375055 [details]
Patch

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

r-

I like the idea, but I don't think we should only show a "highlight"/"callout" on `:hover`, as that requires user interaction (see comment below)

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:37
> +    outline: 1px solid var(--text-color-quaternary);

This is an odd use of this variable.  The outline of an <img> has nothing to do with text colors.  For these cases, I think we either should generalize our variable names ("--text-color-quaternary" to just "--color-quaternary"), or not even use a variable (which is fine, given that this a specific usage).

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:38
> +    cursor: pointer;

We typically only use `cursor: pointer;` for actual links, not clickable things.  Please remove.

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:41
> +.content-view.collection .resource.image img:hover {

"limiting" this to only on `:hover` doesn't actually solve the issue described in this bug.  All it does it make it clearer during an interaction, but not at a glance.  I'd either expect us to _always_ add an `outline`/`border`/`box-shadow` or do some sort of other always-visible styling.

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 375055 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375055&action=review
> 
> r-
> 
> I like the idea, but I don't think we should only show a
> "highlight"/"callout" on `:hover`, as that requires user interaction (see
> comment below)

Based on the Before/After images, it looks like there was a gray border when not-interacting. That actually looks really good! Did that alone not satisfy your concern?

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #4)
> Based on the Before/After images, it looks like there was a gray border when not-interacting.
Oh I totally missed that!  Perhaps we should increase the contrast so it's more visible (given that I didn't notice it before)?

> That actually looks really good! Did that alone not satisfy your concern?
I'm not a fan of the color changing when hovering, so I'd still like that removed.

While we're at it, I think it would be awesome to add a `WI.NavigationItem` for toggling the image transparent background style (the checkerboard icon).  We already show it for individual image resources (and canvas rendering contexts).

[Nikita Vasilyev]

Comment on attachment 375055 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:37
>> +    outline: 1px solid var(--text-color-quaternary);
> 
> This is an odd use of this variable.  The outline of an <img> has nothing to do with text colors.  For these cases, I think we either should generalize our variable names ("--text-color-quaternary" to just "--color-quaternary"), or not even use a variable (which is fine, given that this a specific usage).

--text-color-quaternary is only used as a border/outline color. This is confusing.

    /* Dividers, separators, background fills */
    --text-color-quaternary: hsl(0, 0%, 85%);

This was actually a part of Dark Mode guidelines in 2018, but this doesn't seem to be the case anymore!

    Quaternary label color	The text of a label of lesser importance than a tertiary label such as watermark text.

https://developer.apple.com/design/human-interface-guidelines/ios/visual-design/color/#dynamic-system-colors

Perhaps I should add this variable:

    Separator color	A separator between different sections of content.

>> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:41
>> +.content-view.collection .resource.image img:hover {
> 
> "limiting" this to only on `:hover` doesn't actually solve the issue described in this bug.  All it does it make it clearer during an interaction, but not at a glance.  I'd either expect us to _always_ add an `outline`/`border`/`box-shadow` or do some sort of other always-visible styling.

The light outline (line 37) is always visible, even without hover.

I'd be fine with a light box-shadow as well, but I don't see a lot of shadows in the latest macOS releases.

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #5)
> (In reply to Joseph Pecoraro from comment #4)
> > Based on the Before/After images, it looks like there was a gray border when not-interacting.
> Oh I totally missed that!  Perhaps we should increase the contrast so it's
> more visible (given that I didn't notice it before)?

Probably quaternary is a bit dim.

Navigation items normally have a currentColor of:

    color: hsl(0, 0%, 45%);

And on active:

    color: hsl(0, 0%, 30%);

Maybe those would work better.

> I'm not a fan of the color changing when hovering, so I'd still like that
> removed.

I like the hover. It would be like our :normal / :hover / :active colors highlights on other items. I could see clicking the image taking directly to that Image Resource, which would be pretty nice!

> While we're at it, I think it would be awesome to add a `WI.NavigationItem`
> for toggling the image transparent background style (the checkerboard icon).
> We already show it for individual image resources (and canvas rendering
> contexts).

This sounds like a good idea. Could be done separately.

[Joseph Pecoraro]

> I like the hover. It would be like our :normal / :hover / :active colors
> highlights on other items. I could see clicking the image taking directly to
> that Image Resource, which would be pretty nice!

I say this having not used it yet myself. It just seems like it might focus more when mousing over, but I don't know if it would be distracting in normal use or not.

[Nikita Vasilyev]

(In reply to Joseph Pecoraro from comment #7)
> I like the hover. It would be like our :normal / :hover / :active colors
> highlights on other items. I could see clicking the image taking directly to
> that Image Resource, which would be pretty nice!

We already do this! I thought changing cursor and adding border hightligh would make it more obvious.

[Nikita Vasilyev]

Created attachment 375121 [details]
Patch

I changed the outline to be darker (in the light mode; lighter in the dark mode) and removed the hand cursor. I kept the accent color outline on hover.

[Nikita Vasilyev]

Created attachment 375122 [details]
[Image] After

[Devin Rousso]

Comment on attachment 375121 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:37
> +    outline: 1px solid var(--border-color);

What does this look like with a 0.5px outline-width instead (or with a lighter color, like maybe a `--border-color-light`)?  I'm concerned that it's too similar to the various borders around the `WI.ContentView`.  Ideally, this border should identify the bounds of each image, but not so much so that its confusing as to where the actual content area begins/ends.

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:41
> +    outline-color: var(--glyph-color-active)

It's unfortunate that the only CSS variable we have for the system accent color is "limited" to glyphs :(

A more generic name would've been nice, like `--target-color-active` or `--active-color-active`.

[Devin Rousso]

Comment on attachment 375121 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css:37
>> +    outline: 1px solid var(--border-color);
> 
> What does this look like with a 0.5px outline-width instead (or with a lighter color, like maybe a `--border-color-light`)?  I'm concerned that it's too similar to the various borders around the `WI.ContentView`.  Ideally, this border should identify the bounds of each image, but not so much so that its confusing as to where the actual content area begins/ends.

Oh also, why not use a `border`?  I realize that a `border` would affect the sizing of the element, but I think that's what we want in this case (everything should already have extra padding, so there's no worry of an overlap).

[Nikita Vasilyev]

Created attachment 375175 [details]
Patch

I like the 0.5px wide outline.

[Nikita Vasilyev]

Created attachment 375176 [details]
[Image] After

[Devin Rousso]

Comment on attachment 375175 [details]
Patch

r=me, thanks for iterating!  looks awesome :)

[WebKit Commit Bot]

Comment on attachment 375175 [details]
Patch

Clearing flags on attachment: 375175

Committed r248019: <https://trac.webkit.org/changeset/248019>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53730622>