Bug 84235

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:
Description Flags
current search panel view
none
Patch
none
Screenshot for uploaded patch showing view of search nav keys.
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch pfeldman: review+

Description sam 2012-04-18 04:15:20 PDT
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.
Comment 1 sam 2012-04-18 04:20:44 PDT
I am looking into it and will upload the patch soon!
Comment 2 Pavel Feldman 2012-04-18 04:51:41 PDT
(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?
Comment 3 sam 2012-04-18 06:21:00 PDT
(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.
Comment 4 Pavel Feldman 2012-04-18 06:31:44 PDT
(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.
Comment 5 sam 2012-04-18 08:06:06 PDT
Created attachment 137689 [details]
Patch
Comment 6 Early Warning System Bot 2012-04-18 08:28:21 PDT
Comment on attachment 137689 [details]
Patch

Attachment 137689 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12426320
Comment 7 Early Warning System Bot 2012-04-18 08:31:19 PDT
Comment on attachment 137689 [details]
Patch

Attachment 137689 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12424548
Comment 8 Pavel Feldman 2012-04-18 08:32:02 PDT
Please provide a screenshot taken with the patch applied.
Comment 9 sam 2012-04-18 22:52:54 PDT
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.
Comment 10 sam 2012-04-18 22:54:03 PDT
Created attachment 137845 [details]
Patch
Comment 11 Pavel Feldman 2012-04-19 04:37:31 PDT
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.
Comment 12 sam 2012-04-19 08:56:23 PDT
Created attachment 137908 [details]
Patch
Comment 13 Pavel Feldman 2012-04-19 09:17:59 PDT
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.
Comment 14 sam 2012-04-19 22:28:22 PDT
Created attachment 138041 [details]
Patch
Comment 15 sam 2012-04-19 22:32:49 PDT
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?
Comment 16 sam 2012-04-19 22:33:37 PDT
(In reply to comment #14)
> Created an attachment (id=138041) [details]
> Patch

Patch, incorporated with review comments, uploaded.
Comment 17 Early Warning System Bot 2012-04-19 22:35:43 PDT
Comment on attachment 138041 [details]
Patch

Attachment 138041 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12476086
Comment 18 Early Warning System Bot 2012-04-19 22:36:15 PDT
Comment on attachment 138041 [details]
Patch

Attachment 138041 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12479075
Comment 19 sam 2012-04-19 22:59:11 PDT
Created attachment 138043 [details]
Patch
Comment 20 Pavel Feldman 2012-04-20 02:28:38 PDT
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
Comment 21 sam 2012-04-20 06:57:01 PDT
Created attachment 138088 [details]
Patch
Comment 22 sam 2012-04-20 07:01:40 PDT
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
Comment 23 Pavel Feldman 2012-04-20 08:46:10 PDT
Committed r114748: <http://trac.webkit.org/changeset/114748>
Comment 24 Nikita Vasilyev 2012-05-07 13:38:46 PDT
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