Closed Bug 1398725 Opened 3 years ago Closed 1 year ago

Offset parent indicator can be confusing and could use a tooltip

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox67 fixed)

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: jdescottes, Assigned: danielleleb12, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

It's very nice to have the offset parent indicator in the layout view, but it can be disturbing to see that if you've never heard about offset parent elements. 

And just querying "offset" on internet will not really help the user. With a simple tooltip such as "Offset parent of the selected element" it would already be more accessible.
Assignee: nobody → gl
Status: NEW → ASSIGNED
Priority: -- → P3
Product: Firefox → DevTools
Assignee: gl → nobody
Status: ASSIGNED → NEW
Mentor: gl
Keywords: good-first-bug

I'd be happy to try to create a patch - would it be through adding to the ComputedProperty class, and the BoxModelProperties.js getReferenceElement function? Or is there somewhere else I should start?

So, this should be for https://searchfox.org/mozilla-central/source/devtools/client/inspector/layout/components/ComputedProperty.js#59. We will want to add a title tooltip to that element which will represent the "offset" element.

Since we are adding a tooltip, we will also need to make sure the new tooltip label is localized. To do this, you will need to add a new entry to boxmodel.properties (https://searchfox.org/mozilla-central/source/devtools/client/locales/en-US/boxmodel.properties). You will need to add a new comment and key (something like boxmode.offsetParent.title). Afterwards, you can get that string by importing our localization helpers with that boxmodel.properties file. See https://searchfox.org/mozilla-central/source/devtools/client/inspector/boxmodel/components/BoxModelInfo.js#14-15.

As a bonus task, looking at ComputedProperty is only used in the box model. I think we should probably copy the component over to the boxmodel/components folder. If you choose to do this, you will need to add/remove the file entry in the folder's moz.build file. This tells us what files should be packaged/built. To copy the file, use "hg copy <path to file> <new path to file>". We want to use this command so we can preserve the version control history.

Thanks again for the help on these bugs!

Assignee: nobody → danielleleb12

Hey :gl,

I'm working on moving the ComputerProperty into the boxmodel/components folder - I ran the command, but the file still appears to be in the layout/components folder. But then I get this error:

JavaScript error: resource://devtools/shared/base-loader.js, line 195: Error: Moduleresource://devtools/client/inspector/layout/components/ComputedProperty.jsis not found at resource://devtools/client/inspector/layout/components/ComputedProperty.js

Am I supposed to do something else in addition to the hg copy command posted above?

Thanks in advance for the help!

(In reply to danielleleb12 from comment #3)

Hey :gl,

I'm working on moving the ComputerProperty into the boxmodel/components folder - I ran the command, but the file still appears to be in the layout/components folder. But then I get this error:

JavaScript error: resource://devtools/shared/base-loader.js, line 195: Error: Moduleresource://devtools/client/inspector/layout/components/ComputedProperty.jsis not found at resource://devtools/client/inspector/layout/components/ComputedProperty.js

Am I supposed to do something else in addition to the hg copy command posted above?

Thanks in advance for the help!

Removing the ComputedProperty.js from the moz.build file in that directory should do the trick.

I've removed the ComputedProperty.js from that directory's moz.build file and put it into the component directory's moz.build file. The ComputedProperty.js is still showing in the project tree as being in the layout/components folder - is that how it should still look after running the hg copy command?

Never mind, the IRC channel helped me to fix the issue - BoxModelProperties.js also had a line requiring the ComputedProperty.js at its old path. That issue is fixed now, I'll push the diff. Thanks!

Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb76b90f829c
Adds tooltip to offset parent indicator and moves ComputedProperty.js to boxmodel/components directory. r=gl

Thanks you for the patch Danielle! I have went ahead with landing it.

Awesome - thanks again for all of the help Gabriel!

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.