Closed Bug 1552110 Opened 5 months ago Closed 5 months ago

Wrong color for security values

Categories

(DevTools :: Netmonitor, defect, P2)

defect

Tracking

(firefox-esr60 unaffected, firefox67 unaffected, firefox68 verified, firefox69 verified)

VERIFIED FIXED
Firefox 69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox68 --- verified
firefox69 --- verified

People

(Reporter: Honza, Assigned: Honza)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [qa-triaged])

Attachments

(2 files)

STR:

  1. Open DevTools, select the Network panel
  2. Load google.com
  3. Select the first request
  4. Switch into the Security side panel (you can click on the security icon in the Domain column)
  5. Security value in the panel should be red, but they are blue -> BUG
Priority: -- → P3
Flags: needinfo?(bhackett1024)
Flags: needinfo?(bhackett1024)
  • Last Good build_date: 2019-03-25 17:09:41.576000 changeset: 3d5cd10cb1b20c1f83a189fbeb2e22f470bac0ec

  • First bad build_date: 2019-03-26 01:45:53.488000 changeset: 4146cbd2103cb97b239033f215eb750f1ca880f1

  • Pushlog: link

  • Bug 1537724looks like the regressor.

67.0b19, 67.0 not affected; updating flags.

Has Regression Range: --- → yes
Has STR: --- → yes

@Cristi: thanks!

@Florens: Can you please take a look at this. The bug 1537724 removes the following rule, perhaps we want it back?

.network-monitor .tree-container .treeTable .treeValueCell input {
  color: var(--string-color);
}

Honza

Flags: needinfo?(florens)
Priority: P3 → P2
Regressed by: 1537724

For info, this is tracked as a regression in 68. We just entered the beta cycle for 68, so we have time to uplift any fix we would land in nightly.

Hmm we don't want this text color for input elements that are functionally text inputs.
Only for input elements used as display elements.

Ideally for display we wouldn't have <input> elements, but maybe we can fix that with a more precise CSS selector.

Thanks for the comment Florens!

What do you think about the following patch?

diff --git a/devtools/client/netmonitor/src/components/SecurityPanel.js b/devtools/client/netmonitor/src/components/SecurityPanel.js
--- a/devtools/client/netmonitor/src/components/SecurityPanel.js
+++ b/devtools/client/netmonitor/src/components/SecurityPanel.js
@@ -15,16 +15,23 @@ const {
   fetchNetworkUpdatePacket,
   getUrlHost,
 } = require("../utils/request-utils");

 // Components
 const TreeViewClass = require("devtools/client/shared/components/tree/TreeView");
 const PropertiesView = createFactory(require("./PropertiesView"));

+loader.lazyGetter(this, "Rep", function() {
+  return require("devtools/client/shared/components/reps/reps").REPS.Rep;
+});
+loader.lazyGetter(this, "MODE", function() {
+  return require("devtools/client/shared/components/reps/reps").MODE;
+});
+
 const { div, input, span } = dom;
 const NOT_AVAILABLE = L10N.getStr("netmonitor.security.notAvailable");
 const ERROR_LABEL = L10N.getStr("netmonitor.security.error");
 const CIPHER_SUITE_LABEL = L10N.getStr("netmonitor.security.cipherSuite");
 const WARNING_CIPHER_LABEL = L10N.getStr("netmonitor.security.warning.cipher");
 const ENABLED_LABEL = L10N.getStr("netmonitor.security.enabled");
 const DISABLED_LABEL = L10N.getStr("netmonitor.security.disabled");
 const CONNECTION_LABEL = L10N.getStr("netmonitor.security.connection");
@@ -95,22 +102,24 @@ class SecurityPanel extends Component {
     }

     return span({ className: "security-info-value" },
       member.name === ERROR_LABEL ?
         // Display multiline text for security error
         value
         :
         // Display one line selectable text for security details
-        input({
-          className: "textbox-input",
-          readOnly: "true",
-          value,
-        })
-      ,
+        Rep(Object.assign(props, {
+          // FIXME: A workaround for the issue in StringRep
+          // Force StringRep to crop the text everytime
+          member: Object.assign({}, member, { open: false }),
+          mode: MODE.TINY,
+          cropLimit: 60,
+          noGrip: true,
+        })),
       weaknessReasons.includes("cipher") &&
       member.name === CIPHER_SUITE_LABEL ?
         // Display an extra warning icon after the cipher suite
         div({
           id: "security-warning-cipher",
           className: "security-warning-icon",
           title: WARNING_CIPHER_LABEL,
         })

Honza

Tried it locally and it seems to fix:

  • The text color issue.
  • Selection issues: copy-and-paste now works well (the current code with inputs only works when pasting in HTML mode, e.g. in Word or TextEdit, and doesn't work when copying in plain text mode in a code editor).
  • And we get the benefits from the selection styles work from bug 1537724.

Looking at the code, I'm not familiar with the reps API so I have no comment there.
Just wondering if Object.assign(props, { member, mode, etc }) should be Object.assign({}, props, { member, mode, etc }) (or {...props, member, mode, etc }) to avoid mutating the props object (which is not reused later on so it probably doesn't change a thing).

If we want a CSS-only fix, this seems to work well, but I'd rather go with your patch instead:

diff --git a/devtools/client/netmonitor/src/assets/styles/NetworkDetailsPanel.css b/devtools/client/netmonitor/src/assets/styles/NetworkDetailsPanel.css
--- a/devtools/client/netmonitor/src/assets/styles/NetworkDetailsPanel.css
+++ b/devtools/client/netmonitor/src/assets/styles/NetworkDetailsPanel.css
@@ -470,6 +470,11 @@
   display: flex;
 }

+/* Read-only inputs used to display text values in Security pane */
+.network-monitor .treeRow:not(.selected) .security-info-value input {
+  color: var(--string-color);
+}
+
 .network-monitor .security-warning-icon {
   width: 12px;
   height: 12px;
Flags: needinfo?(florens)

(In reply to Florens Verschelde :fvsch from comment #8)

If we want a CSS-only fix, this seems to work well, but I'd rather go with your patch instead:

Sounds good, I posted the patch into Phab.
Thanks for the help!
Honza

Try try push looks good, so the patch is ready for review.
Honza

Flags: needinfo?(florens)
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0211d4e6183
Properly render values in the Security panel r=fvsch
Flags: needinfo?(florens)
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69

Fix verified with 69.0a1 (2019-05-26) on the available OS's.

Status: RESOLVED → VERIFIED

Please request beta uplift when you get a chance.

Comment on attachment 9067053 [details]
Properly render values in the Security panel

Beta/Release Uplift Approval Request

  • User impact if declined: Wrong colors (for labels) in the Network monitor, DevTools Toolbox.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Small patch, the functionality used only by Developers (not by regular Firefox user)
  • String changes made/needed: n/a
Attachment #9067053 - Flags: approval-mozilla-beta?

Comment on attachment 9067053 [details]
Properly render values in the Security panel

netmonitor fix, approved for 68.0b5

Attachment #9067053 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Whiteboard: [qa-triaged]

Fix confirmed with 68.0b5 on Win10, macOS 10.13, Ubuntu 18.04.

Flags: qe-verify+
Whiteboard: [qa-triaged]
Whiteboard: [qa-triaged]
You need to log in before you can comment on or make changes to this bug.