Closed Bug 692742 Opened 13 years ago Closed 13 years ago

GCLI popup dialogs sometimes have scrollbars

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 11

People

(Reporter: jwalker, Assigned: jwalker)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 4 obsolete files)

      No description provided.
Especially JS.
Perhaps the dialog should have whatever width it wants, and no fixed height - just a max-height.
Assignee: nobody → jwalker
Priority: -- → P1
Summary: GCLI popup dialogs sometimes have scrollbars work out how to prevent this → GCLI popup dialogs sometimes have scrollbars
Blocks: 697140
Status: NEW → ASSIGNED
Attached patch upload 1 (obsolete) — Splinter Review
Attachment #571722 - Flags: review?(dao)
What's the point of those -webkit-* properties in that patch?
(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.
Attached patch upload 2 (obsolete) — Splinter Review
No more -webkit-
Attachment #571722 - Attachment is obsolete: true
Attachment #571722 - Flags: review?(dao)
Attachment #571796 - Flags: review?(dao)
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?
(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.
Attached patch upload 3 (obsolete) — Splinter Review
Attachment #571796 - Attachment is obsolete: true
Attachment #571796 - Flags: review?(dao)
Attachment #571933 - Flags: review?(dao)
Attached patch upload 4 (obsolete) — Splinter Review
Attachment #571933 - Attachment is obsolete: true
Attachment #571933 - Flags: review?(dao)
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):'
Attachment #573168 - Flags: feedback?(paul)
Blocks: 699411
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?
Attachment #573168 - Flags: feedback?(paul) → feedback-
Blocks: 702621
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 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.
Attachment #573168 - Flags: feedback- → feedback+
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
Attachment #573168 - Flags: review?(dcamp)
Attachment #573168 - Flags: review?(dao)
Attachment #573168 - Flags: review?(dcamp) → review+
Attached patch upload 5Splinter Review
Rebase to fix paths.

Dao: Please could you finish reviewing this patch - it's taking a long time to land. Thanks.
Attachment #573168 - Attachment is obsolete: true
Attachment #573168 - Flags: review?(dao)
Attachment #575164 - Flags: review?(dao)
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.
Attachment #575164 - Flags: review?(dao) → review+
(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.
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
Whiteboard: [land-in-fx-team]
No longer blocks: 697140
https://hg.mozilla.org/integration/fx-team/rev/c9ef187ce6bd
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c9ef187ce6bd
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.