669656 - [highlighter] Once a node is locked, the highlighter should give a visual feedback

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Paul Rouget [:paul]]

When the user locks a node, the highlighter should give a visual feedback. A different style (colors?) for the selection box could be a good approach.

Comment 1

[Paul Rouget [:paul]]

Attachment: patch v1

The approach here is to make the selection box borders half transparent while inspecting, and totally opaque while locked. The difference is not big, but it should be enough.

Also, I removed the stopInspecting call in closeInspectorUI, because it would call onlock, which is useless. We called stopInspecting because it was removing some listeners. So I removed stopInspecting and add instead detachPageListeners().

Comment 2

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

Comment on attachment 544486 [details] [diff] [review]
patch v1

Naming nit:

Not sure I like the onlock and onunlock method names. They suggest that they're callbacks but they're being called directly. Maybe rename them locked and unlocked?

Further, since you're just adding and removing classList entries in onlock/unlock, maybe just do that directly instead of calling a full method?

I decoded the CSS and think it looks reasonable. In my local version though, I have outline: 2px and offset: -2px. I think that would look better and be slightly more visible than a 1px half-transparent border.

r+ with the methods removed.

Comment 3

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

ps, do the unittests still pass with the stopInspecting call removed?

Comment 4

[Paul Rouget [:paul]]

Attachment: patch v1.1

Addressed Rob's comments.

I think the selection border should be 1px, not 2px. I just feel like 2px is too "rough". We are delimiting a surface here, and 1px is what all the designer tools do.

Comment 5

[Paul Rouget [:paul]]

Comment on attachment 544814 [details] [diff] [review]
patch v1.1

We should add the same kind of feedback in the HTML panel as well. Working on it.

Comment 6

[Paul Rouget [:paul]]

Attachment: patch v1.3

Change the style of the dashed  border in the highlighter and change the color of the selection in the HTML Inspector.

Comment 7

[Paul Rouget [:paul]]

Attachment: patch v1.4

Sorry, made a mistake in the previous patch.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 546027 [details] [diff] [review]
patch v1.4

>+    this.highlighter.highlighterContainer.classList.add("locked");

Classes should be reusable and distinctive in mozilla code (i.e. using .locked with no id put in front would need to make sense throughout -- at least -- browser.xul). Here you should set an attribute rather than a class since you only want to put this particular element in a certain mode.

Also, why is this highlighterContainer rather than veilTransparentBox?

Comment 9

[Paul Rouget [:paul]]

(In reply to comment #8)
> Comment on attachment 546027 [details] [diff] [review] [review]
> patch v1.4
> 
> >+    this.highlighter.highlighterContainer.classList.add("locked");
> 
> Classes should be reusable and distinctive in mozilla code (i.e. using
> .locked with no id put in front would need to make sense throughout -- at
> least -- browser.xul). Here you should set an attribute rather than a class
> since you only want to put this particular element in a certain mode.

Will do.

> Also, why is this highlighterContainer rather than veilTransparentBox?

In the future, some other elements in the controls box could be affected too (we are working on a floating info bar lying in the controls box, and its style will be affected too).

Comment 10

[Paul Rouget [:paul]]

Attachment: patch v1.5

Use a "locked" attribute instead of a class.
I removed the HTML panel related code (will do that in another bug).

Comment 11

[Dão Gottwald [:dao]]

Is there a downside to setting the attribute directly on highlighter-veil-transparentbox?

Comment 12

[Paul Rouget [:paul]]

Not really. But we will have to move it back to highlighterContainer later (which is not a problem).

Comment 13

[Dão Gottwald [:dao]]

I'm asking because the descendent selector should be avoided. https://developer.mozilla.org/en/Writing_Efficient_CSS#Avoid_the_descendant_selector

Comment 14

[Paul Rouget [:paul]]

Attachment: patch v1.6

(In reply to comment #13)
> I'm asking because the descendent selector should be avoided.
> https://developer.mozilla.org/en/
> Writing_Efficient_CSS#Avoid_the_descendant_selector

I see. Patch updated.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 547732 [details] [diff] [review]
patch v1.6

>   outline: 1px dashed rgba(255,255,255,0.5);
>   outline-offset: -1px;
> }
>+
>+#highlighter-veil-transparentbox[locked] {
>+  box-shadow: 0 0 0 1px rgba(0,0,0,1);
>+  outline: 1px dashed rgba(255,255,255,1);
>+}

"rgba(0,0,0,1)" should just be "black".
"outline: 1px dashed rgba(255,255,255,1)" should just be "outline-color: white".

r=me with that

By the way, please use 8 lines context in patches.

Comment 16

[Paul Rouget [:paul]]

Attachment: [checked-in] patch v1.7

Thanks for the r+ Dao.

Comment 17

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

landed in fx-team:

http://hg.mozilla.org/integration/fx-team/rev/ccb986bb04e1

Comment 18

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

Comment on attachment 547739 [details] [diff] [review]
[checked-in] patch v1.7

http://hg.mozilla.org/mozilla-central/rev/ccb986bb04e1