Bug 173184

Created attachment 312493 [details] Patch

Created attachment 312494 [details] [Image] After Patch is applied

Comment on attachment 312493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312493&action=review > Source/WebInspectorUI/UserInterface/Base/Setting.js:123 > + showImageGrid: new WebInspector.Setting("show-image-grid", false), I think it's overkill to have a setting for this. I anticipate the argument against taking the form: Viewing my image against a grid makes for a poor experience because: a) Some or all of the pixels are the same color as the grid b) I want to see what my image looks like against a plain background I feel b) isn't super compelling, since an image with transparency could be designed for a non-white background as well. An argument could be made for a), but this will depend on the particular image, and the setting is global. A setting also adds complexity and more choices for the user to make for "the right thing to happen". Maybe we should allow toggling the grid temporarily, but not persist the setting?

A context menu on the image would allow the grid to be toggled per image, without adding UI clutter for something that (I speculate) won't be used super often.

Created attachment 312515 [details] Patch

Created attachment 312516 [details] [Image] After Patch is applied

Comment on attachment 312515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312515&action=review > Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.css:-47 > - Could you add a changelog comment explaining this change? > Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:38 > + this._showGridButtonNavigationItem.activated = !!WebInspector.settings.showImageGrid.value; There are a few problems with making this a navigation item: 1) If the button is collapsed (narrow Inspector), it's inaccessible. This isn't unique to this situation, but I think a context menu would fix this in a completely unobtrusive way. 2) The button won't always do something. If we had a way of detective images with transparency, it would be perfect: we could show the button/enable the button as needed. > Source/WebInspectorUI/UserInterface/Views/Main.css:421 > + */ Style: this ASCII art is very cool, but I'm not sure it adds value. I still can't see (at a glance) how we get from gradients to a grid, and would need to read up on `linear-gradient` if I wanted to make changes. That's fine, since this probably won't change much. If you think it adds a clear benefit, it should go in the changelog instead. > Source/WebInspectorUI/UserInterface/Views/Main.css:427 > + background-position: 10px 10px, This should be one line. Having four two-tuples on one line isn't hard to parse, and is well within column limits.

Comment on attachment 312515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312515&action=review >> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:38 >> + this._showGridButtonNavigationItem.activated = !!WebInspector.settings.showImageGrid.value; > > There are a few problems with making this a navigation item: > > 1) If the button is collapsed (narrow Inspector), it's inaccessible. This isn't unique to this situation, but I think a context menu would fix this in a completely unobtrusive way. > 2) The button won't always do something. If we had a way of detective images with transparency, it would be perfect: we could show the button/enable the button as needed. For (1) I don't think we ever collapse button navigation items. I like the idea in (2) of only showing the button if the image has any transparency.

Comment on attachment 312515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312515&action=review >>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:38 >>> + this._showGridButtonNavigationItem.activated = !!WebInspector.settings.showImageGrid.value; >> >> There are a few problems with making this a navigation item: >> >> 1) If the button is collapsed (narrow Inspector), it's inaccessible. This isn't unique to this situation, but I think a context menu would fix this in a completely unobtrusive way. >> 2) The button won't always do something. If we had a way of detective images with transparency, it would be perfect: we could show the button/enable the button as needed. > > For (1) I don't think we ever collapse button navigation items. I like the idea in (2) of only showing the button if the image has any transparency. Because of the current flex implementation even the show/hide sidebar buttons can become hidden in certain circumstances.

Comment on attachment 312515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312515&action=review >>>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:38 >>>> + this._showGridButtonNavigationItem.activated = !!WebInspector.settings.showImageGrid.value; >>> >>> There are a few problems with making this a navigation item: >>> >>> 1) If the button is collapsed (narrow Inspector), it's inaccessible. This isn't unique to this situation, but I think a context menu would fix this in a completely unobtrusive way. >>> 2) The button won't always do something. If we had a way of detective images with transparency, it would be perfect: we could show the button/enable the button as needed. >> >> For (1) I don't think we ever collapse button navigation items. I like the idea in (2) of only showing the button if the image has any transparency. > > Because of the current flex implementation even the show/hide sidebar buttons can become hidden in certain circumstances. I don't like the idea of putting this in the contextmenu. My thinking is that contextmenu should be used for additional actions related specifically to the item being acted upon. I see "Show Grid" as being more like "Pretty Print" in that the user is going to either want it on for every image, or for no images, but might want to switch between them sometimes. I can imagine a situation where a user is working with lots of transparent images and having to contextmenu each time to show the grid. I would rather it be in the navigation bar and face the risk of it being hidden due to the width of the inspector, an issue which I think can be resolved through other means anyways. I was discussing offline with Joe earlier today, but I think we can add additional UI when "Show Grid" is enabled (such as width and height rulers next to the image itself) so that images without transparency will still show something distinguishable. We both agreed, however, that this could be a followup. >> Source/WebInspectorUI/UserInterface/Views/Main.css:421 >> + */ > > Style: this ASCII art is very cool, but I'm not sure it adds value. I still can't see (at a glance) how we get from gradients to a grid, and would need to read up on `linear-gradient` if I wanted to make changes. That's fine, since this probably won't change much. If you think it adds a clear benefit, it should go in the changelog instead. I do think that this illustration is helpful in understanding how it creates the checkerboard pattern, but I do agree that it is a bit cryptic. I will add some additional explanation text and move this to the changelog. >> Source/WebInspectorUI/UserInterface/Views/Main.css:427 >> + background-position: 10px 10px, > > This should be one line. Having four two-tuples on one line isn't hard to parse, and is well within column limits. I was following the pattern set by `background-image` so that it is clearer that each line corresponds to a line in `background-image`. It's simple enough though that I can see it being unnecessary.

Created attachment 312564 [details] Patch

Comment on attachment 312564 [details] Patch r=me, nice work!

Comment on attachment 312564 [details] Patch Clearing flags on attachment: 312564 Committed r218159: <http://trac.webkit.org/changeset/218159>

All reviewed patches have been landed. Closing bug.