1572662 - Make the SelectorHighlighter compatible with Fission

Status: RESOLVED FIXED

(DevTools :: Inspector, task, P2)

Comment 1

[Chris Peterson [:cpeterson]]

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 2

[Chris Peterson [:cpeterson]]

Tracking for enabling Fission in Nightly (M6)

Comment 3

[Chris Peterson [:cpeterson]]

Tracking dt-fission-m2-mvp bugs for Fission Nightly milestone (M6c)

Comment 4

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1572662 - Remove unused option to customize fill color on SelectorHighlighter and BoxModelHighlighter

Comment 5

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1572662 - Clean-up SelectorHighlighter and clarify code comments

Depends on D90245

Comment 6

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1572662 - Update logic to toggle SelectorHighlighter from Rules view

Depends on D90246

This patch updates the logic to toggle the selector highlighter (icon next to CSS selectors in the Rules view) using the process-agnostic approach introduced in HighlightersOverlay.

There are 3 main chunks of logic:

• A) Introduce event delegation in the Rules view (CssRuleView.handleEvent())
• B) Introduce generic handler for highligther events in Rules view (CssRuleView.handleHighlighterEvent())
• C) Toggle the selector highlighter using the HighlightersOverlay.showHighlighterTypeForNode()/HighlightersOverlay.hideHighlighterType() methods with the SELECTOR highlighter type.

Part A

With Part A, CssRuleView.handleEvent(), we're laying the groundwork to have the Rules view react to events within its DOM tree.

Currently, HighlightersOverlay is doing too much. Along with managing highlighters, it acts as an event delegate for the Rules view and Computed view via HighlightersOverlay.addToView(). This adds cumbersome logic to check the target of an event in order to know whether to react to it. This isn't wrong per-se, but it concentrates DOM knowledge from a broad part of the Inspector away from where it is generated.

Ideally, HighlightersOverlay should only manage highlighters. It should be called from various endpoints without regard to who is calling and in reaction to which events.

The intent is to reuse this CssRuleView.handleEvent() for:

• toggling the CSS Transforms Highlighter by reacting to mouseover events from corresponding CSS values
• toggling the Flexbox / Grid / Shapes highlighters by reacting to clicks on corresponding swatch icons next to CSS values

Part B

Part B, CssRuleView.handleHighlighterEvent(), adds a generic highlighter event handler to the Rules view. Checking the event name and highlighter type, the Rules view can update itself in reaction to highlighters triggered from elsewhere in the Inspector.

In this patch, we're using it to toggle the active CSS class name on the selector highlighter icon in response to "highlighter-shown" / "highlighter-hidden" events of the SELECTOR highlighter type.

Probably a bit overkill here. But it gets more useful with the Flexbox and Grid Highlighters which have call sites in multiple places with indicators that need to be reconciled:

• Flex/Grid badges in Markup view
• Checkboxes in Layout panel
• Swatch icons for Flex and Grid in Rules view

Part C

Part C replaces the legacy toggling logic for selector highlighter with the abstract methods in HighligthersOverlay, does some slight clean-up, and fixes some bugs in the previous implementation.

All CSS rules matching the same selector will be marked when the selector is active (see inline comment)
When editing a selector, the selector highlighter for another CSS rule will no longer be hidden (see inline comment)

Comment 7

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1572662 - Update logic to toggle SelectorHighlighter from the new React-based Rules view

Depends on D90247

The new React-based Rules view is a stalled project, but not yet abandoned.
This patch updates the logic to toggle the selector highlighter so it doesn't fall too far behind in parity with the legacy but still active Rules view.

Flip this pref to test the new Rules view: devtools.inspector.new-rulesview.enabled.
It will add a new Rules panel to the Inspector sidebar at the far right end of the tabs.
To avoid confusion and bugs, disable 3-pane mode so you only see one Inspector sidebar panel at a time.

Comment 8

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1572662 - Update tests for SelectorHighlighter

Depends on D90248

This patch combines two test helpers into one: removes getRuleViewSelectorHighlighterIcon() and folds its logic into clickSelectorIcon().
The other callsites for getRuleViewSelectorHighlighterIcon() are removed in D90250.

The augmented clickSelectorIcon() will toggle the selector highlighter accordingly (on or off for the given selector depending on its current state). It returns a promise that resolves with the event data from either the "highlighter-hidden" or "highlighter-shown" events emitted by the selector highlighter. Tests check this data for the expected results.

Tests no longer use mocks for selector highlighter. They test the real thing.

Comment 9

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1572662 - Remove unnecessary getRuleViewSelectorHighlighterIcon() from tests

Depends on D90249

These calls to getRuleViewSelectorHighlighterIcon appear to wait for the selector icon to be generated from an async operation. But they don't test anything to do with that DOM structure. Test seem to pass without it.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb8359116c4f
Remove unused option to customize fill color on SelectorHighlighter and BoxModelHighlighter r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/3f98c27d224a
Clean-up SelectorHighlighter and clarify code comments r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/b5294440f415
Update logic to toggle SelectorHighlighter from Rules view r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/e133fd9557cf
Update logic to toggle SelectorHighlighter from the new React-based Rules view r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/2d21e50aec09
Update tests for SelectorHighlighter r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/78f8c89ab46e
Remove unnecessary getRuleViewSelectorHighlighterIcon() from tests r=jdescottes

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/eb8359116c4f
https://hg.mozilla.org/mozilla-central/rev/3f98c27d224a
https://hg.mozilla.org/mozilla-central/rev/b5294440f415
https://hg.mozilla.org/mozilla-central/rev/e133fd9557cf
https://hg.mozilla.org/mozilla-central/rev/2d21e50aec09
https://hg.mozilla.org/mozilla-central/rev/78f8c89ab46e