Closed Bug 1509004 Opened 1 year ago Closed 1 year ago

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

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox65 verified)

VERIFIED FIXED
Firefox 65
Tracking Status
firefox65 --- verified

People

(Reporter: robin, Assigned: miker)

References

Details

Attachments

(3 files)

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.
I agree, the highlighter should disappear. This would be consistent with the badge in the markup view as well as the the sidebar.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
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!
Flags: needinfo?(robin)
Attached file Basic testcase for bug
Flags: needinfo?(robin)
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.
Assignee: nobody → gl
Status: NEW → ASSIGNED
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.
Assignee: gl → mratcliffe
OS: Unspecified → All
Hardware: Unspecified → All
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.
(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).
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea6a0c8d3bb5
Flexbox highlighter remains in place when toggling display:flex in the styles r=gl
https://hg.mozilla.org/mozilla-central/rev/ea6a0c8d3bb5
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Flags: qe-verify+

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!

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.