Closed Bug 1493937 Opened 2 years ago Closed 2 years ago
Highlighting the various margin/border/padding/content regions from the box-model does not work anymore
Bug 1493937 - Highlighting the various margin/border/padding/content regions from the box-model does not work anymore
46 bytes, text/x-phabricator-request
|Details | Review|
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.
Any idea who can fix this?
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.
2 years ago
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 email@example.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
Is this worth uplifting to 63 with the RC being built next week?
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:
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+
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).
You need to log in before you can comment on or make changes to this bug.