653545 - Add a page overlay that can be used by all tools with multiple capabilities (multi-highlight etc.)

Status: RESOLVED WONTFIX

(DevTools :: Inspector, defect)

Description

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

we'd like to be able to highlight multiple items in a webpage with the highlighter. We should implement an API call to do this and figure out the UI for it later.

Highlighting of multiple nodes (contained in a NodeList or array) should draw boxes around them and a very minimal set of annotations. Possibly just tag or ID.

Comment 1

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

note that this is for the actual highlighting of nodes, not the search field. This bug is to provide means of highlighting multiple items on the page.

Comment 2

[Paul Rouget [:paul]]

The current way to select a node is to have a transparent rectangle surrounded by a dark veil. Let's call this "Primary Selection". We need a "Secondary Selection" mechanism where we can select several nodes, but it doesn't have to be so "visible". Like simple dark-red squares.

We could use these secondary selections for:
. highlighting the result of the search
. once a node is locked, we could highlight other node being hovered, then a click would unlock the current node and highlight this new node (opera behavior)
. if a node is hovered in the breadcrumbs, we could highlight it

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Be careful when marking nodes as selected. If you put a line around a node when it is selected then it is not possible to properly see changes make to the node's borders. A line surrounding a node on selection is a common complaint about another browsers devtools for this reason.

Comment 4

[Paul Rouget [:paul]]

(In reply to comment #3)
> Be careful when marking nodes as selected. If you put a line around a node
> when it is selected then it is not possible to properly see changes make to
> the node's borders. A line surrounding a node on selection is a common
> complaint about another browsers devtools for this reason.

I don't think this is relevant for the Secondary Selection.
For the Primary Selection (what we have today) we draw a dashed outline. Do you think we should provide a way to hide this outline?

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

No, I think a dashed outline will work fine.

Comment 6

[Paul Rouget [:paul]]

This feature will be useful for the global command line to display the nodes matching the selector being written.

Comment 7

[Paul Rouget [:paul]]

Attachment: patch-A <input> - WIP

Comment 8

[Paul Rouget [:paul]]

Attachment: patch-B nodepicker - WIP

Comment 9

[Paul Rouget [:paul]]

Bug triage, filter on PINKISBEAUTIFUL

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The current highlighter.js file is not an independent module as it is tied in with the inspector in ways that it shouldn't be.

We need a shared overlay.js file that is completely independent of any other tool. This overlay could then be used for highlighting, multi-highlighting and displaying information for e.g. visual events although we will probably only be implementing multi-highlighting in this bug.

This shared overlay should also be available to extensions.

Comment 11

[Paul Rouget [:paul]]

Is this something we need today? What does this bug is supposed to cover exactly?

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We need a multi-highlighter and an overlay can cover this.

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Also a box model highlighter.

Whether we need it today is another question though.

Comment 14

[Paul Rouget [:paul]]

If we want to work on that, I recommend that we start with an implementation for Fennec and B2G taking advantage of the browser deck (see browser.xml).

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 1 WIP

(In reply to Paul Rouget [:paul] from comment #14)
> If we want to work on that, I recommend that we start with an implementation
> for Fennec and B2G taking advantage of the browser deck (see browser.xml).

Joe said to ignore that for now. Are you suggesting that we use the current method for local browsers but use the deck from browser.xul on mobile builds?

Check the bottom of BuiltinCommands.jsm and you will see a few new commands that I am using whilst working on this:
highlight start - Start inspection
highlight stop  - Stop inspection

multihighlight someselector - Highlights all items matching the selector

The overlay is not connected with any tools so it can be used by extensions etc. even when the toolbox is closed. The API is simple:

let target = devtools.TargetFactory.forTab(gBrowser.selectedTab);
let Overlay = require("devtools/shared/overlay").Overlay;
let overlay = new Overlay(target);

overlay.multiHighlight(nodes);
overlay.startInspecting();
overlay.stopInspecting();

Tools can listen using the selection API:
overlay.selection.on("new-node"); // new-node, attribute-changed, detached, pseudoclass

Of course, there is more to do here but Joe asked me to get feedback from Paul:
I still need to:
- Get the nodeinfo menu working ... it currently (and annoyingly) fails to receive mouse events so is fairly useless at the moment.
- Overlay closing and destruction.
- It would probably be best if extensions could choose their own colors for the overlay but I have not decided how this is best done yet.
- Adding box model highlighting would be very simple (a few hours work) ... if we do this then Firebug could use this overlay instead of injecting stuff into content.

paul: I think that what Joe wants to know is whether you think that this is going in the right direction as it is effectively highlighter v5(ish).

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Correct patch WIP

Comment 17

[Heather Arthur [:harth]]

UI-wise, this is what Brackets does: http://i.imgur.com/6k3iZu4.png

Comment 18

[Paul Rouget [:paul]]

This looks a lot like the highlighter code with multi-highlight support. If we want to re-write the highlighter, at least we should keep mobile and remotability in mind. And this code won't help. Here is what I think we should be doing here:

I don't think the overlay should depend on the target. Because we want to control it remotely as well, it should just be at the browser level and controlled by an actor. I believe that the overlay is a server-side code, not a front-end code (yeah, weird).

Also, I don't think we want to include the code of nodeinfobar in this. If we want to build something generic that can be used by anybody, we should focus on a tool that doesn't do too much. I would not replace the highlighter with the overlay, but rebuild the highlighter based on the overlay. 

Also, I don't think it should still depend on a selection object (this is not always available). We need something more generic.

This is how I would do define it:

An overlay is a layer on top of the page that has 2 functions:
- feedback: show one or several nodes
- input: let the user select a node

Its API should reflect that (and have some events).

The overlay should be accessible from any <browser> element and should be enabled from a browser.xml method.

I would do that at the XBL level (toolkit then).

The goal of the v1 would be to able to rebuild the original highlighter based on this overlay, and also have some code that could be used a mini-remotable-higlighter for Fennec.

The nodeinfobar would NOT be part of the overlay, but another layer controlled by Highlighter.js.

I would add multi-node selection right after that.

I would have different implementation for Firefox Desktop, Fennec and B2G. For example, we don't care about animation or scroll support in B2G. We can have a very very basic implementation for mobile.

An overlay actor should be available from a tab actor.

What do you think?

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Paul Rouget [:paul] from comment #18)
> I believe that the overlay is a server-side code,
> not a front-end code (yeah, weird).

I would phrase the right way of thinking about it as: anything that affects the toolbox frame is frontend, anything that affects the target's content frame is backend.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Paul Rouget [:paul] from comment #18)
> This looks a lot like the highlighter code with multi-highlight support. If
> we want to re-write the highlighter, at least we should keep mobile and
> remotability in mind. And this code won't help. Here is what I think we
> should be doing here:
> 

This was originally a bug to add multi-highlight support to the highlighter so that Heather can make use of it in the Style Editor. Moving it out and rebranding it the overlay clearly made sense for a more generic overlay system. It is basically highlighter.js with the selector moved inside and all references to the toolbox or inspector removed. I have also _prefixed lots more of the private stuff to clarify the API.

> I don't think the overlay should depend on the target. Because we want to
> control it remotely as well, it should just be at the browser level and
> controlled by an actor. I believe that the overlay is a server-side code,
> not a front-end code (yeah, weird).
> 

Agreed.

> Also, I don't think we want to include the code of nodeinfobar in this. If
> we want to build something generic that can be used by anybody, we should
> focus on a tool that doesn't do too much. I would not replace the
> highlighter with the overlay, but rebuild the highlighter based on the
> overlay. 
> 

I figured that the nodeinfobar would be useful for most tools that use "inspection mode" would benefit from the nodeinfobar as it makes the highlight more visible but for reasons of genericism and customization I think you are right.

Maybe we should have some way for tools to add stuff here.

> Also, I don't think it should still depend on a selection object (this is
> not always available). We need something more generic.
> 

Really? When is the selection object not available?

> This is how I would do define it:
> 
> An overlay is a layer on top of the page that has 2 functions:
> - feedback: show one or several nodes

overlay.highlight(node|nodes)

> - input: let the user select a node
overlay.inspectStart()
overlay.inspectStop()

> 
> Its API should reflect that (and have some events).
> 

Yes, although I think that feedback and input are too ambiguous for API function names.

The event names should also be much clearer, it currently uses the events from selection.js

> The overlay should be accessible from any <browser> element and should be
> enabled from a browser.xml method.
> 
> I would do that at the XBL level (toolkit then).
> 

Agreed, I always planned on moving it into toolkit.

> The goal of the v1 would be to able to rebuild the original highlighter
> based on this overlay, and also have some code that could be used a
> mini-remotable-higlighter for Fennec.
> 

Yup

> The nodeinfobar would NOT be part of the overlay, but another layer
> controlled by Highlighter.js.
> 

Of course

> I would add multi-node selection right after that.
> 

Agreed

> I would have different implementation for Firefox Desktop, Fennec and B2G.
> For example, we don't care about animation or scroll support in B2G. We can
> have a very very basic implementation for mobile.
> 

They would still be very similar and three completely different implementations would be difficult to maintain. One implementation with three modes makes more sense to me.

> An overlay actor should be available from a tab actor.
> 

Yup, that makes sense.

> What do you think?

I will remove the nodeinfobar again. *If* Selection is not always available then I guess it should either be moved into the overlay itself of made available. I can then move it into toolkit, make it remotable and wire up the highlighter.

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We no longer need this.

Comment 22

[Widartongawinganjuk]

[Tracking Requested - why for this release]: