1509004 - Flexbox highlighter remains in place when toggling display:flex in the styles

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Robin Whittleton]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

Opened flexbox highlighter for a node with display:flex, then toggled off the display:flex rule.


Actual results:

Highlighter remained in place with the previous state.


Expected results:

I guess there’s a case to be made that the remaining highlighter state shows the difference between display:flex and not display:flex, but I was expecting the highlighter to disappear.

Comment 1

[Martin Balfanz [:mbalfanz]]

I agree, the highlighter should disappear. This would be consistent with the badge in the markup view as well as the the sidebar.

Comment 2

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

I have not been able to reproduce this. Robin, do you have any steps to reproduce this or can you still reproduce this in the latest nightly?

Thanks!

Comment 3

[Robin Whittleton]

Attachment: Basic testcase for bug

Comment 4

[Robin Whittleton]

Attachment: Screen capture showing issue with the testcase

Comment 5

[Robin Whittleton]

Hi Gabriel, hope the attached files demonstrate the problem I’m seeing a little more clearly. This is with today’s Nightly on macOS 10.14.1.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

STR:

1. Go to https://labs.jensimmons.com/2017/02-010.html
2. Right-click "Here" and inspect.
3. Click the flex badge to switch on the flexbox highlighter.
4. In the rule view uncheck "display: flex;" to disable it.

The flex container highlighter remains.

I have a fix.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1509004 - Flexbox highlighter remains in place when toggling display:flex in the styles r=gl!

Comment 8

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

Martin and I agreed on the following "disable/delete/bring back/re-enable should not bring the highlighter back" on Slack. The reasoning for this is to bring consistency across all the highlighters since the shape editor already has this behaviour.

Unfortunately, the solution is no longer as easy, but this should give you a good opportunity to work on HighlightersOverlay.js.

We should add a new event handler for "display-change" from the Walker in HighlightersOverlay.js. See https://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/markup.js#120 for an example. From there we should work on freeing the grid and flexbox highlighter shown for the node that has its display changed. We will also need unit tests.

This will also fix a bug for the multigrid highlighter. If you go to https://gridbyexample.com/learn/, and turn on highlighting for 2 sections and 1 h2, and then turn off/delete "display: grid" for the h2, you will see we can't toggle on a third grid.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #8)
> Martin and I agreed on the following "disable/delete/bring back/re-enable
> should not bring the highlighter back" on Slack. The reasoning for this is
> to bring consistency across all the highlighters since the shape editor
> already has this behaviour.
> 
> Unfortunately, the solution is no longer as easy, but this should give you a
> good opportunity to work on HighlightersOverlay.js.
> 
> We should add a new event handler for "display-change" from the Walker in
> HighlightersOverlay.js. See
> https://searchfox.org/mozilla-central/source/devtools/client/inspector/
> markup/markup.js#120 for an example. From there we should work on freeing
> the grid and flexbox highlighter shown for the node that has its display
> changed. We will also need unit tests.
> 
> This will also fix a bug for the multigrid highlighter. If you go to
> https://gridbyexample.com/learn/, and turn on highlighting for 2 sections
> and 1 h2, and then turn off/delete "display: grid" for the h2, you will see
> we can't toggle on a third grid.

This is an exception thrown when clicking the flex badge on the input field of google.com so it is very visible.

We should stop throwing errors ASAP so we can land a quick fix for now and break this larger change out into a new bug (bug 1509460 logged).

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea6a0c8d3bb5
Flexbox highlighter remains in place when toggling display:flex in the styles r=gl

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/ea6a0c8d3bb5

Comment 12

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using 65.0a1(2018-11-21), on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 65.0b10 on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 LTS.
Cheers!