it would be awesome to be able to zoom in and pan around images in the Sources Tab
Created attachment 426190 [details] Patch
Comment on attachment 426190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426190&action=review Nice! Provisional r+ from me. > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1178 > +localizedStrings["Reset"] = "Reset"; Should this have a localization description? > Source/WebInspectorUI/UserInterface/Controllers/GestureController.js:26 > +WI.GestureController = class GestureController It feels like the new GestureController would also work well for the TimelineOverview where it seems like much of this code originates.
Comment on attachment 426190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426190&action=review >> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1178 >> +localizedStrings["Reset"] = "Reset"; > > Should this have a localization description? yeah probably i was being lazy/tired 😅 >> Source/WebInspectorUI/UserInterface/Controllers/GestureController.js:26 >> +WI.GestureController = class GestureController > > It feels like the new GestureController would also work well for the TimelineOverview where it seems like much of this code originates. Maybe? I did seek some inspiration from that, but honestly it didn't really fit because `WI.TimelineOverview` has its own way of maintaining the equivalent `_scale` (it uses `secondsPerPixel`) and `_translate` (it uses `scrollStartTime` but also only for changes on the X axis), so sharing code may result in some subtle bugs. I'd rather get this working and then consider using/adjusting this for that code.
Comment on attachment 426190 [details] Patch Overall I like the GestureController approach. Feedback on the feature itself: 1. Dragging only seems to work when clicking off the image. I kept trying to drag the image itself around. 2. I quickly wish there was a sense of scale for the zoom. For example, auto-showing rulers would be good. 3. For accessibility reasons, we should offer Zoom in button, Zoom out button, and current scale. 4. I highly prefer that we install a floating overlay in the upper right (trailing) corner with zoom specific UI 5. I think the initial version needs scroll bars when the zoomed content overflows the view container. This will help accessibility a bit. 6. Much lower down the list would be zoom related system keyboard shortcuts like Cmd+, Cmd-, Cmd-0, Cmd-9, all as they are in Preview.app. And lastly, 7. Images in the image collection content view should not have gesture recognizer active and should not be able to zoom. It just gets really.. awkward. See screenshot. I think #1, 5, and 7 should be addressed in the initial go, since they are more bugs than not IMO.
(In reply to BJ Burg from comment #4) > 1. Dragging only seems to work when clicking off the image. I kept trying to drag the image itself around. This is intentional. I felt that dragging the image is still a useful feature, so `WI.GestureController` will ignore attempts to translate if the initial `"mousedown"` target is `draggable. > 2. I quickly wish there was a sense of scale for the zoom. For example, auto-showing rulers would be good. Oh yeah this would be cool. Rulers might be a whole extra level of effort, but at least showing a value of `1.37×` would be nice. > 3. For accessibility reasons, we should offer Zoom in button, Zoom out button, and current scale. Can do. I think frankly it may be better to just have the controls always visible (i.e. always show "Reset" and just have it be disabled when not modified or something). > 4. I highly prefer that we install a floating overlay in the upper right (trailing) corner with zoom specific UI Hmm, IMO if we had "[Reset] [Zoom In] [Zoom Out]" buttons I kinda feel like that'd look nicer centered at the bottom rather than as a top-right control bar area. > 5. I think the initial version needs scroll bars when the zoomed content overflows the view container. This will help accessibility a bit. The problem with showing scrollbars is that it suggests that the content is scrollable, which would conflict with using the mouse wheel as the zoom. > 6. Much lower down the list would be zoom related system keyboard shortcuts like Cmd+, Cmd-, Cmd-0, Cmd-9, all as they are in Preview.app. These would conflict with the top-level keyboard shortcuts already bound to those keys that switch Web Inspector tabs or zoom in/out the entire interface. > 7. Images in the image collection content view should not have gesture recognizer active and should not be able to zoom. It just gets really.. awkward. See screenshot. LOL totally forgot about this. Will fix :)
(In reply to Devin Rousso from comment #5) > (In reply to BJ Burg from comment #4) > > 1. Dragging only seems to work when clicking off the image. I kept trying to drag the image itself around. > > This is intentional. I felt that dragging the image is still a useful > feature, so `WI.GestureController` will ignore attempts to translate if the > initial `"mousedown"` target is `draggable. OK > > 2. I quickly wish there was a sense of scale for the zoom. For example, auto-showing rulers would be good. > > Oh yeah this would be cool. Rulers might be a whole extra level of effort, > but at least showing a value of `1.37×` would be nice. > > > 3. For accessibility reasons, we should offer Zoom in button, Zoom out button, and current scale. > > Can do. I think frankly it may be better to just have the controls always > visible (i.e. always show "Reset" and just have it be disabled when not > modified or something). OK > > 4. I highly prefer that we install a floating overlay in the upper right (trailing) corner with zoom specific UI > > Hmm, IMO if we had "[Reset] [Zoom In] [Zoom Out]" buttons I kinda feel like > that'd look nicer centered at the bottom rather than as a top-right control > bar area. Centered buttons seem really unpredictable as hit targets. Maybe the zoom level & buttons can go in a status bar type thing? Like the one used to show page weight? If we go the banner route, I'd prefer a banner with action items at the top instead of the bottom, so that people can discover it easily. > > > 5. I think the initial version needs scroll bars when the zoomed content overflows the view container. This will help accessibility a bit. > > The problem with showing scrollbars is that it suggests that the content is > scrollable, which would conflict with using the mouse wheel as the zoom. Hmm, I guess what I'm saying is that if I'm super zoomed in, it can be hard to tell what part of the image I am viewing. Scrollbars are one way to visualize amount of overflow, a mini-map is another (definitely more work). It's not a blocker for this patch. > > 6. Much lower down the list would be zoom related system keyboard shortcuts like Cmd+, Cmd-, Cmd-0, Cmd-9, all as they are in Preview.app. > > These would conflict with the top-level keyboard shortcuts already bound to > those keys that switch Web Inspector tabs or zoom in/out the entire > interface. OK
Created attachment 426761 [details] Patch
Created attachment 426762 [details] [Image] after Patch is applied
Comment on attachment 426761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426761&action=review > Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:241 > + this._resetGestureButtonNavitationItem.label = Number.scaleToString(this._gestureController.scale); @Joseph Pecoraro suggested offline that we use `Number.percentageToString(this._gestureController.scale, 0)` instead. I agree and will make this change.
Created attachment 426876 [details] Patch
<rdar://problem/77061293>
Comment on attachment 426876 [details] Patch Cool!
Comment on attachment 426876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426876&action=review > Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.css:60 > + font-variant-numeric: tabular-nums; num num…
Committed r277279 (237544@main): <https://commits.webkit.org/237544@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426876 [details].