Summary: | Web Inspector: Does not have search navigation button for going through matches in either direction (prev, next) | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | sam <dsam2912> | ||||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Enhancement | CC: | apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, vivekgalatage, yurys | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Attachments: |
|
I am looking into it and will upload the patch soon! (In reply to comment #1) > I am looking into it and will upload the patch soon! Please describe the way you intend to address it (mock?). I was planning on re-implementing search so that it was browser-alike: little search field with up/down arrows sliding into the viewer area upon Ctrl+F. Does this sound close to what you are planning? (In reply to comment #2) > (In reply to comment #1) > > I am looking into it and will upload the patch soon! > > Please describe the way you intend to address it (mock?). I was planning on re-implementing search so that it was browser-alike: little search field with up/down arrows sliding into the viewer area upon Ctrl+F. Does this sound close to what you are planning? Hi Pavel, I was thinking of adding up/down arrows right next to the search field which is there currently. The thought was to make arrows accessible/visible only when there is some matched content to navigate through. So if we do not have any match or only 1 match, arrows will be non-accessible. Is it ok? View wise I was planning to extend current search panel view. (In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > I am looking into it and will upload the patch soon! > > > > Please describe the way you intend to address it (mock?). I was planning on re-implementing search so that it was browser-alike: little search field with up/down arrows sliding into the viewer area upon Ctrl+F. Does this sound close to what you are planning? > > Hi Pavel, > > I was thinking of adding up/down arrows right next to the search field which is there currently. The thought was to make arrows accessible/visible only when there is some matched content to navigate through. So if we do not have any match or only 1 match, arrows will be non-accessible. > Is it ok? > > View wise I was planning to extend current search panel view. As long as it is bound to the search field and allows further refactoring into the form I was suggesting, it sounds good to me. Thanks. Created attachment 137689 [details]
Patch
Comment on attachment 137689 [details] Patch Attachment 137689 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12426320 Comment on attachment 137689 [details] Patch Attachment 137689 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12424548 Please provide a screenshot taken with the patch applied. Created attachment 137844 [details]
Screenshot for uploaded patch showing view of search nav keys.
Different scenarios showing access of search button has been put in one image file.
Created attachment 137845 [details]
Patch
Comment on attachment 137845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137845&action=review > Source/WebCore/inspector/front-end/WebKit.qrc:270 > + <file>Images/searchNext.png</file> You should add these images into WebCore.gypi as well. > Source/WebCore/inspector/front-end/inspector.css:282 > + background: -webkit-gradient(linear, left top, right top, from(rgb(230, 230, 230)), to(rgb(230, 230, 230)), color-stop(40%, rgb(252, 252, 252))); You should not hard-code gradients - they don't look good on non-Mac / different Mac OS versions. > Source/WebCore/inspector/front-end/inspector.css:291 > + background: -webkit-gradient(linear, left top, right top, from(rgb(128, 128, 128)), to(rgb(164, 164, 164)), color-stop(25%, rgb(164, 164, 164))); ditto > Source/WebCore/inspector/front-end/inspector.css:297 > + margin: 5px 0px 0px 5px; These margins look a bit large to me. > Source/WebCore/inspector/front-end/inspector.html:215 > + <div class="toolbar-search-nav-ctrl"> We prefer to not add markup here, but to create it lazily using DOM api. Created attachment 137908 [details]
Patch
Comment on attachment 137908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137908&action=review > Source/WebCore/WebCore.gypi:6555 > + 'inspector/front-end/Images/searchBtnBck.png', We try to not abbreviate in WebKit. searchButtonBack, etc. > Source/WebCore/inspector/front-end/SearchController.js:40 > + this._searchControlBoxElement = document.getElementById("toolbar-search-nav-ctrl"); -navigation-control > Source/WebCore/inspector/front-end/SearchController.js:258 > + _onNextBtnSearch: function(event) No abbreviations please. > Source/WebCore/inspector/front-end/SearchController.js:264 > + _onPrevBtnSearch: function(event) ditto > Source/WebCore/inspector/front-end/SearchController.js:336 > + _createSearchNavBtn: function(direction) { ditto. Also, you should place { on the next line. > Source/WebCore/inspector/front-end/SearchController.js:340 > + switch(direction) { Do not offset case from switch, use 4 characters for indentation within case blocks. > Source/WebCore/inspector/front-end/SearchController.js:342 > + searchNavControlElement.setAttribute("title" , "Search Previous"); You should use WebInspector.UIString("Search Previous") and add the string itself into localizedStrings.js (WebCore/English.lproj). > Source/WebCore/inspector/front-end/SearchController.js:359 > + _populateSearchNavButtons: function() { { on the next line > Source/WebCore/inspector/front-end/inspector.css:279 > + background-image: url('Images/searchBtnBck.png'); Sorry for not being clear in my previous review. I don't think images will make it better - they are still not in line with the theme the OS might use for the inspector. We try to only use thin borders and/or icons with (semi-)transparent background when we make new controls. Created attachment 138041 [details]
Patch
Thanks Pavel for comments. (In reply to comment #13) > (From update of attachment 137908 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137908&action=review > > > Source/WebCore/WebCore.gypi:6555 > > + 'inspector/front-end/Images/searchBtnBck.png', > > We try to not abbreviate in WebKit. searchButtonBack, etc. > corrected. > > Source/WebCore/inspector/front-end/SearchController.js:40 > > + this._searchControlBoxElement = document.getElementById("toolbar-search-nav-ctrl"); > > -navigation-control > corrected. > > Source/WebCore/inspector/front-end/SearchController.js:258 > > + _onNextBtnSearch: function(event) > > No abbreviations please. > corrected. > > Source/WebCore/inspector/front-end/SearchController.js:264 > > + _onPrevBtnSearch: function(event) > > ditto > corrected. > > Source/WebCore/inspector/front-end/SearchController.js:336 > > + _createSearchNavBtn: function(direction) { > > ditto. Also, you should place { on the next line. > my bad!, corrected. > > Source/WebCore/inspector/front-end/SearchController.js:340 > > + switch(direction) { > > Do not offset case from switch, use 4 characters for indentation within case blocks. > corrected. > > Source/WebCore/inspector/front-end/SearchController.js:342 > > + searchNavControlElement.setAttribute("title" , "Search Previous"); > > You should use WebInspector.UIString("Search Previous") and add the string itself into localizedStrings.js (WebCore/English.lproj). > Thank you, corrected. > > Source/WebCore/inspector/front-end/SearchController.js:359 > > + _populateSearchNavButtons: function() { > > { on the next line > > > Source/WebCore/inspector/front-end/inspector.css:279 > > + background-image: url('Images/searchBtnBck.png'); > > Sorry for not being clear in my previous review. I don't think images will make it better - they are still not in line with the theme the OS might use for the inspector. We try to only use thin borders and/or icons with (semi-)transparent background when we make new controls. I have put in border with no background. Is it ok? (In reply to comment #14) > Created an attachment (id=138041) [details] > Patch Patch, incorporated with review comments, uploaded. Comment on attachment 138041 [details] Patch Attachment 138041 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12476086 Comment on attachment 138041 [details] Patch Attachment 138041 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12479075 Created attachment 138043 [details]
Patch
Comment on attachment 138043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138043&action=review This is almost ready, few nits left. > Source/WebCore/inspector/front-end/SearchController.js:344 > + case "prev": As I mentioned, case should be below switch, no indentation. > Source/WebCore/inspector/front-end/inspector.css:279 > + -webkit-border-image: none; Remove this. > Source/WebCore/inspector/front-end/inspector.css:280 > + border: 1px solid rgba(120, 120, 120, 0.4); On my box, it looks like there is a gray border by default that vanishes on hover. I'd suggest to make this one transparent and move the definition into the :hover. > Source/WebCore/inspector/front-end/inspector.css:290 > + -webkit-border-image: none; Remove this. > Source/WebCore/inspector/front-end/inspector.css:302 > + background-image: url('Images/searchPrev.png'); We do not quote paths in url() > Source/WebCore/inspector/front-end/inspector.css:306 > + background-image: url('Images/searchNext.png'); ditto Created attachment 138088 [details]
Patch
Thank you Pavel for review. I have uploaded the patch with review comments incorporated. (In reply to comment #20) > (From update of attachment 138043 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138043&action=review > > This is almost ready, few nits left. > > > Source/WebCore/inspector/front-end/SearchController.js:344 > > + case "prev": > have corrected it. > As I mentioned, case should be below switch, no indentation. > > > Source/WebCore/inspector/front-end/inspector.css:279 > > + -webkit-border-image: none; > corrected > Remove this. > > > Source/WebCore/inspector/front-end/inspector.css:280 > > + border: 1px solid rgba(120, 120, 120, 0.4); > > On my box, it looks like there is a gray border by default that vanishes on hover. I'd suggest to make this one transparent and move the definition into the :hover. > changed > > Source/WebCore/inspector/front-end/inspector.css:290 > > + -webkit-border-image: none; > > Remove this. > corrected > > Source/WebCore/inspector/front-end/inspector.css:302 > > + background-image: url('Images/searchPrev.png'); > > We do not quote paths in url() > changed > > Source/WebCore/inspector/front-end/inspector.css:306 > > + background-image: url('Images/searchNext.png'); > > ditto changed Committed r114748: <http://trac.webkit.org/changeset/114748> Comment on attachment 138088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138088&action=review > Source/WebCore/inspector/front-end/inspector.css:264 > + -webkit-box-orient: horizontal; It breaks positioning of #search-toolbar-label when Inspector undocked or docked to right. http://elv1s.ru/i/toolbar-search-label-horizontal.png |
Created attachment 137668 [details] current search panel view Currently, to navigate between different search matches in either direction (prev or next) we have enter/shift+enter key events as only way. Enabling search navigation through button clicks may add intuitiveness to search.