Last Comment Bug 663898 - [highlighter] make sure the selected node is identifiable even on very dark and very light backgrounds
: [highlighter] make sure the selected node is identifiable even on very dark a...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on: 664436
Blocks: 663830 669656
  Show dependency treegraph
 
Reported: 2011-06-13 10:58 PDT by Paul Rouget [:paul]
Modified: 2011-11-25 08:37 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (7.21 KB, patch)
2011-06-20 06:39 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.1 (7.97 KB, patch)
2011-06-20 07:48 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.2 (7.97 KB, patch)
2011-06-21 08:50 PDT, Paul Rouget [:paul]
mihai.sucan: review+
dietrich: review+
Details | Diff | Review
patch v1.4 (7.89 KB, patch)
2011-06-27 08:36 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.4.1 (rebase) (7.89 KB, patch)
2011-06-28 08:23 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
simple CSS-only patch using box-shadow and outline (1.73 KB, patch)
2011-06-30 01:40 PDT, Dão Gottwald [:dao]
dietrich: review+
rcampbell: review+
Details | Diff | Review
Hard to see dashed background on black element with inspect button active (243.19 KB, image/jpeg)
2011-11-25 08:37 PST, jds2501
no flags Details

Description Paul Rouget [:paul] 2011-06-13 10:58:07 PDT
For example, if we highlight any element in the header of http://developer.mozilla.org, we can't see the veil.
Comment 1 Paul Rouget [:paul] 2011-06-15 05:21:19 PDT
The approach will be to add a black & white dashed border around the element.
A first implementation is available in bug 663781.

Mihai commented on that code:
> This code doesn't deal with iframes properly.
>
> clientLeft/Top are relative to the iframe. You want the clientLeft/Top
> coordinates for the top frame/window. You need to walk the frames tree to
> get the values.
> 
> Still I would recommend against doing that. The highlight() method does the
> tree walking and you can cache information in that step, I believe. Please
> correct me if I am wrong.

Next patch will include tests and support for iframes.
Comment 2 Paul Rouget [:paul] 2011-06-20 06:39:39 PDT
Created attachment 540449 [details] [diff] [review]
patch v1

This patch add black & white dashes around the transparent box. This code supports large border (> 1px), which brings some complexity to the code. We may consider supporting only 1px large borders.
Comment 3 Paul Rouget [:paul] 2011-06-20 07:48:55 PDT
Created attachment 540463 [details] [diff] [review]
patch v1.1
Comment 4 Mihai Sucan [:msucan] 2011-06-21 08:49:56 PDT
Comment on attachment 540463 [details] [diff] [review]
patch v1.1

I've been thinking about this code .... how about using a grid-line background image for the veil? instead of a border around the highlighted node.

I liked the blueish gridlined image Rob had for the veil.

(this patch is overly complex for what we want)
Comment 5 Paul Rouget [:paul] 2011-06-21 08:50:47 PDT
Created attachment 540763 [details] [diff] [review]
patch v1.2
Comment 6 Paul Rouget [:paul] 2011-06-21 09:05:13 PDT
We want to make sure that it's always obvious which node is selected.

In the future, we want to provide to the user a way to toggle the dark background (button will be introduced here https://bugzilla.mozilla.org/show_bug.cgi?id=663833#c2 -- description of the problem there https://bugzilla.mozilla.org/show_bug.cgi?id=663781#c0).

That's why I think we should have this black & white border.

Also, with the current mechanism, it's impossible to draw a grid or a checkerboard: 4 boxes, 4 background, all non-aligned. I tried to align them with some JS, but the result is very laggy.
Comment 7 Mihai Sucan [:msucan] 2011-06-21 09:07:25 PDT
Comment on attachment 540763 [details] [diff] [review]
patch v1.2

Giving the patch r+, but I would like a better solution. I am not sure what I can suggest better - we discussed on IRC several possible different approaches ... each with some problems.
Comment 8 Mihai Sucan [:msucan] 2011-06-21 09:09:19 PDT
Comment on attachment 540763 [details] [diff] [review]
patch v1.2

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

::: browser/themes/gnomestripe/browser/browser.css
@@ +1942,5 @@
>    cursor: pointer;
>  }
> +
> +#highlighter-veil-transparentbox {
> +    border: 1px solid white;

Make sure colors are provided like throughout the rest of the file.

(hex, rgb, hsv, or color names?)
Comment 9 Rob Campbell [:rc] (:robcee) 2011-06-21 11:15:04 PDT
(In reply to comment #6)
...
> Also, with the current mechanism, it's impossible to draw a grid or a
> checkerboard: 4 boxes, 4 background, all non-aligned. I tried to align them
> with some JS, but the result is very laggy.

one of the reasons I wanted to use canvas for the background. The ability to use it as a drawing surface is kind of useful.
Comment 10 Dietrich Ayala (:dietrich) 2011-06-22 08:45:22 PDT
Comment on attachment 540763 [details] [diff] [review]
patch v1.2

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

r=me with nits fixed and if you've ensured RTL works. let's roll with this incremental improvement and see how things look. can backout and try to find the ideal solution later if this doesn't work.

::: browser/base/content/inspector.js
@@ +170,4 @@
>    },
>  
>    /**
> +   * Get the size of the transparent box borders

nit: this doesn't have a return value, so "get" isn't really correct. "save" might be better.

@@ +185,5 @@
> +       top: top,
> +       right: right,
> +       bottom: bottom,
> +       left: left
> +     }

nit: not sure the local vars are worth it. just more LOC.

@@ +355,5 @@
> +        actualBorderLeftWidth = this.veilTransparentBoxOffsets.left;
> +      }
> +
> +      let middleHeight = aRect.height + actualBorderTopWidth + this.veilTransparentBoxOffsets.bottom;
> +      let transparentWidth = aRect.width + actualBorderLeftWidth + this.veilTransparentBoxOffsets.right;

max line length at 80 plz (https://developer.mozilla.org/En/Developer_Guide/Coding_Style).
Comment 11 Paul Rouget [:paul] 2011-06-27 08:36:18 PDT
Created attachment 542165 [details] [diff] [review]
patch v1.4

Addressed comment 8 and comment 10.
Comment 12 Paul Rouget [:paul] 2011-06-28 08:23:32 PDT
Created attachment 542478 [details] [diff] [review]
patch v1.4.1 (rebase)
Comment 13 Dão Gottwald [:dao] 2011-06-29 10:49:18 PDT
Comment on attachment 542478 [details] [diff] [review]
patch v1.4.1 (rebase)

>+        skin/classic/browser/dashes.png

This needs a better name, you'd need to do an MXR search to figure out what "dashes.png" is used for.

By the way (a big one), why not just use a white box-shadow? I imagine this would solve this bug equally well and not require any JS support.
Comment 14 Paul Rouget [:paul] 2011-06-29 13:41:24 PDT
(In reply to comment #13)
> Comment on attachment 542478 [details] [diff] [review] [review]
> patch v1.4.1 (rebase)
> 
> >+        skin/classic/browser/dashes.png
> 
> This needs a better name, you'd need to do an MXR search to figure out what
> "dashes.png" is used for.

Ok!

> By the way (a big one), why not just use a white box-shadow? I imagine this
> would solve this bug equally well and not require any JS support.

We want black and white dashes to make this box visible on dark background AND on white background, even if the dark veil is not visible (we want the user to be able to disable it).
Comment 15 Dão Gottwald [:dao] 2011-06-29 23:35:03 PDT
A white box-shadow does make it visible in case the dark veil isn't visible. What's the use case for disabling the veil? We shouldn't add dubious preferences just because we can.
Comment 16 Paul Rouget [:paul] 2011-06-30 00:53:10 PDT
(In reply to comment #15)
> A white box-shadow does make it visible in case the dark veil isn't visible.

Not if the background is white.

> What's the use case for disabling the veil? We shouldn't add dubious
> preferences just because we can.

I totally agree here. But being able to remove the veil is an important option. While you're debugging the style of your web page, you may not want to have 90% of the "look&feel" of your page altered by the dark veil.
Comment 17 Dão Gottwald [:dao] 2011-06-30 01:04:10 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > A white box-shadow does make it visible in case the dark veil isn't visible.
> 
> Not if the background is white.

Only if you remove the veil. (And as a former web developer, I'm not sure I agree that the veil makes debugging the page style harder.)

But it's not clear to me why any JS should be needed for this anyway. I need to take a closer look at the patch.
Comment 18 Dão Gottwald [:dao] 2011-06-30 01:40:04 PDT
Created attachment 543090 [details] [diff] [review]
simple CSS-only patch using box-shadow and outline

The shadow and outline seem to overlap inconsistently, but I suspect that's a platform bug that should just be fixed there.
Comment 19 Paul Rouget [:paul] 2011-06-30 01:52:45 PDT
(In reply to comment #18)
> Created attachment 543090 [details] [diff] [review] [review]
> simple CSS-only patch using box-shadow and outline

CSS magic! Excellent!
I've tried this approach before, but didn't know about outline-offset.

> The shadow and outline seem to overlap inconsistently, but I suspect that's
> a platform bug that should just be fixed there.

I can't reproduce here (Linux).


Thank you so much for that. I wasn't happy at all with this JS mechanism.
I'll update the patch.
Comment 20 Rob Campbell [:rc] (:robcee) 2011-07-04 08:38:42 PDT
does this new patch supercede the previous ones entirely?
Comment 21 Dão Gottwald [:dao] 2011-07-04 08:39:51 PDT
yes
Comment 22 Rob Campbell [:rc] (:robcee) 2011-07-06 07:03:20 PDT
Comment on attachment 543090 [details] [diff] [review]
simple CSS-only patch using box-shadow and outline

nice and simple. I like this.
Comment 23 Paul Rouget [:paul] 2011-07-07 05:41:21 PDT
One pixel should be enough.
Comment 24 Dietrich Ayala (:dietrich) 2011-07-08 07:50:18 PDT
Comment on attachment 543090 [details] [diff] [review]
simple CSS-only patch using box-shadow and outline

Review of attachment 543090 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 25 Dão Gottwald [:dao] 2011-07-11 15:21:10 PDT
http://hg.mozilla.org/mozilla-central/rev/127eaaa80f8d
Comment 26 Alfred Kayser 2011-07-13 00:33:33 PDT
Note, the patch attached and reviewed here is not the same as the patch committed to the tree... The difference is the width of the 'border': 1px instead of 2px...
Comment 27 jds2501 2011-11-25 08:37:42 PST
Created attachment 576948 [details]
Hard to see dashed background on black element with inspect button active

Bug still needs to remain opened. See the attached picture. If I have the "Inspect" button active and I select a dark element on the Mozilla developer page, the dashed border is hard to see. A fix to this issue could just reuse the same white and black dashed border used when the "Inspect" button isn't active.

Note You need to log in before you can comment on or make changes to this bug.