RESOLVED WONTFIX 173436
Web Inspector: Only enable the "Show Grid" navigation item if the image has transparent pixels
https://bugs.webkit.org/show_bug.cgi?id=173436
Summary Web Inspector: Only enable the "Show Grid" navigation item if the image has t...
Devin Rousso
Reported 2017-06-15 14:18:48 PDT
This will prevent awkward scenarios where the user clicks on the button, only for it to have no effect since the grid is not visible.
Attachments
Patch (6.59 KB, patch)
2017-06-15 15:04 PDT, Devin Rousso
no flags
Patch (4.58 KB, patch)
2017-06-15 15:06 PDT, Devin Rousso
joepeck: review-
Devin Rousso
Comment 1 2017-06-15 15:04:47 PDT
Devin Rousso
Comment 2 2017-06-15 15:06:26 PDT
Created attachment 313016 [details] Patch Oops. Included some extra stuff :P
Joseph Pecoraro
Comment 3 2017-06-15 16:17:42 PDT
Comment on attachment 313016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313016&action=review > Source/WebInspectorUI/UserInterface/Base/ImageUtilities.js:55 > +WebInspector.imageHasTransparency = function(imageElement, skip = 100) I think this may be unnecessarily complex. I'd think the expensive part would be getImageData, not looping over to detect if the alpha bit is < 255. So the skipping might not be necessary and might just lead to misdiagnosing an image with transparency. I think we should also be able to write a simple test for this with some tiny images. > Source/WebInspectorUI/UserInterface/Base/ImageUtilities.js:71 > + for (let i = 3; i < imageData.data.length; i += 4 * skip) { If `skip` is 0 then this will infinite loop. Seems like it is easy to mis-use this function. > Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:70 > + this._imageElement.addEventListener("load", (event) => { > + let imageHasTransparency = WebInspector.imageHasTransparency(this._imageElement); > + this._showGridButtonNavigationItem.enabled = imageHasTransparency; > + if (imageHasTransparency) > + this._toggleImageGrid(); > + > + URL.revokeObjectURL(objectURL); > + }); Style: Put this after setting the more direct attributes. The code is easier to read that way.
Joseph Pecoraro
Comment 4 2017-06-15 18:46:48 PDT
After discussion we may just not want to take this approach at all and just always leave the button enabled?
Devin Rousso
Comment 5 2017-06-15 23:36:27 PDT
(In reply to Joseph Pecoraro from comment #4) > After discussion we may just not want to take this approach at all and just > always leave the button enabled? Agreed. In the case that the image has no transparency, the fact that clicking the Show Grid button has no visible effect is a good indicator in itself.
Note You need to log in before you can comment on or make changes to this bug.