Closed Bug 948324 Opened 7 years ago Closed 7 years ago

DevTools Themes: Remote connection screen does not have devtools text input styling

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox26 unaffected, firefox27 unaffected, firefox28 affected, firefox29 fixed)

RESOLVED FIXED
Firefox 29
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 --- affected
firefox29 --- fixed

People

(Reporter: past, Assigned: bgrins)

References

Details

(Whiteboard: [good first verify])

Attachments

(6 files, 3 obsolete files)

Attached image How it looks now
It looks like the recent style changes had an effect on the remote connection screen. Some things are an improvement, like the way the port number input field is rendered, or the rounded borders for input boxes on Linux. What I find most jarring is the white background on input fields and the button, and maybe the lack of rounded borders on Mac.
We need to move the .devtools-textinput styles into common.css (from toolbars.inc.css) so that they are included for the rest of the browser chrome.
Weird, min-height doesn't seem to apply to input[type=number] so just copying over the styles causes mismatched heights.  We could remove the type=number and it will look like it used to.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
IIRC we were thinking about shrinking the huge input boxes a few months back, so you could just drop min-height.
The only design I see with the textbox in it is this one: https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/DarkTheme-Console-AllToggled@2x.png, which does seem to have a smaller total height (as well as a different background color).
Summary: Style glitches in the remote connection screen → DevTools Themes: Remote connection screen does not have devtools text input styling
Darrin, I'm looking at theming the text boxes to match the new designs (https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/DarkTheme-Console-AllToggled@2x.png).

The textbox seems to have a background, rgb(21,25,30), which is quite close to the sidebar background, rgb(24, 29, 32), as seen here: https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors.  Should I set the input background as it is in the image, or should I match it to one of the palette colors from the theme?

Also, the text color seems to be something like rgb(119, 122, 123) - should I use this or grab one of the colors from the theme?
Flags: needinfo?(dhenein)
(In reply to Panos Astithas [:past] from comment #0)
> Created attachment 8345193 [details]
> How it looks now
> 

I think this looks better than what we had until now :)
I would say use the (dark) background color from the mockup, and a similar border color (from the mockup) as well. The text color in the mockup seems to be for placeholder text (usually less visible), so I would suggest one of the brighter colors from the palette for the text (maybe  rgba(184, 200, 217, 1)?).

Did we make any decisions regarding rounded text boxes?
Flags: needinfo?(dhenein) → needinfo?(bgrinstead)
(In reply to Darrin Henein [:darrin] from comment #8)
> I would say use the (dark) background color from the mockup, and a similar
> border color (from the mockup) as well. The text color in the mockup seems
> to be for placeholder text (usually less visible), so I would suggest one of
> the brighter colors from the palette for the text (maybe  rgba(184, 200,
> 217, 1)?).
> 
> Did we make any decisions regarding rounded text boxes?

You are needinfo'ed on bug 947346 for that decision only :)
Flags: needinfo?(bgrinstead)
> Did we make any decisions regarding rounded text boxes?

There is some disagreement in Bug 947346 about this.

By the way, do you know what the border radius for the text boxes is in the mockup?
Flags: needinfo?(dhenein)
(In reply to Victor Porof [:vp] from comment #7)
> (In reply to Panos Astithas [:past] from comment #0)
> > Created attachment 8345193 [details]
> > How it looks now
> > 
> 
> I think this looks better than what we had until now :)

Yes, it really doesn't look too bad now (without the styling applied).  It isn't consistent with the rest of the tools, but it is kind of a separate page.  I'd be fine going either way with this.
The white background color of the text boxes does look out of place to me though.
> By the way, do you know what the border radius for the text boxes is in the
> mockup?

It looks like 50% (circular) to me. Does that help?
Flags: needinfo?(dhenein)
Darrin,
Here is a screenshot of the work in progress styling on the text boxes.  You can see them both on the connect screen and on the .  I've pushed to try so that a build can be played with to get a feel for the different states: https://tbpl.mozilla.org/?tree=Try&rev=61652a666c8f.  See anything that should be updated in order to match the designs at https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/?

Some of the states aren't pictured in the designs, so I just picked something (see the red validation error in the inspector in the screenshot, for instance).
Attachment #8346222 - Flags: ui-review?(dhenein)
> You can see them both on the connect screen and on the .

I meant to say you can see the styling on both the connect screen on and on the inspector search box.
Attachment #8345289 - Attachment description: Screenshot 2013-12-10 09.04.51.png → input[type=number] min-height
Attachment #8346222 - Attachment description: Screenshot 2013-12-11 17.07.40.png → Updated text input styles.png
Attachment #8346222 - Attachment filename: Screenshot 2013-12-11 17.07.40.png → Updated-text-input-styles.png
Attached patch theme-connect-screen.patch (obsolete) — Splinter Review
The patch going along with the screenshot in Attachment 8346222 [details].  Panos, does this look good (assuming the textbox design passes ui-review)?
Attachment #8348764 - Flags: review?(past)
Comment on attachment 8346222 [details]
Updated text input styles.png

This look great.
Attachment #8346222 - Flags: ui-review?(dhenein) → ui-review+
Comment on attachment 8348764 [details] [diff] [review]
theme-connect-screen.patch

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

I'm surprised we want to use an unstyled button element, but I'm no UX expert, so OK. Funny detail: a high-contrast white button makes its vertical misalignment more apparent, but I don't really care to be honest. It did look better in attachment 8345193 [details] though.
Attachment #8348764 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #18)
> -----------------------------------------------------------------
> 
> I'm surprised we want to use an unstyled button element, but I'm no UX
> expert, so OK.

It was my understanding that this bug addresses the styling of the text inputs only. Should we be discussing the buttons as well? I am fine with that, just didn't want to introduce new scope to this bug.
(In reply to Darrin Henein [:darrin] from comment #19)
> (In reply to Panos Astithas [:past] from comment #18)
> > -----------------------------------------------------------------
> > 
> > I'm surprised we want to use an unstyled button element, but I'm no UX
> > expert, so OK.
> 
> It was my understanding that this bug addresses the styling of the text
> inputs only. Should we be discussing the buttons as well? I am fine with
> that, just didn't want to introduce new scope to this bug.

I guess we may as well go ahead and talk about that button here, as well.  In Bug 947836, it was decided to not move the button styles over into the shared common.css file, to prevent copying too much across to common.css (a file which is loaded inside the main browser UI).

I just thought of a different solution that could work, though - we could load the dark theme CSS file into the connect screen.  This would allow us to get both the button and the new textbox styling without needing to introduce a bunch of global styles to the browser.  Will attach a screenshot of this.
Attached image connect-screen-with-dark-theme (obsolete) —
Using dark-theme.css for textbox and button styling, also moves the button over a bit to the right to vertically align with the text boxes
Attachment #8348803 - Flags: ui-review?(dhenein)
Comment on attachment 8348803 [details]
connect-screen-with-dark-theme

Ya, this is much better. Can we align the button with the right edge of the text inputs though (vs the left as seen)?
Flags: needinfo?(bgrinstead)
Attached patch theme-connect-screen.patch (obsolete) — Splinter Review
Panos,
This should do the same thing, but updates the button as well (and right aligns it with the text boxes).  Can you think of any reason not to include dark-theme.css in this connect.xhtml file?

This also updates the selection text color (for all elements, but the main reason was for readability of selected text for inputs), as discussed with Darrin in IRC.
Attachment #8348764 - Attachment is obsolete: true
Attachment #8348840 - Flags: review?(past)
Flags: needinfo?(bgrinstead)
Updated button alignment, also shows selected text coloring
Attachment #8348803 - Attachment is obsolete: true
Attachment #8348803 - Flags: ui-review?(dhenein)
Attachment #8348841 - Flags: ui-review?(dhenein)
Comment on attachment 8348841 [details]
connect-screen-with-dark-theme

Much better, looks awesome. Thanks Brian!
Attachment #8348841 - Flags: ui-review?(dhenein) → ui-review+
See Comment 23
Attachment #8348840 - Attachment is obsolete: true
Attachment #8348840 - Flags: review?(past)
Attachment #8348846 - Flags: review?(past)
Comment on attachment 8348846 [details] [diff] [review]
theme-connect-screen.patch

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

This seems fine to me.
Attachment #8348846 - Flags: review?(past) → review+
Depends on: 951310
https://hg.mozilla.org/mozilla-central/rev/c835fcc812b2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Brian, can you uplift this to Aurora as well?
We worked around the min-height issue on input[type=number] by not using min-height, so this doesn't depend on Bug 951310.
No longer depends on: 951310
Whiteboard: [good first verify]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.