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

RESOLVED FIXED

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [minotaur][fixed-in-fx-team] )

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Updated

6 years ago
Blocks: 663830
(Assignee)

Updated

6 years ago
No longer depends on: 669652
(Assignee)

Comment 1

6 years ago
Created attachment 544486 [details] [diff] [review]
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().
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #544486 - Flags: review?(rcampbell)
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.
Attachment #544486 - Flags: review?(rcampbell) → review+
ps, do the unittests still pass with the stopInspecting call removed?
(Assignee)

Comment 4

6 years ago
Created attachment 544814 [details] [diff] [review]
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.
Attachment #544486 - Attachment is obsolete: true
Attachment #544814 - Flags: review?(dao)
(Assignee)

Comment 5

6 years ago
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.
Attachment #544814 - Flags: review?(dao)
(Assignee)

Comment 6

6 years ago
Created attachment 546013 [details] [diff] [review]
patch v1.3

Change the style of the dashed  border in the highlighter and change the color of the selection in the HTML Inspector.
Attachment #544814 - Attachment is obsolete: true
Attachment #546013 - Flags: review?(rcampbell)
(Assignee)

Comment 7

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

Sorry, made a mistake in the previous patch.
Attachment #546013 - Attachment is obsolete: true
Attachment #546027 - Flags: review?(rcampbell)
Attachment #546013 - Flags: review?(rcampbell)
(Assignee)

Updated

6 years ago
Blocks: 671689
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?
Attachment #546027 - Flags: review-

Updated

6 years ago
Whiteboard: [minotaur]
(Assignee)

Comment 9

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

Comment 10

6 years ago
Created attachment 547661 [details] [diff] [review]
patch v1.5

Use a "locked" attribute instead of a class.
I removed the HTML panel related code (will do that in another bug).
Attachment #546027 - Attachment is obsolete: true
Attachment #547661 - Flags: review?(dao)
Attachment #546027 - Flags: review?(rcampbell)
Is there a downside to setting the attribute directly on highlighter-veil-transparentbox?
(Assignee)

Comment 12

6 years ago
Not really. But we will have to move it back to highlighterContainer later (which is not a problem).
I'm asking because the descendent selector should be avoided. https://developer.mozilla.org/en/Writing_Efficient_CSS#Avoid_the_descendant_selector
(Assignee)

Comment 14

6 years ago
Created attachment 547732 [details] [diff] [review]
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.
Attachment #547661 - Attachment is obsolete: true
Attachment #547732 - Flags: review?(dao)
Attachment #547661 - Flags: review?(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.
Attachment #547732 - Flags: review?(dao) → review+
(Assignee)

Comment 16

6 years ago
Created attachment 547739 [details] [diff] [review]
[checked-in] patch v1.7

Thanks for the r+ Dao.
(Assignee)

Updated

6 years ago
Whiteboard: [minotaur] → [minotaur][land-in-fx-team]
landed in fx-team:

http://hg.mozilla.org/integration/fx-team/rev/ccb986bb04e1
Whiteboard: [minotaur][land-in-fx-team] → [minotaur][fixed-in-fx-team]
Comment on attachment 547739 [details] [diff] [review]
[checked-in] patch v1.7

http://hg.mozilla.org/mozilla-central/rev/ccb986bb04e1
Attachment #547739 - Attachment description: patch v1.7 → [checked-in] patch v1.7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.