Closed Bug 770818 Opened 9 years ago Closed 9 years ago

[inspector] highlighter v3

Categories

(DevTools :: Inspector, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 17

People

(Reporter: paul, Assigned: paul)

References

Details

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

Attachments

(2 files, 4 obsolete files)

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).
Duplicate of this bug: 725722
Assignee: nobody → bdils
Attached image UX Spec for 770818
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.
(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.
Brian, any good reason to keep the veil?
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...
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
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
(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.
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.
Assignee: bdils → paul
(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
Blocks: 663778
Attached patch v0.1 (obsolete) — Splinter Review
build to come
Todo:
- tests
Attached patch v1 (obsolete) — Splinter Review
with tests
Attachment #650857 - Attachment is obsolete: true
Todo:
- some code has been deleted. I need to make sure nothing is left in locales/ & themes/
- try
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #651805 - Attachment is obsolete: true
Summary: [inspector] hide the veil → [inspector] highlighter v3
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.
QA Contact: paul
QA Contact: paul
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 :)
Attachment #652050 - Flags: feedback?(rcampbell)
Attachment #652050 - Flags: review?(jwalker)
Attachment #652050 - Flags: review?(jwalker) → review+
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?
(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.
Duplicate of this bug: 734906
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #652050 - Attachment is obsolete: true
Attachment #652050 - Flags: feedback?(rcampbell)
Whiteboard: [land-in-fx-team]
Whiteboard: [land-in-fx-team]
Attached patch v1.2Splinter Review
rebased
Attachment #654682 - Attachment is obsolete: true
Just realized there's an orange with WinXP.
Apparently, it's fixed.
Ready to land!
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/cbaa56e4bebb
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Orange.

I'll see if there's an obvious fix. Otherwise, I'll back this out.
Backed out:  https://hg.mozilla.org/integration/fx-team/rev/aeb96f8edcb8
Whiteboard: [fixed-in-fx-team]
Can't reproduce here. I'll use try to experiment some ideas.

First round: https://tbpl.mozilla.org/?tree=Try&rev=99e162ee6721
Duplicate of this bug: 785633
Apparently, it worked. Another try with all the platforms: https://tbpl.mozilla.org/?tree=Try&rev=8e4bfc8f0c7f
It's green. Let's reland that.
https://hg.mozilla.org/mozilla-central/rev/c4d19217db8d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Depends on: 808520
Depends on: 936516
Depends on: 941200
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.