663781 - [highlighter] Once a node is locked, the black background should be removed

Status: RESOLVED WONTFIX

(DevTools :: General, defect)

Description

[Paul Rouget [:paul]]

This black veil around the selected node prevents the user to have an accurate view of the web page he's debugging.

Once the node is locked, the veil should disappear.

Comment 1

[Paul Rouget [:paul]]

Attachment: Remove the veil, add a selection box

Comment 2

[Paul Rouget [:paul]]

Attachment: Remove the veil, add a selection box - v1.1

Remove support for "transform" (the way I do it is not working)

Comment 3

[Paul Rouget [:paul]]

Attachment: Remove the veil, add a selection box - v1.2

Forgot some comments :)

Comment 4

[Paul Rouget [:paul]]

Attachment: Remove the veil, add a selection box - v1.3

The selection was too large. fixed in this patch.

Comment 5

[Mihai Sucan [:msucan]]

Comment on attachment 538931 [details] [diff] [review]
Remove the veil, add a selection box - v1.3

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

The patch is fine, but I have some comments:

- I am not sure if we really need another element to highlight the node once it's locked.
- we need a solution to nicely highlight the element such that it works (looks good) both on dark and bright web sites. Once we have that we may reuse it instead of a separate element. Thoughts? Is it actually possible what I am thinking of?

For now the approach you have in the patch is good. You just need to properly deal with the iframes and write a test for this locking mechanism. Thank you!

::: browser/base/content/inspector.js
@@ +357,5 @@
> +    let style = this.win.getComputedStyle(this.node, null);
> +
> +    let bbox_rect = this._highlightOriginalRect;
> +    this.bboxDiv.style.top = bbox_rect.top - this.bboxDiv.clientTop + "px";
> +    this.bboxDiv.style.left = bbox_rect.left - this.bboxDiv.clientLeft + "px";

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.

Comment 6

[Paul Rouget [:paul]]

(In reply to comment #5)
> - I am not sure if we really need another element to highlight the node once
> it's locked.
> - we need a solution to nicely highlight the element such that it works
> (looks good) both on dark and bright web sites. Once we have that we may
> reuse it instead of a separate element. Thoughts? Is it actually possible
> what I am thinking of?

Yes.

I like this black&white dashed border, and I think this should be visible while inspecting.

So in the next version of the patch, I will only remove the black veil, and I won't include this black & white border, and I'll re-introduce it in bug 663898 ("[highlighter] The dark veil is not visible on dark background"), and there, I'll take you comments into account.

Comment 7

[Paul Rouget [:paul]]

WONTFIX because hiding the black veil will be possible from the toolbar. See bug 663833.