1646028 - Make the BoxModelHighlighter compatible with Fission

Status: RESOLVED FIXED

(DevTools :: Inspector, task, P1)

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1646028 - Update tests for box model highlighter consumers in Inspector

Depends on D81528

Comment 2

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1646028 - Update consumers of box model highlighter in Inspector

Depends on D81528

Comment 3

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1646028 - Add generic highlighter manager to HighlightersOverlay. r=jdescottes,ochameau

Depends on D81525

Introduced a centralized way in HighlightersOverlay to invoke highlighters by type, automatically managing previously active highlighters.

First some context.
HighlightersOverlay is a bit of a misnomer. It already does a few things:

• provides methods to manually invoke some highlighters (Flex/Grid/Shapes/BoxModel)
• provides a way to delegate mouse events from Rules view / Computed view / Markup and invoke highlighters based on the event's target node
• manages state of Flex/Grid highlighters to be able to restore them on page refresh
• exposes the node that is highlighted by a particular highlighter
• misc handlers for events that require hiding highlighters by type (ex mutation events)

The changes in this patch add the core functionality which will automatically manage the visibility of highlighters regardless of their node's host frame:

• this._activeHighlighters is a Map which maps a highlighter type to the currently visible highlighter instance and its corresponding NodeFront
• showHighlighterTypeForNode(type, nodeFront, options) invokes a highlighter type for a given NodeFront while toggling off any existing highlighter for that type. For situations where multiple highlighters of the same type can be visible at the same time (ex: Grid), this is where we could manage their visibility.
• hideHighlighterType(type) hides all highlighters of a given type; can be followed-up with a more specific hideHighlighterTypeForNode() when needed
• getNodeForActiveHighlighter(type) returns the NodeFront currently highlighted by a highlighter type. This is an abstracted replacement for flexboxHighlighterShown, geometryEditorHighlighterShown, etc.

Accommodation for tests

To mitigate the fact that many tests and some consumers expect exact event names to be fired, like "box-model-highlighter-shown", instead of generic shown/hidden events with the highlighter type property as event data, we introduce a temporary map, HIGHLIGHTER_EVENTS, from which we pick the event to fire according to the highlighter type. As we progress in refactoring, the intent is for this map to slowly go away.

Intent for refactoring

The intent is for all client-side consumers to invoke highlighters via these methods. For example:

this.inspector.highlighters.showHighlighterTypeForNode(
      "BoxModelHighlighter",
      nodeFront
    )

See another example of this in the migration of FlexboxHighligther in D79694.

Future plans

In the medium-term, it's likely we'll move this part of HighligthersOverlay from inspector to toolbox and rename it HighlightersManager. This way, other consumers, like Debugger and Console can also use it instead of the existing approach. Haven't attempted this yet. Pending updates to the Flexbox, Grid, Shapes and CSS Transform highlighters so when we migrate paths from inspector.highlighters to ⭐️ toolbox.highlighters is done in one go for all tests and consumers.

Comment 4

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1646028 - Update Node Picker to use Box Model Highlighter. r=jdescottes,ochameau

Depends on D81526

This patch updates tests pertaining only to node picker behavior. Run one-by-one, they're all expected to pass.

However, running the whole Inspector test suite at this point in the commit series will fail some other tests because:

• we're updating shared test helpers here without also updating the other tests;
• there is inter-dependence in the other tests between node picker and markup view which is being migrated to the new highlighters approach in D81530

The rest of the tests are getting updated in D81529 (blocked on D81530).

Comment 5

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1646028 - Add test helpers for waiting on highlighter events

Depends on D81528

Comment 6

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1646028 - Update Markup view to use Box Model Highlighter. r=jdescottes

Depends on D85858

Comment 7

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1646028 - Update Box Model diagram to use Box Model Highlighter. r=jdescottes

Depends on D85859

Comment 8

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1646028 - Update Animation Inspector to use Box Model Highlighter

Depends on D85860

Comment 9

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1646028 - Fix Inspector tests using node reps to use Box Model Highlighter r=jdescottes

Depends on D85861

Comment 10

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1646028 - Hide the correct Box Model Highlighter before a node screenshot. r=jdescottes

Depends on D85862

Comment 11

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1646028 - Rename show/hide events for CssTransformHighlighter to avoid conflicts with generic highlighter events. r=jdescottes

Depends on D85863

Follow-up for code review https://phabricator.services.mozilla.com/D81526#inline-471516

Renaming existing events emitted on behalf of CssTransformHighlighter to avoid conflict and confusion with the generic show/hide highlighter events introduced in HighlightersOverlay. When migrating the CssTransformHighlighter to the Fission-ready highlighters approach, these event names will go away completely.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8d122ab6cca
Add generic highlighter manager to HighlightersOverlay. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/41f041e761d2
Update Node Picker to use Box Model Highlighter. r=ochameau,jdescottes
https://hg.mozilla.org/integration/autoland/rev/1a827ef694de
Add test helpers for waiting on highlighter events r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/6faaa43b8581
Update Markup view to use Box Model Highlighter. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/ea3f6fd17ccf
Update Box Model diagram to use Box Model Highlighter. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/f244dea69b4b
Update Animation Inspector to use Box Model Highlighter r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/42e5958e33dd
Fix Inspector tests using node reps to use Box Model Highlighter r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/75a8335648a8
Hide the correct Box Model Highlighter before a node screenshot. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/19e4fcbb39f6
Rename show/hide events for CssTransformHighlighter to avoid conflicts with generic highlighter events. r=jdescottes

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b8d122ab6cca
https://hg.mozilla.org/mozilla-central/rev/41f041e761d2
https://hg.mozilla.org/mozilla-central/rev/1a827ef694de
https://hg.mozilla.org/mozilla-central/rev/6faaa43b8581
https://hg.mozilla.org/mozilla-central/rev/ea3f6fd17ccf
https://hg.mozilla.org/mozilla-central/rev/f244dea69b4b
https://hg.mozilla.org/mozilla-central/rev/42e5958e33dd
https://hg.mozilla.org/mozilla-central/rev/75a8335648a8
https://hg.mozilla.org/mozilla-central/rev/19e4fcbb39f6