Last Comment Bug 692742 - GCLI popup dialogs sometimes have scrollbars
: GCLI popup dialogs sometimes have scrollbars
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 11
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
Depends on:
Blocks: GCLI-SHIP 699411 702621
  Show dependency treegraph
 
Reported: 2011-10-07 02:43 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2011-11-21 06:32 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
upload 1 (23.14 KB, patch)
2011-11-03 11:41 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Review
upload 2 (22.52 KB, patch)
2011-11-03 15:10 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Review
upload 3 (24.02 KB, patch)
2011-11-04 05:18 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Review
upload 4 (52.17 KB, patch)
2011-11-09 05:23 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
dcamp: review+
paul: feedback+
Details | Diff | Review
upload 5 (52.08 KB, patch)
2011-11-17 06:27 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
dao+bmo: review+
Details | Diff | Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-07 02:43:17 PDT

    
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-14 06:14:31 PDT
Especially JS.
Perhaps the dialog should have whatever width it wants, and no fixed height - just a max-height.
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-03 11:41:57 PDT
Created attachment 571722 [details] [diff] [review]
upload 1
Comment 3 Dão Gottwald [:dao] 2011-11-03 12:03:30 PDT
What's the point of those -webkit-* properties in that patch?
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-03 15:09:22 PDT
(In reply to Dão Gottwald [:dao] from comment #3)
> What's the point of those -webkit-* properties in that patch?

GCLI works on the web so those properties are needed to support that use-case. I have a system that is supposed to strip them out before submitting them, however there was a bug in the regex, so some slipped through, and despite checking the file over, I didn't spot them.

Regex fixed. Will upload again.
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-03 15:10:11 PDT
Created attachment 571796 [details] [diff] [review]
upload 2

No more -webkit-
Comment 6 Dão Gottwald [:dao] 2011-11-03 20:32:13 PDT
Comment on attachment 571796 [details] [diff] [review]
upload 2

>+.gcliterm-input-node {
>+  padding-top: 2px;
>+  padding-bottom: 0;
>+  padding-start: 16px;
>+  -moz-padding-start: 16px;
>+  padding-end: 0;
>+  -moz-padding-end: 0;
> }

padding-start and padding-end aren't proposed to become standard CSS, so you can omit them even in the unstripped source.

>+.gcliCliEle {
>+  width: 100%;
>+  box-sizing: border-box;
>+  -moz-box-sizing: border-box;
>+  display: -moz-box;
>+  -moz-box-flex: 1;
>+}

Can you teach your script to strip box-sizing as well, as long as Gecko doesn't support it?

I don't think width:100% makes sense in combination with display:-moz-box.

Am I supposed to review the jsm changes?
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-04 05:17:27 PDT
(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 571796 [details] [diff] [review] [diff] [details] [review]
> upload 2

> >+.gcliCliEle {
> >+  width: 100%;
> >+  box-sizing: border-box;
> >+  -moz-box-sizing: border-box;
> >+  display: -moz-box;
> >+  -moz-box-flex: 1;
> >+}
> 
> Can you teach your script to strip box-sizing as well, as long as Gecko
> doesn't support it?

Gecko doesn't support it, however it's a standard so it seems likely that it will, and that the -moz- version will in future become the 'wrong' way to do things. Leaving them both in felt like it was more future proof.

However read on

> I don't think width:100% makes sense in combination with display:-moz-box.

The problem is that flexbox is too buggy to work on the web (even in Firefox), so we have a different layout system there. You're looking at a way of addressing those 2 cases.

What I've done is to separate the classes so there is a different one for the web and for firefox.


> >+.gcliterm-input-node {
> >+  padding-top: 2px;
> >+  padding-bottom: 0;
> >+  padding-start: 16px;
> >+  -moz-padding-start: 16px;
> >+  padding-end: 0;
> >+  -moz-padding-end: 0;
> > }
> 
> padding-start and padding-end aren't proposed to become standard CSS, so you
> can omit them even in the unstripped source.

True, the logic was that they're in webkit too, and it seems like such an obvious direction to take that it felt like the code is more likely to be better in the future than worse. However I'm not bothered, so I'll take them out.

> Am I supposed to review the jsm changes?

Not at all. Dave reviews those.
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-04 05:18:04 PDT
Created attachment 571933 [details] [diff] [review]
upload 3
Comment 9 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-09 05:23:43 PST
Created attachment 573168 [details] [diff] [review]
upload 4
Comment 10 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-09 07:00:32 PST
See https://tbpl.mozilla.org/?tree=Try&rev=2a2808e08d26
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-09 07:16:21 PST
There's quite a bit of churn to the CSS in this bug and bug 699411. See bug 699411 comment 4 for more details.

Also see https://github.com/mozilla/gcli/pull/43 (just the entries that start 'Bug 692742 (noscroll):'
Comment 12 Paul Rouget [:paul] 2011-11-15 05:49:52 PST
Comment on attachment 573168 [details] [diff] [review]
upload 4

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

Beside some hard-coded values in the JS code, it looks good.

Also, two general comments (can be fixed in some followup bugs):
a) if the system theme is dark, some hard-coded colors make the console not usable (the main text input is white on white). Don't assume that the background of the console is white, or that the text is black;
b) on RTL, the prototype of the command for a specific argument should be left-aligned (write "console", see "console" and "console clear" being right aligned when they should be left aligned).

f+ if the hard-coded values are moved the CSS files, or justified as JS comments.

::: browser/devtools/webconsole/gcli.jsm
@@ +5108,5 @@
> + * Called on chrome window resize, or on divider slide
> + */
> +Popup.prototype.resizer = function() {
> +  var parentRect = this.consoleWrap.getBoundingClientRect();
> +  var parentHeight = parentRect.bottom - parentRect.top - 64;

Why 64?

@@ +5110,5 @@
> +Popup.prototype.resizer = function() {
> +  var parentRect = this.consoleWrap.getBoundingClientRect();
> +  var parentHeight = parentRect.bottom - parentRect.top - 64;
> +
> +  if (parentHeight < 100) {

Why 100?

@@ +5140,5 @@
> +    else {
> +      this.argFetcher.setMaxHeight(parentHeight);
> +
> +      this.hintElement.style.overflowY = null;
> +      this.hintElement.style.borderBottomColor = 'white';

I don't think these borderBottomColors (threedshadow and white) should be here. That should be in the browser/theme/. If you want to change them, you should change the className.

Same for overflowY (in a CSS, but not in browser/theme/).

@@ +6473,3 @@
>    this.input.type = 'text';
>    this.input.addEventListener('keyup', this.onInputChange, false);
> +  this.input.style.marginBottom = '0';

Does it have to be hard-coded in the JS code?

::: browser/themes/gnomestripe/browser/devtools/gcli.css
@@ -44,3 +47,3 @@
> >    vertical-align: middle;
> >    background-color: transparent;
> >    font: 12px Consolas, "Lucida Console", monospace;

Can you use a relative font-size? And can you use the system monospace font?

@@ +191,5 @@
>  
>  .gcliRequired {
> +  font-size: 90%;
> +  color: #f66;
> +  padding-left: 5px;

Is that RTL friendly?
Comment 13 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-15 09:27:29 PST
There's quite a bit of stuff that depends on this bug, so unless we find something that's broken rather than just 'could be better' I'd like to fix it in bug 702621.

(In reply to Paul Rouget [:paul] from comment #12)
> Comment on attachment 573168 [details] [diff] [review] [diff] [details] [review]
> upload 4
> 
> Review of attachment 573168 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Beside some hard-coded values in the JS code, it looks good.
> 
> Also, two general comments (can be fixed in some followup bugs):
> a) if the system theme is dark, some hard-coded colors make the console not
> usable (the main text input is white on white). Don't assume that the
> background of the console is white, or that the text is black;
> b) on RTL, the prototype of the command for a specific argument should be
> left-aligned (write "console", see "console" and "console clear" being right
> aligned when they should be left aligned).
> 
> f+ if the hard-coded values are moved the CSS files, or justified as JS
> comments.
> 
> ::: browser/devtools/webconsole/gcli.jsm
> @@ +5108,5 @@
> > + * Called on chrome window resize, or on divider slide
> > + */
> > +Popup.prototype.resizer = function() {
> > +  var parentRect = this.consoleWrap.getBoundingClientRect();
> > +  var parentHeight = parentRect.bottom - parentRect.top - 64;
> 
> Why 64?
> 
> @@ +5110,5 @@
> > +Popup.prototype.resizer = function() {
> > +  var parentRect = this.consoleWrap.getBoundingClientRect();
> > +  var parentHeight = parentRect.bottom - parentRect.top - 64;
> > +
> > +  if (parentHeight < 100) {
> 
> Why 100?

Will add comments in bug 702621
https://github.com/joewalker/gcli/commit/b9b4de93f2fb07227b18f7e26812347d8da58364

> @@ +5140,5 @@
> > +    else {
> > +      this.argFetcher.setMaxHeight(parentHeight);
> > +
> > +      this.hintElement.style.overflowY = null;
> > +      this.hintElement.style.borderBottomColor = 'white';
> 
> I don't think these borderBottomColors (threedshadow and white) should be
> here. That should be in the browser/theme/. If you want to change them, you
> should change the className.

Will also fix in bug 702621
https://github.com/joewalker/gcli/commit/9fe4cb76c00d7cec8bc1033cc62f3e8ea6a379bb

> Same for overflowY (in a CSS, but not in browser/theme/).
> 
> @@ +6473,3 @@
> >    this.input.type = 'text';
> >    this.input.addEventListener('keyup', this.onInputChange, false);
> > +  this.input.style.marginBottom = '0';
> 
> Does it have to be hard-coded in the JS code?

Also bug 702621
https://github.com/joewalker/gcli/commit/d188839a141577790b91db3d774bf0321c6a13bd

> ::: browser/themes/gnomestripe/browser/devtools/gcli.css
> @@ -44,3 +47,3 @@
> > >    vertical-align: middle;
> > >    background-color: transparent;
> > >    font: 12px Consolas, "Lucida Console", monospace;
> 
> Can you use a relative font-size? And can you use the system monospace font?

This has to align with:
https://hg.mozilla.org/integration/fx-team/file/188ee9eaabe7/toolkit/themes/winstripe/global/webConsole.css#l138

> @@ +191,5 @@
> >  
> >  .gcliRequired {
> > +  font-size: 90%;
> > +  color: #f66;
> > +  padding-left: 5px;
> 
> Is that RTL friendly?

Possibly not. It's in a dialog so I need to check. Also in bug 702621
https://github.com/joewalker/gcli/commit/275ec49620ddf95a10329d3fbe322c77d324684f
Comment 14 Paul Rouget [:paul] 2011-11-15 09:41:05 PST
Comment on attachment 573168 [details] [diff] [review]
upload 4

(In reply to Joe Walker from comment #13)
> There's quite a bit of stuff that depends on this bug, so unless we find
> something that's broken rather than just 'could be better' I'd like to fix
> it in bug 702621.

Ok. f+ then.
Comment 15 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-15 10:07:27 PST
Comment on attachment 573168 [details] [diff] [review]
upload 4


Dao: We've done an internal feedback. Comment 11 and comment 13 are worth reading.

Dave: r+ should be a formality given that you've already done this in pull/43, specifically https://github.com/mozilla/gcli/pull/43#issuecomment-2675923
Comment 16 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-17 06:27:36 PST
Created attachment 575164 [details] [diff] [review]
upload 5

Rebase to fix paths.

Dao: Please could you finish reviewing this patch - it's taking a long time to land. Thanks.
Comment 17 Dão Gottwald [:dao] 2011-11-17 12:59:09 PST
Comment on attachment 575164 [details] [diff] [review]
upload 5

>+.gcliterm-hint-nospace {
>+  display: none;
>+}
>+
>+/*
>+ * The language of a console is not en_US or any other common language
>+ * (i.e we don't attempt to translate 'console.log(x)')
>+ * So we fix .gcliterm-input-node/.gcliterm-complete-node elements to be ltr.
>+ * As a result we also want the hints to pop up on the left (above the prompt)
>+ */
>+.gcliterm-input-node,
>+.gcliterm-complete-node,
>+.gcliterm-display {
>+  direction: ltr;
>+}
>+
>+/*
>+ * We want the stuff under .gcliterm-display to obey normal direction rules
>+ * so we need to swap back when the document is in rtl mode.
>+ * The selectors below are faster, but equivalent to:
>+ * .gcliterm-display > *:-moz-locale-dir(rtl) {
>+ *   direction: rtl;
>+ * }
>+ * In non-performance critical situations the above is preferred due to it's
>+ * greater resilience to refactoring
>+ */
>+.gcliterm-hint-parent:-moz-locale-dir(rtl),
>+.hud-output-node:-moz-locale-dir(rtl) {
>+  direction: rtl;
>+}

This stuff belongs in a content stylesheet.
Comment 18 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-18 00:45:35 PST
(In reply to Dão Gottwald [:dao] from comment #17)
> This stuff belongs in a content stylesheet.

Thanks for the reviews - I'm working on the best way of doing this.
Comment 19 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-18 00:46:51 PST
https://tbpl.mozilla.org/?tree=Try&rev=47353dce1691
Comment 20 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-18 09:59:12 PST
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Comment 21 Rob Campbell [:rc] (:robcee) 2011-11-19 08:49:16 PST
https://hg.mozilla.org/integration/fx-team/rev/c9ef187ce6bd
Comment 22 Rob Campbell [:rc] (:robcee) 2011-11-21 06:32:38 PST
https://hg.mozilla.org/mozilla-central/rev/c9ef187ce6bd

Note You need to log in before you can comment on or make changes to this bug.