786803 - GCLI help command panel is too big

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

Attachment: screenshot

The GCLI help command output is too tall and too narrow.

Comment 1

[Joe Walker [:jwalker]]

Mike - is this fixed by our resize fix?

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I have never seen it look like that so I doubt it but it could be.

Comment 3

[Joe Walker [:jwalker]]

Mihai - do you know how you got it to look like that?

Comment 4

[Mihai Sucan [:msucan]]

Joe: I just open Firefox, enable dev toolbar and type "help". I use Ubuntu 12.04, all up-to-date.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I was almost able to reproduce this by making the browser really narrow before typing help. Seems like it happens for Mihai without a narrow browser :o/

We could just set min-width in _resize() using Math.max(x, min_width). It would be good to know the cause - I suspect that _input is not very narrow as we try to match it's size.

This could possibly be fixed by the addon install command (bug 771066) where I switched from _input.clientWidth to _input.getBoundingClientRect().width.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The patch for bug 785134 allows us to reliably reproduce this, at least on Linux.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch

Fairly simple patch ... not yet tested on Windows or OSX.

Comment 8

[Joe Walker [:jwalker]]

Comment on attachment 666987 [details] [diff] [review]
Patch

Review of attachment 666987 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those fixes.

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +723,5 @@
>    }
>  
> +  let width = Math.min(
> +    this._panel.ownerDocument.documentElement.clientWidth,
> +    this.document.documentElement.scrollWidth + 15

Magic numbers. I know I was bad here, but some people say you should never have magic numbers at all. I can see the downside, but I also get that sometimes the calculations to work out the magic number as just as fragile as the magic number itself.

Assuming 15 is hard to calculate, could you at least document what it represents?

@@ +731,5 @@
> +    this._panel.ownerDocument.documentElement.clientHeight,
> +    this.document.documentElement.scrollHeight
> +  );
> +
> +  this._frame.style.minWidth = width + 2 + "px";

Insert magic number comment here too.
Also, nit, could you say: (width + 2) + "px";
Just to make it clear of the order that the addition should be done in.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Reimplemented

When making the requested changes I found the panel placement still to be unreliable.

This new patch addresses this issue basically by manually positioning and sizing panels.

Comment 10

[Joe Walker [:jwalker]]

Comment on attachment 667555 [details] [diff] [review]
Reimplemented

Review of attachment 667555 [details] [diff] [review]:
-----------------------------------------------------------------

You don't have to fix this unless you strongly agree, or are editing the file for another reason.
On the other hand I would be interested to know if you agree that Object.defineProperty is a better solution here.

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +678,5 @@
> + * Determine the scrollbar width in the current document.
> + *
> + * @private
> + */
> +OutputPanel.prototype.__defineGetter__("scrollbarWidth", function() {

I think that

Object.defineProperty(OutputPanel.prototype, 'scrollbarWidth', {
  get: function() {
...
  },
  enumerable: true
};

Is probably a more trendy way to spell __defineGetter__ these days?

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Now uses Object.defineProperty

(In reply to Joe Walker [:jwalker] from comment #10)
> Comment on attachment 667555 [details] [diff] [review]
> Reimplemented
> 
> Review of attachment 667555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You don't have to fix this unless you strongly agree, or are editing the
> file for another reason.
> On the other hand I would be interested to know if you agree that
> Object.defineProperty is a better solution here.
> 
> ::: browser/devtools/shared/DeveloperToolbar.jsm
> @@ +678,5 @@
> > + * Determine the scrollbar width in the current document.
> > + *
> > + * @private
> > + */
> > +OutputPanel.prototype.__defineGetter__("scrollbarWidth", function() {
> 
> I think that
> 
> Object.defineProperty(OutputPanel.prototype, 'scrollbarWidth', {
>   get: function() {
> ...
>   },
>   enumerable: true
> };
> 
> Is probably a more trendy way to spell __defineGetter__ these days?

Yes, all of the cool kids are using it. Typing __defineGetter__ is an old habit that I do without thinking.

Anyhow, we shouldn't be using deprecated stuff so I have updated it.

Comment 12

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/003656caed2a

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/003656caed2a