Closed Bug 1036709 Opened 6 years ago Closed 6 years ago

The styling of active / changing nodes in the source view is inconsistent and hard to read

Categories

(DevTools :: Inspector, defect)

30 Branch
x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: canuckistani, Assigned: gl)

Details

Attachments

(5 files, 2 obsolete files)

Attached image uglay-mutation.gif
The colour used in the source view to indicate a node is changing is ugly, and more importantly hard to read. See attachment.
Assignee: nobody → gabriel.luong
Status: NEW → ASSIGNED
Please try to use a color from: https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors, possibly one of the orangish accent colors (possibly with alpha if needed).  Also keep in mind that this color (currently #a18650) is used for flashing variable view and table widget values that have changed.
Attached patch 1036709.patch (obsolete) — Splinter Review
Attachment #8458174 - Flags: review?(bgrinstead)
I should note that the colours do fade quickly and the screenshot for the markup view is more representative of the colour transitions.
Comment on attachment 8458174 [details] [diff] [review]
1036709.patch

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

You'll need to also update the dark theme using a corresponding color from the colors page.  Take a look at all instances of the string: http://dxr.mozilla.org/mozilla-central/search?q=%23a18650&redirect=true.
Attachment #8458174 - Flags: review?(bgrinstead)
As discussed on IRC, the current color in the dark theme isn't really too bad.  But I think we could still take this as a chance to update it to be a bit more inline with the rest of the pallete.

I've thought about this a bit more.  We should use non-alpha versions and keep the wiki updated with them, since we are counting these as different from 'accent' colors.  So, first we pick an accent color with alpha, then measure what that actually becomes 'flattened' on the background.

Here are a couple of suggestions (I don't feel too strongly about the final colors, as long as they are derived from the existing palette and look reasonable):

Dark theme:

 * 'Light Orange' with alpha: rgba(217, 155, 40, .8) which flattened becomes: #B28025

Light theme

 * 'Light Orange' with alpha: background: rgba(217, 126, 0, .6) which flattened becomes: #E6B064

Aside: there is one spot that will be trickier to update: http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/widgets.inc.css#1370.  I guess we will need a separate keyframe for light and dark theme then change the animation name for .table-widget-cell.flash-out based on which theme is applied.
(In reply to Gabriel Luong (:gl) from comment #4)
> Created attachment 8458181 [details]
> New highlight colour in the variables view
> 
> I should note that the colours do fade quickly and the screenshot for the
> markup view is more representative of the colour transitions.

Yeah, I commented out the line that removes the BG class when testing locally: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#1547
Attached patch 1036709.patch (obsolete) — Splinter Review
Attachment #8458174 - Attachment is obsolete: true
Attachment #8458321 - Flags: review?(bgrinstead)
Comment on attachment 8458321 [details] [diff] [review]
1036709.patch

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

One main note below - also, please change commit message to describe what the patch is doing, not stating the problem

::: browser/devtools/shared/widgets/TableWidget.js
@@ +910,5 @@
>     * Flashes the cell for a brief time. This when done for ith cells in all
>     * columns, makes it look like the row is being highlighted/flashed.
>     */
>    flash: function() {
> +    let flashClass = (Services.prefs.getCharPref("devtools.theme") === "light") ?

I'm looking at the code, and I don't think you actually need this change - looks like the css changes in widgets.inc.css will handle this case fine already, right?

::: browser/themes/shared/devtools/dark-theme.css
@@ +32,5 @@
>  }
>  
>  .theme-bg-contrast,
>  .variable-or-property:not([overridden])[changed] { /* contrast bg color to attract attention on a container */
> +  background: #B28025;

Add a comment after each usage of the new colors with /* Background - Attention */.  Helps keep track of what's what until the switch to variables.
Attachment #8458321 - Flags: review?(bgrinstead)
Comment on attachment 8458321 [details] [diff] [review]
1036709.patch

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

Patching incoming.

::: browser/devtools/shared/widgets/TableWidget.js
@@ +910,5 @@
>     * Flashes the cell for a brief time. This when done for ith cells in all
>     * columns, makes it look like the row is being highlighted/flashed.
>     */
>    flash: function() {
> +    let flashClass = (Services.prefs.getCharPref("devtools.theme") === "light") ?

Fixed. This change was indeed unnecessary. I must've thought I changed the class names to light/dark-flash-out, but in reality those are the keyframes animation name.

::: browser/themes/shared/devtools/dark-theme.css
@@ +32,5 @@
>  }
>  
>  .theme-bg-contrast,
>  .variable-or-property:not([overridden])[changed] { /* contrast bg color to attract attention on a container */
> +  background: #B28025;

Fixed. Added the comments
Attached patch 1036709.patchSplinter Review
Attachment #8458321 - Attachment is obsolete: true
Attachment #8458854 - Flags: review?(bgrinstead)
Comment on attachment 8458854 [details] [diff] [review]
1036709.patch

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

Looks good
Attachment #8458854 - Flags: review?(bgrinstead) → review+
Reminder to myself to update the wiki after this lands
Flags: needinfo?(bgrinstead)
(In reply to Gabriel Luong (:gl) from comment #14)
> https://tbpl.mozilla.org/?tree=Try&rev=ec0561b27905

Jeff, what do you think about the new look?
Flags: needinfo?(jgriffiths)
Attached image new gif screencast
Here's how it looks with the light theme now ( see attached gif )
Flags: needinfo?(jgriffiths)
(In reply to Jeff Griffiths (:canuckistani) from comment #16)
> Created attachment 8459074 [details]
> new gif screencast
> 
> Here's how it looks with the light theme now ( see attached gif )

Marking checkin needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cfe6339e0fcb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Updated "Background - Attention" colors on https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors to match the new colors
Flags: needinfo?(bgrinstead)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.