Closed Bug 1360868 Opened 3 years ago Closed 3 years ago

Data URL source links and their tooltips are unreadable

Categories

(DevTools :: Inspector: Rules, enhancement, P3)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: decembre56, Assigned: sebo)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(4 files, 2 obsolete files)

Here what i see in Dev Toolbar for a selector used in One of my Userstyles
(Here the short version : this Unreadable CSS Style infos is 412617 characters too long!):

view-source:data:text/css,/*Userstyles%20TableView+Enhancer%20-%20DarkGrey%2069%20(new69)%20-%20POST%C3%A9%20-%20-%20CLEAN03%20-%20AGENT%20because%20Table%20View%20Plus*/%2F*%20AGENT_SHEET%20*%2F%0A%40namespace%20url(http%3A%2F%2Fwww.w3.org%2F1999%2Fxhtml)%3B%0A%40-moz-document%20domain(%22userstyles.org%22)%0A%2C%20domain(%22greasyfork.org%22)%0A%2C%20domain(%22sleazyfork.org%22)%20%0A%2C%20url-prefix(%22https%3A%2F%2Fgreasyfork.org%2Fforum%2F%22)%20%2C%0Aurl-prefix(%22https%3A%2F%2F74.86.192.75%2F%22)%20%20%7B%0A%0A%2F*%20%3D%3D%3D%3D%20Userstyles%20TableView%2BEnhancer%20-%20DarkGrey%2068%20(new68))%20-%20AGENT_SHEET%20%3D%3D%3D%3D%20*%2F%0A%0A%2F*%0A%20_______________

And if i hover it, a popup open with the same unreadable code.

That's is not usable at all for me !

With Firebug, which become discontinued (snif...) , the selector line info is more usable:

/*User..."userst (line 7463) */

And on hover, the popup give a very readable first lines of my Userstyles:
Better usability. 

A solution ?

PS:
Originaly i posted in Userstyles Forum with 2 screenshots of the problem (with Firebug and with the Dev toolbar):
https://forum.userstyles.org/discussion/53223/firefoxs-developer-toolbar-readable-userstyles-name-for-css-selector-lines-like-firebug#latest
Blocks: firebug-gaps
OS: Windows 8.1 → All
Hardware: x86_64 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 53 Branch → unspecified
Severity: normal → enhancement
Priority: -- → P3
No news ?
Hmm, doesn't seem so. If you feel up to it, you may want to fix the issue yourself. Here's a guide explaining how to do that:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

As far as I can see, the code that sets the link label and title is this one:
https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/devtools/client/inspector/rules/views/rule-editor.js#240,244,265

Sebastian
Thanks, but i thinks it too difficult for me.-)
I hope someone can do it ....
(In reply to Sebastian Zartner [:sebo] from comment #3)
> Hmm, doesn't seem so. If you feel up to it, you may want to fix the issue
> yourself. Here's a guide explaining how to do that:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
> 
> As far as I can see, the code that sets the link label and title is this one:
> https://dxr.mozilla.org/mozilla-central/rev/
> 7d2e89fb92331d7e4296391213c1e63db628e046/devtools/client/inspector/rules/
> views/rule-editor.js#240,244,265
> 
> Sebastian

It seems like using a combination of decodeURI/decodeURIComponent + a cropping function should solve the issue.
Clarifying the summary, as it took me like ten minutes to find this bug again.

Sebastian
Summary: Firefox's Developer Toolbar don't provide Readable Userstyles Name for CSS selector lines (like it is in Firebug). → Data URL source links and their tooltips are unreadable
I tried my luck on this one. I am wondering if the two pushes to MozReview are correct, using it for the first time.
Please let me know if something's wrong.

Sebastian
Assignee: nobody → sebastianzartner
Status: NEW → ASSIGNED
Looks like your MozReview request actually overrode each other. So, the latest review only shows your unit test.
Yep, the review request only contains your testcase. Feel free to fold both patches in one, both parts should be small enough to be in the same patch.
Flags: needinfo?(sebastianzartner)
Comment on attachment 8890915 [details] [diff] [review]
Bug 1360868 - Properly formatted data URLs in source links

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

Will look at this later today
Attachment #8890915 - Flags: review?(ntim.bugs)
Attached image Screenshot of patch
I've noticed that inline styles now show as "null" in the sidebar.
Comment on attachment 8890915 [details] [diff] [review]
Bug 1360868 - Properly formatted data URLs in source links

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

So, this patch causes inline styles to show up as "null" instead of "inline". Please fix that.

Other than that, data URIs look great with this patch! Thanks!

::: devtools/client/inspector/rules/models/rule.js
@@ +144,5 @@
> +
> +      try {
> +        decodedHref = decodeURI(href);
> +      } catch(e) {
> +        console.error(e);

We should just use the original string in this case.

@@ +147,5 @@
> +      } catch(e) {
> +        console.error(e);
> +      }
> +
> +      decodedHref = unescape(href);

Not sure why we need to decode the URI *and* unescape ? Only one should be enough.

Also, you should test whether for potential vulnerabilities (if someone writes data:text/css, /* <script>alert("hello")</script> */), but this shouldn't happen as we should be using textContent on the front-end anyway.

::: devtools/shared/inspector/css-logic.js
@@ +110,5 @@
>    }
>  
> +  let dataUrl = sheet.href.trim().match(/^data:.*?,((?:.|\r|\n)*)$/);
> +  if (dataUrl) {
> +    return dataUrl[1].length > 40 ? dataUrl[1].substr(0, 39) + "…" : dataUrl[1];

nit: 40 should be a constant.

const MAX_DATA_URL_LENGTH = 40;

(39 should be MAX_DATA_URL_LENGTH - 1)
Attachment #8890915 - Flags: review?(ntim.bugs)
Comment on attachment 8890899 [details]
Bug 1360868 - Properly formatted data URLs in source links

https://reviewboard.mozilla.org/r/162106/#review167866

Looks fine to me, but this depends on the previous patch.
Attachment #8890899 - Flags: review?(ntim.bugs)
Also, please ask :pbro or :gl for review.
Sorry, I didn't see your last comments. Comment 19 refers to the squashed commit.

I'll try to fix the issues and create a new review request.

Sebastian
Flags: needinfo?(sebastianzartner)
Attachment #8890899 - Flags: review?(ntim.bugs)
Attachment #8890915 - Attachment is obsolete: true
Comment on attachment 8890899 [details]
Bug 1360868 - Properly formatted data URLs in source links

https://reviewboard.mozilla.org/r/162106/#review170094

Thank you for patch Sebastian. I am sorry for the delay in the review, and I think we're real close to landing this. 

I have also pushed this patch through try to make sure we catch anything we miss. https://treeherder.mozilla.org/#/jobs?repo=try&revision=697d9e7be571015a0a0880bf1f21ea99b9f88490

::: devtools/client/inspector/rules/models/rule.js:140
(Diff revision 4)
>     *         Promise which resolves with location as an object containing
>     *         both the full and short version of the source string.
>     */
>    getOriginalSourceStrings: function () {
>      return this.domRule.getOriginalLocation().then(({href,
>                                                       line, mediaText}) => {

Seems like {href, line, mediaText} can also just fit on the same line. Let's fix that while we are at it.

::: devtools/client/inspector/rules/models/rule.js:149
(Diff revision 4)
> +
> +      if (decodedHref) {
> +        try {
> +          decodedHref = decodeURI(href);
> +        } catch (e) {
> +          decodedHref = href;

This assignment is not needed since we already set it to href before.

::: devtools/client/inspector/rules/models/rule.js:152
(Diff revision 4)
> +          decodedHref = decodeURI(href);
> +        } catch (e) {
> +          decodedHref = href;
> +        }
> +
> +        decodedHref = unescape(href);

I don't think we need to do unescape since this is equivalent to decodeURI and it also seems to be deprecated as well according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/unescape.

::: devtools/client/inspector/rules/test/browser_rules_style-editor-link.js:179
(Diff revision 4)
>  
>    Services.prefs.clearUserPref("devtools.styleeditor.enabled");
>  }
>  
>  function testRuleViewLinkLabel(view) {
> -  let link = getRuleViewLinkByIndex(view, 2);
> +  // Check data URL link label

Use info() instead of comments so we can tell what is also happening during the mochitest.

::: devtools/client/inspector/rules/test/browser_rules_style-editor-link.js:186
(Diff revision 4)
>    let labelElem = link.querySelector(".ruleview-rule-source-label");
>    let value = labelElem.textContent;
>    let tooltipText = labelElem.getAttribute("title");
>  
> -  is(value, EXTERNAL_STYLESHEET_FILE_NAME + ":1",
> -    "rule view stylesheet display value matches filename and line number");
> +  is(value, `${STYLESHEET_DATA_URL_CONTENTS}:1`,
> +    "rule view data URL stylesheet display value matches contents");

s/rule/Rule

::: devtools/client/inspector/rules/test/browser_rules_style-editor-link.js:188
(Diff revision 4)
>    let tooltipText = labelElem.getAttribute("title");
>  
> -  is(value, EXTERNAL_STYLESHEET_FILE_NAME + ":1",
> -    "rule view stylesheet display value matches filename and line number");
> -  is(tooltipText, EXTERNAL_STYLESHEET_URL + ":1",
> -    "rule view stylesheet tooltip text matches the full URI path");
> +  is(value, `${STYLESHEET_DATA_URL_CONTENTS}:1`,
> +    "rule view data URL stylesheet display value matches contents");
> +  is(tooltipText, `${STYLESHEET_DECODED_DATA_URL}:1`,
> +    "rule view data URL stylesheet tooltip text matches the full URI path");

s/rule/Rule

::: devtools/client/inspector/rules/test/browser_rules_style-editor-link.js:190
(Diff revision 4)
> -  is(value, EXTERNAL_STYLESHEET_FILE_NAME + ":1",
> -    "rule view stylesheet display value matches filename and line number");
> -  is(tooltipText, EXTERNAL_STYLESHEET_URL + ":1",
> -    "rule view stylesheet tooltip text matches the full URI path");
> +  is(value, `${STYLESHEET_DATA_URL_CONTENTS}:1`,
> +    "rule view data URL stylesheet display value matches contents");
> +  is(tooltipText, `${STYLESHEET_DECODED_DATA_URL}:1`,
> +    "rule view data URL stylesheet tooltip text matches the full URI path");
> +
> +  // Check external link label

Same as above.

::: devtools/shared/inspector/css-logic.js:12
(Diff revision 4)
>  "use strict";
>  
>  const { getRootBindingParent } = require("devtools/shared/layout/utils");
>  const { getTabPrefs } = require("devtools/shared/indentation");
>  
> +const CROP_STRING_LENGTH = 40;

s/CROP_STRING_LENGTH/MAX_DATA_URL_LENGTH

::: devtools/shared/inspector/css-logic.js:114
(Diff revision 4)
>    // Use a string like "inline" if there is no source href
>    if (!sheet || !sheet.href) {
>      return exports.l10n("rule.sourceInline");
>    }
>  
> +  let dataUrl = sheet.href.trim().match(/^data:.*?,((?:.|\r|\n)*)$/);

Add a comment for this to explain our dataUrl handling.

::: devtools/shared/inspector/css-logic.js:117
(Diff revision 4)
>    }
>  
> +  let dataUrl = sheet.href.trim().match(/^data:.*?,((?:.|\r|\n)*)$/);
> +  if (dataUrl) {
> +    return dataUrl[1].length > CROP_STRING_LENGTH ?
> +        dataUrl[1].substr(0, CROP_STRING_LENGTH - 1) + "…" : dataUrl[1];

Let's simplify "dataUrl[1].substr(0, CROP_STRING_LENGTH - 1) + "…"" using string template literals. https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Template_literals
Attachment #8890899 - Flags: review?(gl)
Hey sebo,

I am gonna just make the changes and land this for you today.
Attachment #8890899 - Attachment is obsolete: true
Attachment #8893851 - Flags: review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce165e8b5e0
Properly formatted data URLs in source links. r=gl
Thank you for fixing the remaining issues, Gabriel!

Sebastian
https://hg.mozilla.org/mozilla-central/rev/6ce165e8b5e0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
;-)
not possible to integrate the patch for v.54> ?
I use firefox 54.0.1
(In reply to decembre56 from comment #28)
> ;-)
> not possible to integrate the patch for v.54> ?
> I use firefox 54.0.1

Unfortunately not, we aren't able to uplift all the way to such an old branch. Firefox 55 would technically be the latest release version.
I updated to waterfox 55.20 and the patch not integrated ?...
Sorry , i re-read the bug (and can't edit it):
Yes the patch should wait firefox 57 ...

It's hard to wait and the DEv Tool is very difficult to use for CSS related things.
Firebug was the master for that.
I miss too the possibility to find and test CSS selector as the addon Firepath simply does before.
I retest it:
well that's better...

But now if we can find the number line of the css easily, it could be better.

By example, actually it provide only :
"inline:74074"

Should be more useful (specially when we test many Userstyle for one site) :
"(inline)Userstyles TableView+Enhancer - Dark-Grey v.105:74074"
or generally:
"USERSYTLE NAME" + CSS Line Number"

It's possible to have the userstyle name from which the CSS rule come from + line number in it ??
I tested too with Waterfox (56.01):
That's not fixed with it (Not human readable style infos
(In reply to decembre56 from comment #32)
> I retest it:
> well that's better...
> 
> But now if we can find the number line of the css easily, it could be better.
> 
> By example, actually it provide only :
> "inline:74074"

As this change already shipped in Firefox 57, can you please open a new bug for that, so it can be discussed and tracked separately? Might be that this is because of the switch to WebExtensions.

> Should be more useful (specially when we test many Userstyle for one site) :
> "(inline)Userstyles TableView+Enhancer - Dark-Grey v.105:74074"
> or generally:
> "USERSYTLE NAME" + CSS Line Number"
> 
> It's possible to have the userstyle name from which the CSS rule come from +
> line number in it ??

Depending on how the styles are now embedded into the page it might be possible to get the name from the comment, but that needs to be investigated.

(In reply to decembre56 from comment #33)
> I tested too with Waterfox (56.01):
> That's not fixed with it (Not human readable style infos

Waterfox is not maintained by Mozilla, so in that case you should contact its developer.

Sebastian
Thanks Sebastian.
- I filled a bug for Firefox 59:
https://bugzilla.mozilla.org/show_bug.cgi?id=1454337

- About Waterfox i filled too a bug:
<a href="https://blog.waterfoxproject.org/waterfox-56.1.0-release-download#comment-3856720275">Waterfox 56.1.0 Release : Data URL source links and their tooltips are unreadable):</a>

http://disq.us/p/1rs6wer
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.