Closed
Bug 1265759
Opened 9 years ago
Closed 8 years ago
Create an HTML replacement for inspector Search Box
Categories
(DevTools :: Framework, enhancement, P1)
DevTools
Framework
Tracking
(firefox51 verified)
Tracking | Status | |
---|---|---|
firefox51 | --- | verified |
People
(Reporter: Honza, Assigned: gasolin)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [devtools-html])
Attachments
(2 files)
We need HTML version of the Search Box Honza
Reporter | ||
Updated•9 years ago
|
Comment 1•9 years ago
|
||
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.
Reporter | ||
Comment 2•9 years ago
|
||
(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
Updated•9 years ago
|
Flags: qe-verify+
Priority: -- → P2
QA Contact: alexandra.lucinet
Comment 3•8 years ago
|
||
(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•8 years ago
|
||
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
Updated•8 years ago
|
Priority: P2 → P1
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → odvarko
Updated•8 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 50.1
Updated•8 years ago
|
Iteration: 50.1 → 50.2
Updated•8 years ago
|
Assignee: odvarko → nobody
Status: ASSIGNED → NEW
Iteration: 50.2 → ---
Priority: P1 → P2
Assignee | ||
Comment 5•8 years ago
|
||
> * 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.
Assignee | ||
Comment 7•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Updated•8 years ago
|
Iteration: --- → 50.4 - Aug 1
Priority: P2 → P1
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/67346/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67346/
Assignee | ||
Comment 9•8 years ago
|
||
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/
Assignee | ||
Comment 10•8 years ago
|
||
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
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 11•8 years ago
|
||
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/
Assignee | ||
Comment 12•8 years ago
|
||
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/
Assignee | ||
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
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
Assignee | ||
Comment 15•8 years ago
|
||
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/
Assignee | ||
Comment 16•8 years ago
|
||
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
Assignee | ||
Comment 17•8 years ago
|
||
this issue will focused on inspector's searchbox, will create separate bugs for canvasdebugger, debugger's searchbox
Summary: Create an HTML replacement for Search Box → Create an HTML replacement for inspector Search Box
Updated•8 years ago
|
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
Assignee | ||
Comment 18•8 years ago
|
||
> activeElement.ownerDocument.documentElement
seems html element don't need any extra treatment to get bindingFocus
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 19•8 years ago
|
||
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/
Attachment #8775010 -
Attachment description: Bug 1265759 - Create an HTML replacement for Search Box; → Bug 1265759 - Create an HTML replacement for inspector Search Box;
Attachment #8775010 -
Flags: review?(bgrinstead)
Comment 20•8 years ago
|
||
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
Attachment #8775010 -
Flags: review?(bgrinstead)
Updated•8 years ago
|
Attachment #8775010 -
Flags: review?(gl)
Comment 21•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8775010 -
Flags: review?(gl) → review-
Comment 22•8 years ago
|
||
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.
Updated•8 years ago
|
QA Contact: adalucin → cristian.comorasu
Assignee | ||
Comment 23•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 25•8 years ago
|
||
(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•8 years ago
|
||
mozreview-review |
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.
Attachment #8775010 -
Flags: review?(gl) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•8 years ago
|
||
issue addressed, treeherder green, thanks!
Keywords: checkin-needed
Comment 30•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 32•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3269dd1a824d1b42cb021d1fb6858885179940b0 Bug 1265759 - Create an HTML replacement for inspector Search Box;r=gl
Updated•8 years ago
|
Keywords: checkin-needed
Comment 33•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3269dd1a824d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment 34•8 years ago
|
||
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!
Updated•8 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•