Closed Bug 1572662 Opened 2 years ago Closed 7 months ago

Make the SelectorHighlighter compatible with Fission

Categories

(DevTools :: Inspector, task, P2)

task

Tracking

(Fission Milestone:M6c, firefox82 fixed)

RESOLVED FIXED
82 Branch
Fission Milestone M6c
Tracking Status
firefox82 --- fixed

People

(Reporter: pbro, Assigned: rcaliman)

References

(Blocks 2 open bugs)

Details

(Whiteboard: dt-fission-m2-mvp)

Attachments

(6 files)

No description provided.
Priority: P2 → P3
Whiteboard: dt-fission → dt-fission-reserve

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

Fission Milestone: --- → ?

Tracking for enabling Fission in Nightly (M6)

Fission Milestone: ? → M6
Priority: P3 → P2
Whiteboard: dt-fission-reserve → dt-fission-m2-mvp

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

Fission Milestone: M6 → M6c
Depends on: 1598307
Depends on: 1646028
No longer depends on: 1598307
Assignee: nobody → rcaliman
Status: NEW → ASSIGNED
Summary: Make the SelectorsHighlighter compatible with Fission → Make the SelectorHighlighter compatible with Fission

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)

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.

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.

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.

Pushed by rcaliman@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.