663898 - [highlighter] make sure the selected node is identifiable even on very dark and very light backgrounds

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Paul Rouget [:paul]]

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]]

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]]

Attachment: 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]]

Attachment: patch v1.1

Comment 4

[Mihai Sucan [:msucan]]

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]]

Attachment: patch v1.2

Comment 6

[Paul Rouget [:paul]]

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]]

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]]

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)]

(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)]

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]]

Attachment: patch v1.4

Addressed comment 8 and comment 10.

Comment 12

[Paul Rouget [:paul]]

Attachment: patch v1.4.1 (rebase)

Comment 13

[Dão Gottwald [:dao]]

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]]

(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]]

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]]

(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]]

(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]]

Attachment: 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]]

(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)]

does this new patch supercede the previous ones entirely?

Comment 21

[Dão Gottwald [:dao]]

yes

Comment 22

[Rob Campbell [:rc] (:robcee)]

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]]

One pixel should be enough.

Comment 24

[Dietrich Ayala (:dietrich)]

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]]

http://hg.mozilla.org/mozilla-central/rev/127eaaa80f8d

Comment 26

[Alfred Kayser]

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]

Attachment: 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.