702621 - GCLI needs fixes for the minor issues created by bug 692742

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Joe Walker [:jwalker]]

There's lots of work that's been done on top of this bug, and rather than go on a mad rebasingfest, I'd like to fix some issues in a separate bug.

The issues are:
- Remove the magic numbers in Popup.resizer()
- Move dynamic styles to dynamic classes in Popup.resizer()
- JavascriptField() should use a class rather than style.marginBottom
- field.js adds class names in old format
- The font assigned to .gcliterm-input-node, .gcliterm-complete-node needs to be theme specific
- Check that .gcli-af-required { padding-left } is RTL friendly

Comment 1

[Joe Walker [:jwalker]]

These are fixed in https://github.com/campd/gcli/pull/2
Will create official patch soon.

Comment 2

[Joe Walker [:jwalker]]

Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.

Comment 3

[Joe Walker [:jwalker]]

Attachment: upload 1

These are the changes that you asked for in bug 692742 - should be a simple review.

Comment 4

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=28134b5df88a

Comment 5

[Joe Walker [:jwalker]]

Failures in previous try were not down to this patch, but just in case:
https://tbpl.mozilla.org/?tree=Try&rev=587bd6d86427

Comment 6

[Dão Gottwald [:dao]]

(In reply to Joe Walker from comment #0)
> - Remove the magic numbers in Popup.resizer()

You didn't actually remove them.

Comment 7

[Joe Walker [:jwalker]]

(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to Joe Walker from comment #0)
> > - Remove the magic numbers in Popup.resizer()
> 
> You didn't actually remove them.

That's true, but I did explain them. I don't believe that it's the best thing to be spending time on.

Comment 8

[Dão Gottwald [:dao]]

It's bogus code, it doesn't help to document it. Those values are imaginary and may or may not match what they're supposed to match.

Comment 9

[Joe Walker [:jwalker]]

(In reply to Dão Gottwald [:dao] from comment #8)
> It's bogus code, it doesn't help to document it. Those values are imaginary
> and may or may not match what they're supposed to match.

Bug 705109

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 575857 [details] [diff] [review]
upload 1

> Display.prototype.resizer = function() {
>+  // Note on magic numbers: There are several magic numbers in this function.
>+  // We have 2 options - lots of complex dom math to derive the numbers or hard
>+  // code them. The former is *slightly* more resilient to refactoring (but
>+  // still breaks with dom structure changes), the latter is simpler, faster
>+  // and easier. For now we're hard coding, but will probably accept patches to
>+  // the latter technically better way of doing things.

replace with:

// FIXME bug 705109: There are several numbers hard-coded in this function.
// This is simpler than calculating them, but error-prone when the UI setup,
// the styling or display settings change.

>   this.element = dom.createElement(this.document, 'input');
>   this.element.type = 'text';
>-  this.element.className = 'gcli-field';
>+  this.element.classList.add('gcli-field');
> 
>   this.onInputChange = this.onInputChange.bind(this);
>   this.element.addEventListener('keyup', this.onInputChange, false);
>@@ -6686,7 +6689,7 @@ function SelectionField(type, options) {
>   this.items = [];
> 
>   this.element = dom.createElement(this.document, 'select');
>-  this.element.className = 'gcli-field';
>+  this.element.classList.add('gcli-field');
[...]

These changes are unnecessary and don't seem like improvements.

> /* From: $GCLI/mozilla/gcli/ui/gcliterm-gnomestripe.css */
> 
>+.gcliterm-input-node,
>+.gcliterm-complete-node {
>+  font: 12px "DejaVu Sans Mono", monospace;
>+}

Aren't we automatically using DejaVu Sans Mono when that's the monospace font the OS specifies? I.e. why hard-code it here?

Comment 11

[Joe Walker [:jwalker]]

(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 575857 [details] [diff] [review] [diff] [details] [review]
> upload 1
> > /* From: $GCLI/mozilla/gcli/ui/gcliterm-gnomestripe.css */
> > 
> >+.gcliterm-input-node,
> >+.gcliterm-complete-node {
> >+  font: 12px "DejaVu Sans Mono", monospace;
> >+}
> 
> Aren't we automatically using DejaVu Sans Mono when that's the monospace
> font the OS specifies? I.e. why hard-code it here?

gcliterm-complete-node - because that's not what the OS specifies. It's a div.
gcliterm-input-node is here symmetry with how it works on jsterm. I'm expecting that we will fix this in bug 704181.

Comment 12

[Joe Walker [:jwalker]]

Attachment: upload 2

Comment 13

[Dão Gottwald [:dao]]

(In reply to Joe Walker from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > Comment on attachment 575857 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > upload 1
> > > /* From: $GCLI/mozilla/gcli/ui/gcliterm-gnomestripe.css */
> > > 
> > >+.gcliterm-input-node,
> > >+.gcliterm-complete-node {
> > >+  font: 12px "DejaVu Sans Mono", monospace;
> > >+}
> > 
> > Aren't we automatically using DejaVu Sans Mono when that's the monospace
> > font the OS specifies? I.e. why hard-code it here?
> 
> gcliterm-complete-node - because that's not what the OS specifies. It's a
> div.

I think you misunderstood my question, as I don't see how this response answers it.
Let me try this again: Why did you write '"DejaVu Sans Mono", monospace' instead of just 'monospace'?

Comment 14

[Joe Walker [:jwalker]]

(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to Joe Walker from comment #11)
> > (In reply to Dão Gottwald [:dao] from comment #10)
...
> > > >+.gcliterm-input-node,
> > > >+.gcliterm-complete-node {
> > > >+  font: 12px "DejaVu Sans Mono", monospace;
> > > >+}
> > > 
> > > Aren't we automatically using DejaVu Sans Mono when that's the monospace
> > > font the OS specifies? I.e. why hard-code it here?
> > 
> > gcliterm-complete-node - because that's not what the OS specifies. It's a
> > div.
> 
> I think you misunderstood my question, as I don't see how this response
> answers it.
> Let me try this again: Why did you write '"DejaVu Sans Mono", monospace'
> instead of just 'monospace'?

Ah - I see. Specifically because that's what jsterm does.

I think this is the right thing to do. If the user has selected a different font for monospace, then I think we want to do the same as jsterm.

I'm adding Mihai to the CC list in case he has insight into the settings for jsterm.

Comment 15

[Mihai Sucan [:msucan]]

If I recall correctly the DejaVu Sans Mono font was added because on some systems the default monospace font in Firefox was not a reasonable choice - Courier New and the likes.

This is not good solution, as Dão points out. We should make sure that monospace defaults are sensible on supported platforms.

Joe: Scratchpad uses monospace by default - afaik nobody complained about it. We should probably no longer bother to have DejaVu Sans Mono hard coded in the Web Console.

Thoughts?

Comment 16

[Joe Walker [:jwalker]]

(In reply to Mihai Sucan [:msucan] from comment #15)
> If I recall correctly the DejaVu Sans Mono font was added because on some
> systems the default monospace font in Firefox was not a reasonable choice -
> Courier New and the likes.
> 
> This is not good solution, as Dão points out. We should make sure that
> monospace defaults are sensible on supported platforms.
> 
> Joe: Scratchpad uses monospace by default - afaik nobody complained about
> it. We should probably no longer bother to have DejaVu Sans Mono hard coded
> in the Web Console.
> 
> Thoughts?

I raised bug 706047.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 577218 [details] [diff] [review]
upload 2

> Display.prototype.resizer = function() {
>+  // Bug 705109: There are several numbers hard-coded in this function.

Add "FIXME" here, as suggested earlier. This is a standard annotation in Mozilla code.

Comment 18

[Joe Walker [:jwalker]]

(In reply to Dão Gottwald [:dao] from comment #17)
> Comment on attachment 577218 [details] [diff] [review] [diff] [details] [review]
> upload 2
> 
> > Display.prototype.resizer = function() {
> >+  // Bug 705109: There are several numbers hard-coded in this function.
> 
> Add "FIXME" here, as suggested earlier. This is a standard annotation in
> Mozilla code.

GCLI has used Bug for this since before it's inclusion in Firefox. It's documented here: https://github.com/mozilla/gcli/blob/master/docs/index.md

Thanks.

Comment 19

[Dão Gottwald [:dao]]

Any reason why GCLI shouldn't adopt the Mozilla standard?

Comment 20

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=cf3ed4316481

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/71054aef1a3a