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

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Patrick Brosset <:pbro>]

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.

Comment 1

[Patrick Brosset <:pbro>]

It seems to work in 63, but not in my latest m-c build (64).

Comment 2

[Patrick Brosset <:pbro>]

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.

Comment 3

[Patrick Brosset <:pbro>]

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.

Comment 4

[Jason Duell]

Any idea who can fix this?

Comment 5

[Patrick Brosset <:pbro>]

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?

Comment 6

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

Yup, I'll finish adding zoom to the current section of the flexbox highlighter and then fix this.

Comment 7

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

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.

Comment 8

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

Oh, looks to me like bug 1485115 is the culprit.

Comment 9

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

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.

Comment 10

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

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/98972a72031f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this worth uplifting to 63 with the RC being built next week?

Comment 14

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

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:

Comment 15

[Pascal Chevrel:pascalc]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/db5fb5feecf9

Comment 17

[Cristian Craciun]

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).