Last Comment Bug 669656 - [highlighter] Once a node is locked, the highlighter should give a visual feedback
: [highlighter] Once a node is locked, the highlighter should give a visual fee...
Status: RESOLVED FIXED
[minotaur][fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Paul Rouget [:paul]
:
Mentors:
Depends on: 663898
Blocks: 663830 671689
  Show dependency treegraph
 
Reported: 2011-07-06 08:49 PDT by Paul Rouget [:paul]
Modified: 2011-07-25 06:47 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (2.85 KB, patch)
2011-07-07 07:55 PDT, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Splinter Review
patch v1.1 (2.59 KB, patch)
2011-07-08 07:47 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.3 (6.91 KB, patch)
2011-07-14 15:16 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.4 (6.78 KB, patch)
2011-07-14 16:01 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.5 (1.94 KB, patch)
2011-07-22 04:44 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.6 (1.89 KB, patch)
2011-07-22 10:11 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review
[checked-in] patch v1.7 (2.70 KB, patch)
2011-07-22 10:42 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-07-06 08:49:52 PDT
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] 2011-07-07 07:55:33 PDT
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().
Comment 2 Rob Campbell [:rc] (:robcee) 2011-07-07 13:04:30 PDT
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) 2011-07-07 13:04:58 PDT
ps, do the unittests still pass with the stopInspecting call removed?
Comment 4 Paul Rouget [:paul] 2011-07-08 07:47:47 PDT
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.
Comment 5 Paul Rouget [:paul] 2011-07-14 14:40:20 PDT
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] 2011-07-14 15:16:20 PDT
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.
Comment 7 Paul Rouget [:paul] 2011-07-14 16:01:35 PDT
Created attachment 546027 [details] [diff] [review]
patch v1.4

Sorry, made a mistake in the previous patch.
Comment 8 Dão Gottwald [:dao] 2011-07-16 10:24:59 PDT
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] 2011-07-22 04:11:52 PDT
(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] 2011-07-22 04:44:42 PDT
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).
Comment 11 Dão Gottwald [:dao] 2011-07-22 08:45:49 PDT
Is there a downside to setting the attribute directly on highlighter-veil-transparentbox?
Comment 12 Paul Rouget [:paul] 2011-07-22 09:03:31 PDT
Not really. But we will have to move it back to highlighterContainer later (which is not a problem).
Comment 13 Dão Gottwald [:dao] 2011-07-22 09:12:40 PDT
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] 2011-07-22 10:11:59 PDT
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.
Comment 15 Dão Gottwald [:dao] 2011-07-22 10:29:10 PDT
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] 2011-07-22 10:42:36 PDT
Created attachment 547739 [details] [diff] [review]
[checked-in] patch v1.7

Thanks for the r+ Dao.
Comment 17 Rob Campbell [:rc] (:robcee) 2011-07-22 12:21:00 PDT
landed in fx-team:

http://hg.mozilla.org/integration/fx-team/rev/ccb986bb04e1
Comment 18 Rob Campbell [:rc] (:robcee) 2011-07-25 06:47:03 PDT
Comment on attachment 547739 [details] [diff] [review]
[checked-in] patch v1.7

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

Note You need to log in before you can comment on or make changes to this bug.