Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 8

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: paul, Assigned: dao)

Tracking

unspecified
Firefox 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
For example, if we highlight any element in the header of http://developer.mozilla.org, we can't see the veil.
(Reporter)

Updated

6 years ago
Assignee: nobody → paul
(Reporter)

Comment 1

6 years ago
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.
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Comment 3

6 years ago
Created attachment 540463 [details] [diff] [review]
patch v1.1
Attachment #540449 - Attachment is obsolete: true
Attachment #540463 - Flags: review?(mihai.sucan)
(Reporter)

Updated

6 years ago
Depends on: 664436
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)
(Reporter)

Comment 5

6 years ago
Created attachment 540763 [details] [diff] [review]
patch v1.2
Attachment #540463 - Attachment is obsolete: true
Attachment #540463 - Flags: review?(mihai.sucan)
(Reporter)

Comment 6

6 years ago
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 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.
Attachment #540763 - Flags: review+
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?)
(Reporter)

Updated

6 years ago
Summary: [highlighter] The dark veil is not visible on dark background → [highlighter] make sure the selected node is identifiable even on very dark and very light backgrounds
(Reporter)

Updated

6 years ago
Attachment #540763 - Flags: review?(dietrich)
(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 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).
Attachment #540763 - Flags: review?(dietrich) → review+
(Reporter)

Comment 11

6 years ago
Created attachment 542165 [details] [diff] [review]
patch v1.4

Addressed comment 8 and comment 10.
(Reporter)

Comment 12

6 years ago
Created attachment 542478 [details] [diff] [review]
patch v1.4.1 (rebase)
(Assignee)

Comment 13

6 years ago
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.
(Reporter)

Comment 14

6 years ago
(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).
(Assignee)

Comment 15

6 years ago
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.
(Reporter)

Comment 16

6 years ago
(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.
(Assignee)

Comment 17

6 years ago
(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.
(Assignee)

Comment 18

6 years ago
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.
(Reporter)

Comment 19

6 years ago
(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.
(Assignee)

Updated

6 years ago
Attachment #543090 - Flags: review?(dietrich)
does this new patch supercede the previous ones entirely?
Status: NEW → ASSIGNED
(Assignee)

Comment 21

6 years ago
yes
Attachment #542478 - Attachment is obsolete: true
Attachment #542165 - Attachment is obsolete: true
Attachment #540763 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Assignee: paul → dao
(Assignee)

Updated

6 years ago
OS: Linux → All
Hardware: x86 → All
Comment on attachment 543090 [details] [diff] [review]
simple CSS-only patch using box-shadow and outline

nice and simple. I like this.
Attachment #543090 - Flags: review+
(Reporter)

Updated

6 years ago
Blocks: 669656
(Reporter)

Comment 23

6 years ago
One pixel should be enough.
Comment on attachment 543090 [details] [diff] [review]
simple CSS-only patch using box-shadow and outline

Review of attachment 543090 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #543090 - Flags: review?(dietrich) → review+
(Assignee)

Comment 25

6 years ago
http://hg.mozilla.org/mozilla-central/rev/127eaaa80f8d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8

Comment 26

6 years ago
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

6 years ago
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.
You need to log in before you can comment on or make changes to this bug.