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

RESOLVED WONTFIX

Status

()

Firefox
Developer Tools: Inspector
RESOLVED WONTFIX
6 years ago
a year ago

People

(Reporter: rc, Assigned: miker)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [highlighter])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
Depends on: 642471
(Reporter)

Comment 1

6 years ago
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.
Blocks: 663830, 650804

Comment 2

6 years ago
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
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

6 years ago
(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?
No, I think a dashed outline will work fine.

Updated

6 years ago
Whiteboard: [best: 1d, likely: 2d, worst: 5d][highlighter] → [highlighter]

Comment 6

6 years ago
This feature will be useful for the global command line to display the nodes matching the selector being written.
Blocks: 697043

Updated

6 years ago
Depends on: 703031

Updated

6 years ago
Blocks: 703383

Updated

6 years ago
Component: Developer Tools → Developer Tools: Inspector
QA Contact: developer.tools → developer.tools.inspector

Comment 7

6 years ago
Created attachment 579336 [details] [diff] [review]
patch-A <input> - WIP

Comment 8

6 years ago
Created attachment 579337 [details] [diff] [review]
patch-B nodepicker - WIP

Updated

6 years ago
Priority: -- → P3

Comment 9

5 years ago
Bug triage, filter on PINKISBEAUTIFUL
Priority: P3 → --
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
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.
Summary: Create a means to highlight multiple items on a single webpage (highlighter) → Add a page overlay that can be used by all tools with multiple capabilities (multi-highlight etc.)

Comment 11

4 years ago
Is this something we need today? What does this bug is supposed to cover exactly?
We need a multi-highlighter and an overlay can cover this.
Also a box model highlighter.

Whether we need it today is another question though.

Comment 14

4 years ago
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).
Created attachment 764178 [details] [diff] [review]
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).
Attachment #579336 - Attachment is obsolete: true
Attachment #579337 - Attachment is obsolete: true
Attachment #764178 - Flags: feedback?(paul)
Created attachment 764179 [details] [diff] [review]
Correct patch WIP
Attachment #764178 - Attachment is obsolete: true
Attachment #764178 - Flags: feedback?(paul)
Attachment #764179 - Flags: feedback?(paul)
UI-wise, this is what Brackets does: http://i.imgur.com/6k3iZu4.png

Comment 18

4 years ago
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?

Updated

4 years ago
Attachment #764179 - Flags: feedback?(paul)
(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.
(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.
Blocks: 736078
Blocks: 895880

Updated

4 years ago
Blocks: 663778
We no longer need this.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
[Tracking Requested - why for this release]:
tracking-e10s: --- → ?
Flags: a11y-review+
tracking-e10s: ? → ---
Flags: a11y-review+
You need to log in before you can comment on or make changes to this bug.