1431236 - Grid / Flex highlighter icons in Rule View don't work with 3 Pane mode

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, enhancement, P2)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

If you flip the prefs to enable 3 pane inspector (bug 1369945), then the Rule View icons to toggle grid / flex highlighters (the ones that appear next `display: grid` etc.) stop working.

When 3 pane is enabled, clicking the icons enters the property editor for the `grid` value, instead of toggling highlighters.

I am guessing there's some event handler that's not wired up quite right in 3 pane mode...

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Oh yes, I know this one. I think I forgot to add one of changes to the bug 1369945 patch.

We need to change the way we handle the click events in the sidebar when checking if it is the rule view. 
If we are in the split rule view, we need to do target.closest("<rule view container selector>") to see if we are in the rule view.

https://searchfox.org/mozilla-central/source/devtools/client/inspector/shared/highlighters-overlay.js#76

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1431236 - Update grid / flex toggles for 3 pane.

Event handling in `highlighters-overlay.js` needed some additional tweaks to
understand whether we're in the rule view when 3 pane mode is active.

Review commit: https://reviewboard.mozilla.org/r/214212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214212/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ac542cdf0b9f11274e9b54340b50ff3592d413c

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8943833 [details]
Bug 1431236 - Update grid / flex toggles for 3 pane.

https://reviewboard.mozilla.org/r/214212/#review219918

Good catch on the getNodeInfo

::: devtools/client/inspector/shared/highlighters-overlay.js:75
(Diff revision 1)
>      this.inspector.target.on("will-navigate", this.onWillNavigate);
>  
>      EventEmitter.decorate(this);
>    }
>  
> -  get isRuleView() {
> +  isRuleView(node) {

Add a JSDoc for this.

::: devtools/client/inspector/shared/highlighters-overlay.js:799
(Diff revision 1)
>      // Only one highlighter can be displayed at a time, hide the currently shown.
>      this._hideHoveredHighlighter();
>  
>      this._lastHovered = event.target;
>  
> -    let view = this.isRuleView ?
> +    let view = this.isRuleView(event.target) ?

s/event.target/this._lastHovered

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8943833 [details]
Bug 1431236 - Update grid / flex toggles for 3 pane.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214212/diff/1-2/

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8943833 [details]
Bug 1431236 - Update grid / flex toggles for 3 pane.

https://reviewboard.mozilla.org/r/214212/#review219918

> Add a JSDoc for this.

Okay, added.

> s/event.target/this._lastHovered

Changed.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f66c51df9f4
Update grid / flex toggles for 3 pane. r=gl

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/0f66c51df9f4