Bug 84235 – Web Inspector: Does not have search navigation button for going through matches in either direction (prev, next)

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[sam]

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.

[sam]

I am looking into it and will upload the patch soon!

[Pavel Feldman]

(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?

[sam]

(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.

[Pavel Feldman]

(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.

[sam]

Created attachment 137689 [details]
Patch

[Early Warning System Bot]

Comment on attachment 137689 [details]
Patch

Attachment 137689 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12426320

[Early Warning System Bot]

Comment on attachment 137689 [details]
Patch

Attachment 137689 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12424548

[Pavel Feldman]

Please provide a screenshot taken with the patch applied.

[sam]

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.

[sam]

Created attachment 137845 [details]
Patch

[Pavel Feldman]

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.

[sam]

Created attachment 137908 [details]
Patch

[Pavel Feldman]

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.

[sam]

Created attachment 138041 [details]
Patch

[sam]

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?

[sam]

(In reply to comment #14)
> Created an attachment (id=138041) [details]
> Patch

Patch, incorporated with review comments, uploaded.

[Early Warning System Bot]

Comment on attachment 138041 [details]
Patch

Attachment 138041 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12476086

[Early Warning System Bot]

Comment on attachment 138041 [details]
Patch

Attachment 138041 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12479075

[sam]

Created attachment 138043 [details]
Patch

[Pavel Feldman]

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

[sam]

Created attachment 138088 [details]
Patch

[sam]

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

[Pavel Feldman]

Committed r114748: <http://trac.webkit.org/changeset/114748>

[Nikita Vasilyev]

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