Closed Bug 988283 Opened 6 years ago Closed 6 years ago

[rule view] Show full path to CSS file on link hover

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox30 verified, firefox31 verified, firefox32 verified)

VERIFIED FIXED
Firefox 31
Tracking Status
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified

People

(Reporter: thomas, Assigned: thomas)

Details

(Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] [bugday-20140430])

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

1. Open the Inspector on this page
2. Make sure the Rules tab is selected
3. In the html tree frame, select some element



Actual results:

When mousing over the file/line reference (to the right of the the CSS selector) it only shows the file name in the tooltip text. 


Expected results:

It would be nice if the tooltip text shows the full path to the CSS resource.

In cases when the selector overrides a selector in a file that has the same filename but different paths. 

Eg.

skin/windows/style.css
skin/mac/style.css
Component: Untriaged → Developer Tools: Inspector
OS: Mac OS X → All
Hardware: x86 → All
Summary: Inspector: Rules: Show full path to CSS file → [rule view] Show full path to CSS file on link hover
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
The style editor does this, and I think it will be limited to one or two changes in rule-view.js.  Thomas, are you interested in working on this?  I can mentor
Ok, I can look at it. I did a fix for the ellipsis in the source link a while ago so I should be a bit familiar with the code.

Thomas
(In reply to Thomas Andersen from comment #2)
> Ok, I can look at it. I did a fix for the ellipsis in the source link a
> while ago so I should be a bit familiar with the code.
> 
> Thomas

OK great, thanks!  The tooltiptext is being set in [updateSourceLink()][0].  There are two cases here, based on whether showOriginalSources is set.  If not, then we just need to check for this.rule.sheet && this.rule.sheet.href.  If so, then we should probably modify the getOriginalSourceString function to return both the shortsource and the full href in an object, and set value and tooltiptext appropriately.

[0]: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#1704
Assignee: nobody → thomas
Status: NEW → ASSIGNED
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js]
Attached patch 988283.patch (obsolete) — Splinter Review
Brian
Thank you for the information. 
This patch should solve this.

Awaiting feedback.

Thomas
Attachment #8398852 - Flags: review?(bgrinstead)
Comment on attachment 8398852 [details] [diff] [review]
988283.patch

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

This code looks good.  Two things:

1) Can you edit the commit message to something like: "Bug 988283 - [rule view] Show full path to CSS file on link hover; r=bgrins"
2) Can you add a test case in styleinspector/test/browser_ruleview_734259_style_editor_link.js somewhere like testExternalStyleSheet to make sure the tooltiptext (and value, for that matter) is set properly?
Attachment #8398852 - Flags: review?(bgrinstead)
Attached patch 988283.patch (obsolete) — Splinter Review
ok, here is a new patch.

* improved commit message
* added test case

Thomas
Attachment #8398852 - Attachment is obsolete: true
Attached patch 988283.patch (obsolete) — Splinter Review
Sorry for the double post. I forgot to set the review flag.

* improved commit message
* added test case

Thomas
Attachment #8399626 - Attachment is obsolete: true
Attachment #8399628 - Flags: review?(bgrinstead)
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=dccf45460a0d.  And Patrick, just FYI this patch is modifying one of the styleinspector tests that you are working on refactoring.
Comment on attachment 8399628 [details] [diff] [review]
988283.patch

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

Unfortunately it looks like the test is not passing anymore with this patch applied: https://tbpl.mozilla.org/?tree=Try&rev=f5296ff59db3.

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_734259_style_editor_link.js | style editor tool selected - Got inspector, expected styleeditor
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_734259_style_editor_link.js | loaded stylesheet matches document stylesheet - Got chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_734259_style_editor_link.css, expected data:text/css,%23first%20%7B%0Acolor%3A%20blue%0A%7D

Can you take a look and see if you can reproduce locally?
Attachment #8399628 - Flags: review?(bgrinstead)
Attached patch 988283.patch (obsolete) — Splinter Review
Could there have been some changes to the order of rules in ruleview?
The test passed locally before I updated the repo today.

Anyway, this patch passes locally now.

Changed in this patch:

function testInlineStyleSheet()
... getLinkByIndex(2) -> getLinkByIndex(4)

Thomas
Attachment #8399628 - Attachment is obsolete: true
Attachment #8400851 - Flags: review?(bgrinstead)
Comment on attachment 8400851 [details] [diff] [review]
988283.patch

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

Just one more thing - can you get rid of the change in head.js and use TEST_BASE_HTTP in the test?  The http://example.com/ URLs are pointing to the same thing during the tests

::: browser/devtools/styleinspector/test/browser_ruleview_734259_style_editor_link.js
@@ +17,5 @@
>     "color: blue",
>     "}"].join("\n"));
>  
> +const EXTERNAL_STYLESHEET_FILE_NAME = "browser_ruleview_734259_style_editor_link.css"
> +const EXTERNAL_STYLESHEET_URL = TEST_BASE + EXTERNAL_STYLESHEET_FILE_NAME;

TEST_BASE -> TEST_BASE_HTTP

@@ +114,5 @@
>        });
>      });
>    });
>  
> +  let link = getLinkByIndex(4);

This is right, since we've added a new selector that pushes the inline rule further down in the list.  I don't know what changed that was allowing the test to pass before without this, but it looks fine now.

::: browser/devtools/styleinspector/test/head.js
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +const TEST_BASE = "chrome://mochitests/content/browser/browser/devtools/styleinspector/test/";

You shouldn't need to add this variable - you should be able to use TEST_BASE_HTTP in place of this in the test
Attachment #8400851 - Flags: review?(bgrinstead)
Attached patch 988283.patchSplinter Review
Hi

This patch addresses the issue mentioned in comment #11.
All styleinspector tests passes locally.

Thomas
Attachment #8400851 - Attachment is obsolete: true
Attachment #8401982 - Flags: review?(bgrinstead)
Attachment #8401982 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/integration/fx-team/rev/bbec90f687fc
Keywords: checkin-needed
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] → [mentor=bgrins][lang=html][good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bbec90f687fc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js][fixed-in-fx-team] → [mentor=bgrins][lang=html][good first bug][lang=js]
Target Milestone: --- → Firefox 31
Comment on attachment 8401982 [details] [diff] [review]
988283.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Style editor showing full tooltip in 974638.  Shorter tooltip was added in inspector in 777681 
User impact if declined: The tooltips of links to CSS files in the inspector will not match the links in style editor 
Testing completed (on m-c, etc.): On m-c since 4-8
Risk to taking this patch (and alternatives if risky): Pretty low risk - this is a devtools only frontend change that includes a test
String or IDL/UUID changes made by this patch:
Attachment #8401982 - Flags: approval-mozilla-aurora?
Attachment #8401982 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi,

I was able to reproduce it on Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140326030203 CSet: 140ac04d7454

I can confirm that the bug no longer exists on:

- latest Nightly Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140429030201 CSet: d7c07694f339

- latest Aurora Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140429004000 CSet: 4974d9da2f7d

- latest Beta Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140428174145 CSet: 9c77192752db
Status: RESOLVED → VERIFIED
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] → [mentor=bgrins][lang=html][good first bug][lang=js] [bugday-20140430]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.