1265759 - Create an HTML replacement for inspector Search Box

Status: VERIFIED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

We need HTML version of the Search Box

Honza

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Just a note the search box implemented in the rule/computed view is in HTML and matches the style of the existing search boxes you will find in other panels. We should try to extract this into its own widget.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #1)
> Just a note the search box implemented in the rule/computed view is in HTML
> and matches the style of the existing search boxes you will find in other
> panels. We should try to extract this into its own widget.
Yes

Some more comments:

* There are many searchboxes in the UI, but most of them already use HTML
* Search boxes use shared devtools-searchinput' & 'devtools-searchinput' classes
* We focus on the Inspector panel and there are three search boxes:
  - Search HTML (the main panel)  -> xul:textbox wrapping html:input
  - Filter Styles (Rules View)    -> html:input
  - Filter Styles (Computed View) -> html:input
* We'll have troubles with context menu. We should way for the platform support.
* Otherwise expected API are like as follows:
  - handling 'input', 'keypress' events
  - accessing the current text value
  - customizing style (no match)
  - focus
* The main Inspector panel uses existing InspectorSearch module with built-in auto-completion. The new react component could be used within this object. API expectations are the same as in the previous point.

So, I agree that this is also an adept for React component. Perhaps in case of time press, we might want to reevaluate and consider in-place HTML replacement.

Honza

Comment 3

[Sebastian Zartner [:sebo]]

(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> * There are many searchboxes in the UI, but most of them already use HTML

Unfortunately that's only the story. The Inspector, Console, Debugger, Network, and Storage panel all use xul:textbox and most of them wrap an html:input.

> * Otherwise expected API are like as follows:
>   - handling 'input', 'keypress' events
>   - accessing the current text value
>   - customizing style (no match)
>   - focus

Also:
- clearing the field (via a button)
- search suggestions

Sebastian

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

If anyone is ever working on this, please consult me since I am working on the new refresh of the inspector search which will remove clearing the field for instance. See https://bugzilla.mozilla.org/show_bug.cgi?id=1249893

Comment 5

[[e-mail]]

> * We focus on the Inspector panel and there are three search boxes:
>   - Search HTML (the main panel)  -> xul:textbox wrapping html:input
>   - Filter Styles (Rules View)    -> html:input
>   - Filter Styles (Computed View) -> html:input

The Rules View/Computed View are not React component yet (meta bug 1288400). To make some progress, we might better focus on convert Search HTML (the main panel) to html at this issue.

Comment 6

[[e-mail]]

Attachment: before/after convert to html

WIP: convert textbox to html:input make the cross sign missed

Comment 7

[[e-mail]]

WIP, converted, several test cases fail.

The plan is only convert the inspector main search box from xul textbox to html:input 

So this work wont depends on bug 1249893, which will only change the sidebar panel style

Comment 8

[[e-mail]]

Attachment: Bug 1265759 - Create an HTML replacement for inspector Search Box;

Review commit: https://reviewboard.mozilla.org/r/67346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67346/

Comment 9

[[e-mail]]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67346/diff/1-2/

Comment 10

[[e-mail]]

WIP: client/inspector/test cases fixed

I found the split window focus test case[1] failed because of toolbox cannot get the right focus element.

When select a html element(focus), the _lastFocusedElement/originalTarget store an empty object[2], therefore in this case, a converted searchbox are not focused after close the split window.


bgrin, is this a known issue or any suggestions?


[1] https://github.com/mozilla/gecko-dev/blob/master/devtools/client/webconsole/test/browser_webconsole_split_focus.js#L46
[2] https://github.com/mozilla/gecko-dev/blob/master/devtools/client/framework/toolbox.js#L1448

Comment 11

[[e-mail]]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67346/diff/2-3/

Comment 12

[[e-mail]]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67346/diff/3-4/

Comment 13

[[e-mail]]

for browser_inspector_pane-toggle-04.js failed on linux,
because of the new icon (eyedropper) is added in the row, the narrow width window make .sidebar-toggle not selectable on linux.

Comment 14

[[e-mail]]

Another reason for browser_inspector_pane-toggle-04.js fail is the WIP html input field does not adjust its width. should apply `width: 100%;` like rule view search box

Comment 15

[[e-mail]]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67346/diff/4-5/

Comment 16

[[e-mail]]

for webconsole/test/browser_webconsole_split_focus.js 

the FAIL message is
> Search box is focused - Got null, expected [object HTMLInputElement]


According to the comment[1], it seems related to the difference of xul/html element.
I changed the getBindingParent sentence from

> activeElement = activeElement.ownerDocument.getBindingParent(activeElement);

to

> activeElement.ownerDocument.documentElement

then got the following error:

>  Search box is focused - Got [object XULElement], expected [object HTMLInputElement]

Seems we need different way to match for active element in html


[1] https://github.com/mozilla/gecko-dev/blob/master/devtools/client/webconsole/test/browser_webconsole_split_focus.js#L29

Comment 17

[[e-mail]]

this issue will focused on inspector's searchbox, will create separate bugs for canvasdebugger, debugger's searchbox

Comment 18

[[e-mail]]

> activeElement.ownerDocument.documentElement

seems html element don't need any extra treatment to get bindingFocus

Comment 19

[[e-mail]]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67346/diff/5-6/

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

https://reviewboard.mozilla.org/r/67346/#review66146

Sorry for the delay!  I think Gabe would be the best reviewer for this, so redirecting

Comment 21

[Tim Nguyen :ntim]

https://reviewboard.mozilla.org/r/67346/#review66414

::: devtools/client/inspector/inspector-search.js:7
(Diff revision 6)
>  /* eslint-disable mozilla/reject-some-requires */
> -const {Ci} = require("chrome");
> -/* eslint-enable mozilla/reject-some-requires */

Can you remove the eslint-disable comment as well? It used to ignore the eslint error on require("chrome"), but now that it's gone, it should be removed.

Comment 22

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

https://reviewboard.mozilla.org/r/67346/#review66730

::: devtools/client/inspector/inspector-panel.js:360
(Diff revision 6)
>     * Hooks the searchbar to show result and auto completion suggestions.
>     */
>    setupSearchBox: function () {
>      this.searchBox = this.panelDoc.getElementById("inspector-searchbox");
>      this.searchResultsLabel = this.panelDoc.getElementById("inspector-searchlabel");
> +    this.searchClearButton = this.panelDoc.getElementById("inspector-searchinput-clear");

Let's put this.searchClearButton assignment after this.searchBox, so, that we group the variables relevant to the search box closer together.

::: devtools/client/inspector/inspector-search.js:7
(Diff revision 6)
>  /* eslint-disable mozilla/reject-some-requires */
> -const {Ci} = require("chrome");
> -/* eslint-enable mozilla/reject-some-requires */

We definitely want to remove this line. The point of this was to disable one of the eslint rules and re-enable immediately afterwards. By leaving this in, we just keep the eslint rule disabled.

::: devtools/client/inspector/inspector-search.js:25
(Diff revision 6)
>   *
>   * @param {InspectorPanel} inspector The InspectorPanel whose `walker` attribute
>   * should be used for document traversal.
>   * @param {DOMNode} input The input element to which the panel will be attached
>   * and from where search input will be taken.
> + * @param {DOMNode} tap clear sign in the input element will clean the content.

This should be:
@param {DOMNode} clearBtn The clear button in the input field that will clear the input value.

::: devtools/client/inspector/inspector.xul:43
(Diff revision 6)
>          <html:button id="inspector-element-add-button"
>            title="&inspectorAddNode.label;"
>            class="devtools-button" />
>          <html:div class="devtools-toolbar-spacer" />
> +        <html:div id="inspector-search" class="devtools-searchbox">
> -        <html:span id="inspector-searchlabel" />
> +          <html:span id="inspector-searchlabel" />

#inspector-searchlabel is not actually part of the searchbox. The search label displays something like "1 of 19" which would otherwise indicate the current search index in the total number of search matches.

::: devtools/client/themes/inspector.css:30
(Diff revision 6)
>    display: inline-block;
>  }
>  
>  #inspector-searchlabel {
>    overflow: hidden;
> +  margin-inline-end: 2px;

I would prefer to use the more standard margin-right: 2px over margin-line-end

::: devtools/client/themes/inspector.css:40
(Diff revision 6)
> +}
> +
> +/* TODO: bug 1265759: should apply to .devtools-searchinput once all searchbox
> +   is converted to html*/
> +#inspector-searchbox {
> +  -moz-box-flex: 1;

We should not use -moz prefix whenever we can. We should be able to use flex: 1 here.

Comment 23

[[e-mail]]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

https://reviewboard.mozilla.org/r/67346/#review66730

> I would prefer to use the more standard margin-right: 2px over margin-line-end

`margin-inline-end` is the new standard to compatible with rtl mode. https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-end Many of devtools modules use that too. Is there a cross browser compatibility concern on inspector that we should not use that?

Comment 24

[[e-mail]]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67346/diff/6-7/

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Fred Lin [:gasolin] from comment #23)
> Comment on attachment 8775010 [details]
> Bug 1265759 - Create an HTML replacement for inspector Search Box;
> 
> https://reviewboard.mozilla.org/r/67346/#review66730
> 
> > I would prefer to use the more standard margin-right: 2px over margin-line-end
> 
> `margin-inline-end` is the new standard to compatible with rtl mode.
> https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-end Many of
> devtools modules use that too. Is there a cross browser compatibility
> concern on inspector that we should not use that?

I was not aware of the new standard. It would be fine to keep it as margin-inline-end.

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

https://reviewboard.mozilla.org/r/67346/#review67244

::: devtools/client/inspector/inspector-panel.js:216
(Diff revisions 6 - 7)
>        // We show the warning only when the inspector
>        // is selected.
>        this.updateDebuggerPausedWarning = () => {
>          let notificationBox = this._toolbox.getNotificationBox();
> -        let notification = notificationBox.getNotificationWithValue("inspector-script-paused");
> +        let notification =
> +            notificationBox.getNotificationWithValue("inspector-script-paused");

Doing a quick search of our codebase for regexp(=$) shows that we typically only do 1 tab (2spaces) indentation. So, please switch to just 1 tab indentation for this variable assignment.

::: devtools/client/inspector/inspector-panel.js:1520
(Diff revisions 6 - 7)
>    },
>  
>    /**
> -   * Copy the content of a longString (via a promise resolving a LongStringActor) to the clipboard
> -   * @param  {Promise} longStringActorPromise promise expected to resolve a LongStringActor instance
> -   * @return {Promise} promise resolving (with no argument) when the string is sent to the clipboard
> +   * Copy the content of a longString (via a promise resolving a
> +   * LongStringActor) to the clipboard
> +   * @param  {Promise} longStringActorPromise promise expected to

If we gonna format the JSDocs for these @param, I would prefer if we follow the convention in rules.js where we list the parameter description on a new line.

An example would be:
@param {Promise} longStringActorPromise
       promise expected to resolve LongStringActor instance

::: devtools/client/inspector/inspector-panel.js:1533
(Diff revisions 6 - 7)
>      }).catch(e => console.error(e));
>    },
>  
>    /**
>     * Retrieve the content of a longString (via a promise resolving a LongStringActor)
> -   * @param  {Promise} longStringActorPromise promise expected to resolve a LongStringActor instance
> +   * @param  {Promise} longStringActorPromise promise expected to

Same as above.

::: devtools/client/inspector/inspector.xul:45
(Diff revisions 6 - 7)
>          <html:div class="devtools-toolbar-spacer" />
> -        <html:div id="inspector-search" class="devtools-searchbox">
> -          <html:span id="inspector-searchlabel" />
> +        <html:span id="inspector-searchlabel" />
> +        <html:div id="inspector-search" class="devtools-searchbox">
>            <html:input id="inspector-searchbox"
>              class="devtools-searchinput"

It seems we generally line up the align the attributes. So, please line up these attributes (class, type, placeholder) with the id.

Comment 27

[[e-mail]]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67346/diff/7-8/

Comment 28

[[e-mail]]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67346/diff/8-9/

Comment 29

[[e-mail]]

issue addressed, treeherder green, 

thanks!

Comment 30

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

https://reviewboard.mozilla.org/r/67346/#review67914

::: devtools/client/inspector/inspector.xul:45
(Diff revision 9)
>            class="devtools-button" />
>          <html:div class="devtools-toolbar-spacer" />
>          <html:span id="inspector-searchlabel" />
> -        <textbox id="inspector-searchbox"
> +        <html:div id="inspector-search" class="devtools-searchbox">
> +          <html:input id="inspector-searchbox" class="devtools-searchinput"
> -          type="search"
> +            type="search"

Please indent these attributes to align with the id.

Comment 31

[[e-mail]]

Comment on attachment 8775010 [details]
Bug 1265759 - Create an HTML replacement for inspector Search Box;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67346/diff/9-10/

Comment 32

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/3269dd1a824d1b42cb021d1fb6858885179940b0
Bug 1265759 - Create an HTML replacement for inspector Search Box;r=gl

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3269dd1a824d

Comment 34

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue  using Fx 51.0a1, build ID:  20160419030312, on Windows 10 x64.
I can confirm that using the latest nightly the behavior is close to the intended one, however I will not mark it as verified until the dependencies are fixed. 

Cheers!