Closed Bug 1156291 Opened 9 years ago Closed 8 years ago

Tooltips in the layout-view sometimes show "null" as the stylesheet that contains the CSS rule

Categories

(DevTools :: Inspector, defect, P3)

x86
macOS
defect

Tracking

(firefox51 verified)

VERIFIED FIXED
Firefox 51
Tracking Status
firefox51 --- verified

People

(Reporter: pbro, Assigned: jdj, Mentored)

References

()

Details

(Whiteboard: [btpp-backlog][good first bug])

Attachments

(2 files, 1 obsolete file)

Since bug 1151956, we have tooltips in the layout-view that say which css rule (selector) from which stylesheet (href and line number) caused a given margin, padding, border value.

In some cases (inline <style>), the href displayed in the tooltip is 'null'.

STR:
- open google.com
- select any element that has padding
- switch to the layout-view in the inspector
- hover over the padding value

See the attached screenshot.
Depends on: 1151956
Updated STRs:
1. open "data:text/html,<style>div{padding:50px}</style><div>test"
2. open inspector
3. pick the div
4. go to the "Box model" inspector tab
5. hover a padding value

AR: Tooltip shows "padding div null:1"
ER: The tooltip should either display no "sourcefile:line" information, or could display "inline:1" to be consistent with what is diplayed in the rule view. So either just "padding" or "padding div inline:1" here.

The tooltip is created at:
https://hg.mozilla.org/integration/fx-team/file/80fa4e428fb2/devtools/client/inspector/layout/layout.js#l643
Original test to modify (or create a new one based on it) : devtools/client/inspector/layout/test/browser_layout_tooltips.js

Marking as good-first-bug! 

Inspector bug triage. Filter on CLIMBING SHOES
Mentor: jdescottes
Priority: -- → P3
Whiteboard: [btpp-backlog][good first bug]
Hello Julian,

Could you assign this bug to me?

Thanks
Flags: needinfo?(jdescottes)
Assignee: nobody → jdinartejesus
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)
Thanks a lot for picking this up, Dinarte! (and thanks to Claas for assigning the bug :) )

You can have a look at my previous comment to see which part of the code should be modified.

The main point of interest here is the updateSourceRuleTooltip method of layout.js.

If href is not defined on parentStyleSheet, we have two options:
1. modify the if statement to avoid adding the "href:line" section in the title string
2. display "inline" instead of the href. "inline" is a localized string named "rule.sourceInline" which can be found in the chrome://devtools-shared/locale/styleinspector.properties string bundle. In layout.js we already create a localization helper called SHARED_L10N. You can create a new helper in a similar manner.

The second option is more complex, so it's totally fine to start with the first option.

One of our integration tests (browser_layout_tooltips.js) is actually testing that "null:1" is displayed in the tooltip, so we will need to update this test.

It looks like you are new to Bugzilla, so I assume we first need to set up your environment. You can find information about this and learn how to build and reload your changes at https://developer.mozilla.org/en-US/docs/Tools/Contributing

And if you need help with the setup or the bug, you can ping us in the bug or directly on IRC in #devtools!
Hey Julian [:jdescottes],

Thanks for the introduction and also for the help on find the bug.
The bug is already fixed so soon I'll push the patch. I just have one question about the commit message.

The commit message should have the number of the bug and a description how was solve?

I'll also need some Review since I'm not sure if the code style was the best approach. I'm still learning this code guide. 

Cheers!
Hi Dinarte! 

For more information on how to submit your patch, check out:
- https://wiki.mozilla.org/DevTools/Hacking#Making_and_Submitting_a_Patch
- https://developer.mozilla.org/en-US/docs/Tools/Contributing#Checking_in (<-- more git-oriented)

The convention for commit messages is: "Bug 1156291 - <description>. r=<reviewer>". The description should be about what you changed, not a description of the bug. Here I will take care of the review, so you can use "r=jdescottes". Make sure your username and email are set in your git/hg preferences as well.

Once you are satisfied with your changes, you will export them as a HG patch and upload this patch here. When you upload an attachment, you can set flags. Here you should ask for feedback (f?) or review (r?) to me. I will then review the patch.

About the coding style itself, we use eslint to validate the syntax of our code. You can find more information about eslint and the devtools' coding style at https://wiki.mozilla.org/DevTools/CodingStandards.

(when you want to ask a question to someone in a bug, you can use the "need more information" form below. Increases the odds to get a prompt reply :) )
Attachment #8777927 - Flags: review?(jdescottes)
Comment on attachment 8777927 [details] [diff] [review]
Prevent tooltip from displaying null on line number for inline styles

Review of attachment 8777927 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch Dinarte, great work! 
Looks really good overall, just a comment about the creation of the new localization bundle.

Let's do another round of review after my comments have been addressed, I think the patch should be almost ready to go after that.

::: devtools/client/inspector/layout/layout.js
@@ +22,3 @@
>  const SHARED_L10N = new LocalizationHelper(STRINGS_URI);
> +
> +const _strings = Services.strings.createBundle(STRINGS_INSPECTOR);

This class already uses a helper for the localization (LocalizationHelper). Let's use the same approach here.

Instead of creating a bundle, create a second LocalizationHelper, trying to follow the same naming conventions. You can use the getStr() method on this helper which is a wrapper on getStringForName().

@@ +732,5 @@
>          break;
>        }
>      }
>  
> +    console.log("Value", rules, property, el);

Remove the leftover console.log.

@@ +743,5 @@
> +      if (sourceRule.parentStyleSheet.href) {
> +        title += "\n" + sourceRule.parentStyleSheet.href + ":" + sourceRule.line;
> +      } else {
> +        title += "\n" + _strings.GetStringFromName("rule.sourceInline") +
> +        ":" + sourceRule.line;

nit: indent with two additional spaces.
Attachment #8777927 - Flags: review?(jdescottes) → feedback+
Attachment #8777927 - Attachment is obsolete: true
Julian it's fixed all the recommendation above mention.
Thanks for the helpful comments on the review.
Comment on attachment 8778588 [details] [diff] [review]
Prevent tooltip from displaying null on line number for inline styles

Review of attachment 8778588 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks Dinarte!

I just pushed your changeset to our continuous integration server.
This will run our tests on various platforms to make sure this new change is not introducing any regression.

You can follow the progress at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7064151b1e06 (will take a few hours to complete).

Once the tests are finished, and if there doesn't seem to be any failure linked to your change, we will set the checkin-needed flag here. 
A sheriff will then pick up your patch and land it on the fx-team integration branch.
Attachment #8778588 - Flags: review?(jdescottes) → review+
Thanks Julian, looking forward for having a new patch done.
Try looks good, setting the checkin-needed flag.

Thanks a lot for the contribution!
Keywords: checkin-needed
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/6275988f9d94
Prevent tooltip from displaying null on line number for inline styles. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6275988f9d94
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I have reproduced this bug with Nightly 40.0a1 (2015-04-20) on Windows 7 , 64 Bit!

This bug's fix is verified with latest Nightly 

 Build ID    20160816030459
 User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0 
 [bugday-20160817]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.