Closed Bug 1493937 Opened 11 months ago Closed 11 months ago

Highlighting the various margin/border/padding/content regions from the box-model does not work anymore

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 verified, firefox64 verified)

VERIFIED FIXED
Firefox 64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- verified
firefox64 --- verified

People

(Reporter: pbro, Assigned: miker)

References

Details

(Keywords: regression)

Attachments

(1 file)

STR:
- open any page that contains an element with some padding, or border, or margin
- open the inspector
- in the Layout sidebar, open the box model accordion
- hover over any of the regions in the box model diagram

==> In theory, the corresponding region should get highlighted in the page.
This used to work, but doesn't anymore.
It seems to work in 63, but not in my latest m-c build (64).
I'm trying to get a regression range. Once we know where this is coming from and have a fix, we should also add a test to avoid it from regressing again in the future. Looking at the list of tests for this component, I don't think we have any tests for this feature.
Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d494c690a5f2746cb61c87ed11bf0ee65582de44&tochange=1a90b5aa838cd4651ffb93582f2ab11194ed150b
This leads to bug 1476549. I'm a bit puzzled as to why that is. The code changed there doesn't seem like it should impact this feature.
Priority: -- → P3
Any idea who can fix this?
Flags: needinfo?(pbrosset)
Ideally, we'd like to fix this before 64 ships, because it's the release where this regressed.
I don't think this is the end of the world if we don't. The tool isn't broken. Only the highlight-on-hover feature is missing, but it wasn't discoverable to start with, hence probably not used a lot.
So I think we should keep on tracking 64, but as we get nearer to the beta merge, we might want to mark it as fix-optional at some point.

Anyway, let's start investigating.
From what I can see so far, the onHighlightMouseOver event handler in /devtools/client/inspector/boxmodel/components/BoxModelMain.js is called several times in a row (2?). And doing this actually shows and then hides the highlighter in a short time, which means you don't see it at all.

The reason it first shows and then hides is the way AutoRefreshHighlighter.show is implemented in /devtools/server/actors/highlighters/auto-refresh.js.
Indeed, if it is called twice with the same node and the same options, it just hides the second time.
I don't know why we do this.

However the first thing to figure out is why onHighlightMouseOver is called multiple times. Was it called this way before? What changed in  the meantime?
It seems to me like we could set a flag to true (this.isHighlighted) once the highlighter was shown already, and then set it to false again once the mouse moves to another box-model region, or leaves the box-model altogether. This way we would avoid requesting to show the highlighter many times over the protocol.

While doing this, we might want to simplify the do/while loop in onHighlightMouseOver. I think we could use a event.target.closest("[data-box]") instead of looping at all, and therefore saving ~10 lines of code.

Mike, would you mind looking into this issue please?
Flags: needinfo?(pbrosset) → needinfo?(mratcliffe)
Yup, I'll finish adding zoom to the current section of the flexbox highlighter and then fix this.
Flags: needinfo?(mratcliffe)
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
To be honest it looks to me like this was regressed in 63 because the responsible code has not been touched since then... the event is getting called twice, which is a XUL issue (we often get this and will until we are XUL free).

Simply adding an event.preventDefault() fixes the problem.

I will experiment further and attach the fix later.
Oh, looks to me like bug 1485115 is the culprit.
Yep, whether bug 1485115 is the culprit or not I know that the headers in the Layout view all need to use `event.preventDefault()` for the same reason.

Anyhow, the fix is simple so uploading my patch.
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98972a72031f
Highlighting the various margin/border/padding/content regions from the box-model does not work anymore r=pbro
https://hg.mozilla.org/mozilla-central/rev/98972a72031f
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Is this worth uplifting to 63 with the RC being built next week?
Blocks: 1485115
Flags: needinfo?(mratcliffe)
Comment on attachment 9015318 [details]
Bug 1493937 - Highlighting the various margin/border/padding/content regions from the box-model does not work anymore

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: ?

User impact if declined: Developer Tools layout view will simply not work.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): One liner... `event.preventDefault()`

String changes made/needed:
Flags: needinfo?(mratcliffe)
Attachment #9015318 - Flags: approval-mozilla-beta?
Comment on attachment 9015318 [details]
Bug 1493937 - Highlighting the various margin/border/padding/content regions from the box-model does not work anymore

Oneliner fixing a visible devtools regression in 63, approved for our last 63 beta.
Attachment #9015318 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20181011200118

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:64.0) Gecko/20100101 Firefox/64.0
Build ID: 20181015100128

Verified as fixed on the latest Nightly build and on the latest Beta build (v63beta14).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.