1304328 - Stop using XUL in webconsole/jsterm.js

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Julian Descottes [:jdescottes]]

Webconsole's jsterm is still using XUL in a few places:
- relies on two XUL textbox elements from webconsole.xul
  - jsterm-input-node: main input element
  - jsterm-complete-node: used to display the selected autocomplete suggestion in a light font-color
- includes/depends on the XUL-based variable view
- uses XUL clipboard helper

For devtools-html we need to create a XUL-free version of jsterm.

Comment 1

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

Attachment: Bug 1304328 - Stop using XUL in webconsole/jsterm.js;

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

Comment 2

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

Comment on attachment 8911689 [details]
Bug 1304328 - Stop using XUL in webconsole/jsterm.js;

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

Comment 3

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

Try push is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fc5bdee60589c14f2543170488587e0fb53d20d

(I am yet looking at osx layout issue)

Honza

Comment 4

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

Comment on attachment 8911689 [details]
Bug 1304328 - Stop using XUL in webconsole/jsterm.js;

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

Comment 5

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

I couldn't find a 'flex' way to adjust overall size of the app-wrapper container when the input-node/jsterm changes it's height (it happens when the user inserts multi-line input). It probably needs broader refactoring of the overall panel markup. So, it's done in JS at the same place where the height of the input-node is calculated.

Honza

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8911689 [details]
Bug 1304328 - Stop using XUL in webconsole/jsterm.js;

https://reviewboard.mozilla.org/r/183078/#review188424

::: devtools/client/webconsole/webconsole.xhtml:28
(Diff revision 3)
>      <div id="app-wrapper" class="theme-body">
>        <div id="output-container" role="document" aria-live="polite"/>
>        <div id="jsterm-wrapper">
> -        <xul:notificationbox id="webconsole-notificationbox">
> +        <div id="webconsole-notificationbox">
>            <div class="jsterm-input-container" style="direction:ltr">
> -            <xul:stack class="jsterm-stack-node" flex="1">
> +            <div class="jsterm-stack-node" flex="1">

`flex` attribute should be removable

::: devtools/client/webconsole/webconsole.xhtml:30
(Diff revision 3)
>        <div id="jsterm-wrapper">
> -        <xul:notificationbox id="webconsole-notificationbox">
> +        <div id="webconsole-notificationbox">
>            <div class="jsterm-input-container" style="direction:ltr">
> -            <xul:stack class="jsterm-stack-node" flex="1">
> -              <xul:textbox class="jsterm-complete-node devtools-monospace"
> +            <div class="jsterm-stack-node" flex="1">
> +              <textarea class="jsterm-complete-node devtools-monospace"
>                         multiline="true" rows="1" tabindex="-1"/>

I think the `multiline` attribute can be removed now that these are textareas

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: Bug 1304328 - Stop using XUL in webconsole/jsterm.js

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

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8911905 [details]
Bug 1304328 - Stop using XUL in webconsole/jsterm.js

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

Comment 9

[Brian Grinstead [:bgrins]]

Honza, I've uploaded a patch that accomplishes the resizing without extra JS (other than the code needed to properly select the input field). Can you check and make sure it is working for the cases you were testing and then re-request review from Nicolas if so?

I basically pulled the jsterm-complete-node out as an absolutely positioned element but kept the input node with static positioning. This let us get rid of absolute positioning on the parent and means we don't need to the calc() and js workarounds to manually resize.  I also reorganized the css a bit to keep the new/old styles more separate and to make the diff easier to follow (even though it meant a little bit of duplication with focus rules for instance).

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: positioning-left-side-xul-vs-html.png

Something I noticed when working on this is that the left-alignment of the autocomplete panel seems improved in the new version

For some reason in the XUL version (Browser Console) if you open the autocomplete panel near the left of the window it gets pinned on the window's edge instead of by the "." (this stops happening if the popup opens more centrally on the screen). In the HTML version the popup always opens alongside the "."

Comment 11

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

Comment on attachment 8911689 [details]
Bug 1304328 - Stop using XUL in webconsole/jsterm.js;

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

Comment 12

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

Tested on Win and LGTM

Honza

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8911689 [details]
Bug 1304328 - Stop using XUL in webconsole/jsterm.js;

https://reviewboard.mozilla.org/r/183078/#review188742

let's ship with the change on the toolbar height.
Myabe we can have a follow up for cases where the toolbar is much taller (hidden message label and/or filter bar displayed)

::: devtools/client/themes/webconsole.css:413
(Diff revision 4)
>  }
>  
> +.jsterm-input-node {
> +  /* Always allow scrolling on input - it auto expands in js by setting height,
> +     but don't want it to get bigger than the window. 24px = toolbar height. */
> +  max-height: calc(90vh - 24px);

the toolbar is 29px tall now (http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/client/themes/webconsole.css#782), but we can use --primary-toolbar-height to get it.
It can be taller though with the hidden messages label, if the toolbars wrap (or if we show the filters bar).
I think this is fine for now though.

Comment 14

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

Thanks for the review Nicolas!

Btw. can you please file the toolbar-height follow up if needed?
(I think you have more info about this).

Honza

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e98b59ba190d
Stop using XUL in webconsole/jsterm.js; r=nchevobbe

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for linting failure at devtools/client/webconsole/jsterm.js:1013: More than 1 blank line not allowed:

https://hg.mozilla.org/integration/autoland/rev/fa45427baa38ad8b33b483ac4e38013aea37e1e6

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e98b59ba190d05b97317a4ebb2050ea47b0d5393&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133528409&repo=autoland
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/webconsole/jsterm.js:1013:1 | More than 1 blank line not allowed. (no-multiple-empty-lines)

Comment 17

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

Comment on attachment 8911689 [details]
Bug 1304328 - Stop using XUL in webconsole/jsterm.js;

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

Comment 18

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

Fixed linting issue and re-landing.

Honza

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/367484c62750
Stop using XUL in webconsole/jsterm.js; r=nchevobbe

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/367484c62750