1590019 - Make placeholder styling consistent in Debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P3)

Description

[:Harald Kirschner :digitarald]

Follow up from https://bugzilla.mozilla.org/show_bug.cgi?id=1585764#c11

• Remove italic style
• Add opacity 1
• Use text color --theme-text-color-alt (or --theme-text-color-inactive)

Comment 1

[Florens Verschelde :fvsch]

I'm not finding the specific changeset(s) where this style was introduced.
If we tried to address some UX issue by using italic placeholders, it would be great to find the original issue.

Some files where we have this style:

https://searchfox.org/mozilla-central/source/devtools/client/debugger/src/components/SecondaryPanes/XHRBreakpoints.css#49
https://searchfox.org/mozilla-central/source/devtools/client/debugger/src/components/SecondaryPanes/Expressions.css#40
https://searchfox.org/mozilla-central/source/devtools/client/debugger/src/components/SecondaryPanes/EventListeners.css#138
https://searchfox.org/mozilla-central/source/devtools/client/debugger/src/components/PrimaryPanes/OutlineFilter.css#20

Comment 2

[Kyle Knaggs]

Attachment: inputLocationDiagram.png

Hello, I’ve been looking into this bug during the past couple of days and I believe have found some information that would be really useful to whoever decides to tackle this issue.

I was able to locate the Debugger inputs with italic placeholders in the UI. These inputs are highlighted in red in the screenshot above.

I also spent a fair amount of time attempting to answer Florens’ question about the specific changeset where the font-style:italic rule was first introduced for placeholder text.

My research indicates that the font-style:italic rule was first introduced for placeholder text in changeset number 2301f25d15957f24d36b68103b1b4e9786344392. This patch, which landed in January 2017 has a corresponding Bugzilla Issue, GitHub Pull Request, and GitHub Issue. Even though the issue associated with the pull request does not explicitly state a desire for italic placeholder text, it appears that this style rule made it into the code base as part of a desire to “improve the UI”. I also think that it is important to note that the screenshot associated with the pull request indicates that the author of this pull request was probably styling the Watch Expressions input so that it very closely resembled the Watch Expressions input in Chrome Debugger.

The links associated with the introduction of this feature are as follows:

Watch Expressions input:
Mercurial Diff: https://hg.mozilla.org/mozilla-central/rev/2301f25d15957f24d36b68103b1b4e9786344392
Bugzilla Issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1331654
GitHub Pull Request: https://github.com/firefox-devtools/debugger/pull/1593
GitHub Issue: https://github.com/firefox-devtools/debugger/issues/1491

After the introduction of the Watch Expressions input, it appears that the authors of the corresponding style rules for the other 3 inputs were probably just attempting to match the appearance of their features to the original Watch Expressions input. This is my hypothesis as:

1. All 3 of these inputs appeared after the Watch Expressions input was present in the UI.
2. All 3 of these inputs have had italic placeholder text since their inception.
3. I have not found any text in the issues or pull requests associated with these features that suggest otherwise.

The links associated with the introduction of these features are as follows:

XHR Breakpoints input:
Pull Request: https://github.com/firefox-devtools/debugger/pull/6934
Issue: https://github.com/firefox-devtools/debugger/issues/5255

Outline input:
Pull Request: https://github.com/firefox-devtools/debugger/pull/7088
Issue: https://github.com/firefox-devtools/debugger/issues/7054

Event Listener Breakpoints input:
Diff: https://hg.mozilla.org/mozilla-central/rev/47f5993a8467ea4a801f9ea6abb8a81309a7577e
Issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1567966

Comment 3

[Kyle Knaggs]

If nobody else is working on this bug I will be more than happy to claim it if that's okay.

Comment 4

[Florens Verschelde :fvsch]

Hi Kyle, thanks for the research. The bug is yours, feel free to assign me as a reviewer (r=fvsch) if you push a patch to Phabricator.

I think the only undecided question is whether we want to use --theme-text-color-alt (medium contrast) or --theme-text-color-inactive (less contrasted, could be an accessibility issue) for the placeholder text color. Trying both and making screenshots for a quick UI review would be great.

Comment 5

[Kyle Knaggs]

Attachment: --theme-text-color-alt.png

Comment 6

[Kyle Knaggs]

Attachment: --theme-text-color-inactive.png

Comment 7

[Kyle Knaggs]

Hi Florens, thank you for assigning me to the bug and volunteering to be a reviewer for this issue.

I have attached screenshots of the two options above for your reference. The first image has placeholder text whose color has been set to --theme-text-color-alt. The second image has placeholder text whose color has been set to --theme-text-color-inactive. In both instances font-style: italic has been removed and the opacity has been set to 1 per Harald's request.

The contrast ratios for each of these options on a white background are as follows:

1. --theme-text-color-alt: 4.74
2. --theme-text-color-inactive: 2.14

When thinking about what the color of the placeholder text should be for the inputs we are discussing I thought that it might also be worth considering what the color of the placeholder text was in:

1. The other search inputs in the Debugger tab. The “Find in Files…”, “Find in File…” and “Go to File…” inputs are what I am referring to here.
2. The search inputs in the remaining DevTools tabs. I have used devtools/client/shared/components/SearchBox.js as a reference point for all non-DevTools search inputs this as this appears to be the most commonly used search input throughout the remainder of the project.

From my research I was able to learn that:

1. The color of the placeholder text in the other search inputs in the Debugger tab is --theme-toolbar-color. In addition to this the opacity value has not been reset to 1 here either.
2. There appears to be no additional styling applied to the placeholder text in the search inputs in the other DevTools tabs.

Let me know what you think about the two options that I have posted above. I have one commit with the --theme-text-color-alt option and another commit with the --theme-text-color-inactive option locally on my computer. Once you let me know which solution you prefer I can create a patch with the corresponding commit and submit it to Phabricator.

Comment 8

[Kyle Knaggs]

Attachment: Bug 1590019 - Make the styling of placeholder text in the Debugger consistent r=davidwalsh

Before this change the styling of the placeholder text for inputs in the Debugger was inconsistent. The adjustment of the color to --theme-text-color-alt and the opacity to 1 sets the contrast ratio of the placeholder text to 4.74 when paired with a white background. This ensures that it passes the 4.5:1 contrast requirement of WCAG.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/547bd3a15586
Make the styling of placeholder text in the Debugger consistent r=nchevobbe

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/547bd3a15586