Last Comment Bug 770818 - [inspector] highlighter v3
: [inspector] highlighter v3
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 17
Assigned To: Paul Rouget [:paul]
:
Mentors:
: 725722 734906 785633 (view as bug list)
Depends on: 808520 936516 941200
Blocks: 663778
  Show dependency treegraph
 
Reported: 2012-07-04 02:23 PDT by Paul Rouget [:paul]
Modified: 2013-12-03 06:57 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
UX Spec for 770818 (2.10 MB, image/jpeg)
2012-07-26 08:42 PDT, Dils
no flags Details
v0.1 (25.73 KB, patch)
2012-08-10 05:52 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
v1 (34.57 KB, patch)
2012-08-14 10:00 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
v1.1 (48.92 KB, patch)
2012-08-15 02:53 PDT, Paul Rouget [:paul]
jwalker: review+
Details | Diff | Splinter Review
v1.2 (49.16 KB, patch)
2012-08-23 10:28 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
v1.2 (49.15 KB, patch)
2012-08-24 05:18 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2012-07-04 02:23:06 PDT
We should only show the veil when:
- the user is hovering the page to select a node
- the user is selecting nodes in the breadcrumbs
- the user is hovering the infobar
- the user is using the Markup View

That might mean that we also need to change the "look" of the selection (the dashed rectangle around the selected node).
Comment 1 Paul Rouget [:paul] 2012-07-04 02:25:07 PDT
*** Bug 725722 has been marked as a duplicate of this bug. ***
Comment 2 Dils 2012-07-26 08:42:20 PDT
Created attachment 646154 [details]
UX Spec for 770818
Comment 3 Dils 2012-07-26 08:42:35 PDT
See the attached JPG for recommended UI changes.

Paul and I  discussed a few other, more aggressive changes to the UI, but since the UX team is working on a new UI structure for all the dev tools as a whole, I didn't want to recommend features that could end up being changed after a big development effort.

Just so we can keep track, here are some future UX ideas I want to explore for the Inspector:

#1: When the user is hover over the selected element, a zoomed in view of the element can be seen in the layout view with the measurements overlaid on top

#2 Add a button for a zoom tool to the Node Info UI

#3 Add zoom tool to the Inspector toolbar

#4 When the user is hovering over the various parts of the element in layout view, such as margin, border, etc the corresponding areas on the live element on the page are highlighted while the rest of the page is dimmed.
Comment 4 Paul Rouget [:paul] 2012-07-26 09:50:07 PDT
(In reply to Dils from comment #3)
> See the attached JPG for recommended UI changes.

I don't have any comment on this. Looks right to me.

> #1: When the user is hover over the selected element, a zoomed in view of
> the element can be seen in the layout view with the measurements overlaid on
> top
> 
> #2 Add a button for a zoom tool to the Node Info UI
> 
> #3 Add zoom tool to the Inspector toolbar
> 
> #4 When the user is hovering over the various parts of the element in layout
> view, such as margin, border, etc the corresponding areas on the live
> element on the page are highlighted while the rest of the page is dimmed.

We need specific bugs for that. Also, let's not discuss this on bugzilla (we prefer ml discussion for feature definition).

See bug 731652 and bug 663778 tool.
Comment 5 Paul Rouget [:paul] 2012-07-26 12:57:39 PDT
Brian, any good reason to keep the veil?
Comment 6 Dils 2012-07-26 13:23:19 PDT
I mean the veil looks great, but I don't see needing both the veil and the dashed border. I can see it working well for idea #4 listed above...
Comment 7 Paul Rouget [:paul] 2012-07-27 11:42:36 PDT
Brian, please test this build (we need to wait a bit before the builds the show up):
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/prouget@mozilla.com-61dc79b156a8

Changes:
- no veil
- removed the gear menu
- outline is "strong" while highlighter unlocked. outline is faded when locked.
- outline is hidden when the user overs the sidebar.
- infobar bar is moved up and faded out when the user overs the sidebar.

Fading out the infobar feels weird. It works if there's no content behind it, but if there is some text or any image, it looks messy: http://i.imgur.com/9NEjZ.png
Comment 8 Dils 2012-07-28 13:28:15 PDT
Thoughts on the changes:

- no veil
  * I think this works well, I'd say keep this setting

- removed the gear menu
  * Works for me. Lets pay attention to whether users complain about the veil disappearing

- outline is "strong" while highlighter unlocked. outline is faded when locked.
  * Works for me as well
- outline is hidden when the user overs the sidebar.
  * I think works too

- infobar bar is moved up and faded out when the user overs the sidebar.
  * I think we should either slide the infobar away or use fading. Personally I really like the sliding effect you built. I'd suggesting keeping the sliding action and not fading the infobar
Comment 9 Paul Rouget [:paul] 2012-08-09 02:56:16 PDT
(In reply to Dils from comment #8)
> - removed the gear menu
>   * Works for me. Lets pay attention to whether users complain about the
>     veil disappearing

Ok. I'd like to have another approval for that.
Kevin, what do you think?

> - infobar bar is moved up and faded out when the user overs the sidebar.
>   * I think we should either slide the infobar away or use fading.
>     Personally I really like the sliding effect you built. I'd suggesting
>     keeping the sliding action and not fading the infobar

Understood.
Comment 10 Paul Rouget [:paul] 2012-08-09 06:20:32 PDT
Just talked to Kevin:
1) we get rid of the veil
2) we get rid of the gear-menu
3) we totally hide the infobar while editing the style of the page

About 3), people want to have a "clean" page while editing code. Not having a way to remove the infobar (because we remove the gear-menu) might my a real problem.

I'll bake something, and make a build available, and we'll see how it feels.
Comment 11 Kevin Dangoor 2012-08-09 06:25:39 PDT
(In reply to Paul Rouget [:paul] from comment #9)
> (In reply to Dils from comment #8)
> > - removed the gear menu
> >   * Works for me. Lets pay attention to whether users complain about the
> >     veil disappearing
> 
> Ok. I'd like to have another approval for that.
> Kevin, what do you think?

I tried the build out. I very much like the removal of the gear menu, and I like the behavior when I hover over the style panel. Feedback that we've gotten (which partly led to the gear menu in the first place) is that designers want to see their designs without any distractions while they're trying to figure out what changes to make, so I think the infobar should go away as well when styling.

Paul and I discussed this a bit on IRC. Paul said he could try using the animation in this build to fade out the infobar entirely rather than just fading it to some low opacity.

The veil is visually distinctive and has certainly gotten attention for our tools. However, it does get in the way for designers. Paul also tried fading out the veil, but described that variation as "annoying". The veil is a bit too heavy to fade in and out frequently.

Eliminating modes while keeping things useful for everyone seems like a win. bug 663778 will be a useful addition to this, because then the node will have a bit more highlighting to it than just a small dashed line.

tldr; my opinion: overall good, make the infobar fade away completely rather than mostly
Comment 12 Paul Rouget [:paul] 2012-08-10 05:52:14 PDT
Created attachment 650857 [details] [diff] [review]
v0.1

build to come
Comment 14 Paul Rouget [:paul] 2012-08-10 06:08:18 PDT
Todo:
- tests
Comment 15 Paul Rouget [:paul] 2012-08-14 10:00:08 PDT
Created attachment 651805 [details] [diff] [review]
v1

with tests
Comment 16 Paul Rouget [:paul] 2012-08-14 10:01:10 PDT
Todo:
- some code has been deleted. I need to make sure nothing is left in locales/ & themes/
- try
Comment 17 Paul Rouget [:paul] 2012-08-15 02:53:40 PDT
Created attachment 652050 [details] [diff] [review]
v1.1
Comment 18 Paul Rouget [:paul] 2012-08-15 03:00:25 PDT
This patch goes beyond just removing the veil:

- removed the option menu in the toolbar
- removed the veil
- hide the infobar + outline the mouse is overring the rule view
- only one reflow when moving the selection (performance++)
- ignore page events while inspecting (performance+++++)
- inversed outline contrast: before it was low contrast while inspecting and high contrast once locked. Now it's the other way around.
Comment 19 Paul Rouget [:paul] 2012-08-15 03:14:49 PDT
Comment on attachment 652050 [details] [diff] [review]
v1.1

Rob, can you try this patch and tell me what you think about this new way to highlight nodes.

And a quick code review won't hurt :)
Comment 20 Paul Rouget [:paul] 2012-08-15 03:15:44 PDT
https://tbpl.mozilla.org/?tree=Try&rev=6171571117e3
Comment 21 Dão Gottwald [:dao] 2012-08-15 08:23:17 PDT
Comment on attachment 652050 [details] [diff] [review]
v1.1

> <!-- LOCALIZATION NOTE: These are for the menu in the Inspector Toolbar -->
> <!ENTITY inspectorToggleVeil.label          "Dim The Page">
> <!ENTITY inspectorToggleVeil.accesskey      "D">
> <!ENTITY inspectorToggleInfobar.label       "Show Node Info">
> <!ENTITY inspectorToggleInfobar.accesskey   "S">

What's the plan for these strings?
Comment 22 Paul Rouget [:paul] 2012-08-15 08:27:49 PDT
(In reply to Dão Gottwald [:dao] from comment #21)
> Comment on attachment 652050 [details] [diff] [review]
> v1.1
> 
> > <!-- LOCALIZATION NOTE: These are for the menu in the Inspector Toolbar -->
> > <!ENTITY inspectorToggleVeil.label          "Dim The Page">
> > <!ENTITY inspectorToggleVeil.accesskey      "D">
> > <!ENTITY inspectorToggleInfobar.label       "Show Node Info">
> > <!ENTITY inspectorToggleInfobar.accesskey   "S">
> 
> What's the plan for these strings?

I knew I'd miss something :) Thank you Dao.
Comment 23 Paul Rouget [:paul] 2012-08-17 02:19:39 PDT
*** Bug 734906 has been marked as a duplicate of this bug. ***
Comment 24 Paul Rouget [:paul] 2012-08-23 10:28:56 PDT
Created attachment 654682 [details] [diff] [review]
v1.2
Comment 25 Paul Rouget [:paul] 2012-08-24 05:18:56 PDT
Created attachment 654982 [details] [diff] [review]
v1.2

rebased
Comment 26 Paul Rouget [:paul] 2012-08-24 05:41:14 PDT
Just realized there's an orange with WinXP.
Comment 27 Paul Rouget [:paul] 2012-08-24 06:14:48 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=beeeef68ee29
Comment 28 Paul Rouget [:paul] 2012-08-25 05:49:48 PDT
Apparently, it's fixed.
Ready to land!
Comment 30 Paul Rouget [:paul] 2012-08-25 09:31:54 PDT
Orange.

I'll see if there's an obvious fix. Otherwise, I'll back this out.
Comment 31 Paul Rouget [:paul] 2012-08-25 11:07:22 PDT
Backed out:  https://hg.mozilla.org/integration/fx-team/rev/aeb96f8edcb8
Comment 32 Paul Rouget [:paul] 2012-08-25 11:08:00 PDT
Can't reproduce here. I'll use try to experiment some ideas.

First round: https://tbpl.mozilla.org/?tree=Try&rev=99e162ee6721
Comment 33 Paul Rouget [:paul] 2012-08-25 11:08:08 PDT
*** Bug 785633 has been marked as a duplicate of this bug. ***
Comment 34 Paul Rouget [:paul] 2012-08-25 19:09:04 PDT
Apparently, it worked. Another try with all the platforms: https://tbpl.mozilla.org/?tree=Try&rev=8e4bfc8f0c7f
Comment 35 Paul Rouget [:paul] 2012-08-26 05:50:37 PDT
It's green. Let's reland that.
Comment 37 Paul Rouget [:paul] 2012-08-26 15:09:01 PDT
https://hg.mozilla.org/mozilla-central/rev/c4d19217db8d

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