1360868 - Data URL source links and their tooltips are unreadable

Status: RESOLVED FIXED

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

Description

[decembre56]

Attachment: PB - Dev Toolbar - Inspector CSS - Unreadable Styles infos - 2017-02-12_120431.png

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

Comment 1

[Tim Nguyen :ntim]

Attachment: Firebug screenshot for reference

Comment 2

[decembre56]

No news ?

Comment 3

[Sebastian Zartner [:sebo]]

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

Comment 4

[decembre56]

Thanks, but i thinks it too difficult for me.-)
I hope someone can do it ....

Comment 5

[Tim Nguyen :ntim]

(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.

Comment 6

[Sebastian Zartner [:sebo]]

Clarifying the summary, as it took me like ten minutes to find this bug again.

Sebastian

Comment 7

[Sebastian Zartner [:sebo]]

Attachment: Bug 1360868 - Properly formatted data URLs in source links

Review commit: https://reviewboard.mozilla.org/r/162106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162106/

Comment 8

[Sebastian Zartner [:sebo]]

Comment on attachment 8890899 [details]
Bug 1360868 - Properly formatted data URLs in source links

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162106/diff/1-2/

Comment 9

[Sebastian Zartner [:sebo]]

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

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Looks like your MozReview request actually overrode each other. So, the latest review only shows your unit test.

Comment 11

[Tim Nguyen :ntim]

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.

Comment 12

[Tim Nguyen :ntim]

Found the original changes: https://reviewboard-hg.mozilla.org/gecko/rev/7a683cf37fd6 ;)

Comment 13

[Tim Nguyen :ntim]

Attachment: Bug 1360868 - Properly formatted data URLs in source links

Comment 14

[Tim Nguyen :ntim]

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

Comment 15

[Tim Nguyen :ntim]

Attachment: Screenshot of patch

I've noticed that inline styles now show as "null" in the sidebar.

Comment 16

[Tim Nguyen :ntim]

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)

Comment 17

[Tim Nguyen :ntim]

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.

Comment 18

[Tim Nguyen :ntim]

Also, please ask :pbro or :gl for review.

Comment 19

[Sebastian Zartner [:sebo]]

Comment on attachment 8890899 [details]
Bug 1360868 - Properly formatted data URLs in source links

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162106/diff/2-3/

Comment 20

[Sebastian Zartner [:sebo]]

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

Comment 21

[Sebastian Zartner [:sebo]]

Comment on attachment 8890899 [details]
Bug 1360868 - Properly formatted data URLs in source links

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162106/diff/3-4/

Comment 22

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 23

[Gabriel Luong [:gl] (ΦωΦ)]

Hey sebo,

I am gonna just make the changes and land this for you today.

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1360868.patch [1.0]

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce165e8b5e0
Properly formatted data URLs in source links. r=gl

Comment 26

[Sebastian Zartner [:sebo]]

Thank you for fixing the remaining issues, Gabriel!

Sebastian

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6ce165e8b5e0

Comment 28

[decembre56]

;-)
not possible to integrate the patch for v.54> ?
I use firefox 54.0.1

Comment 29

[Gabriel Luong [:gl] (ΦωΦ)]

(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.

Comment 30

[decembre56]

I updated to waterfox 55.20 and the patch not integrated ?...

Comment 31

[decembre56]

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.

Comment 32

[decembre56]

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 ??

Comment 33

[decembre56]

I tested too with Waterfox (56.01):
That's not fixed with it (Not human readable style infos

Comment 34

[Sebastian Zartner [:sebo]]

(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

Comment 35

[decembre56]

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

Comment 36

[decembre56]

After reading:
<a href="https://github.com/openstyles/stylus/pull/1403#issuecomment-1034068344">expose style name #1403 [github - Stylus addon]</a>
I was enjoy to haveworkaroud to this bug;;;;
But the dev reply to me:

Note that this PR is mostly useless for Firefox because it doesn't support names in the inspector and doesn't support opening the original in a new tab.

So, after re testing the fact that Firefox Quantum expose just <inline> without more infos (Userstyle name / Number line in this CSS) near a CSS rule, i hope you can find a way to apply a fix for that too..

Comment 37

[Sebastian Zartner [:sebo]]

Please do not post comments on bugs closed for years! You should rather open new ones instead and explain in more detail what you want.

From what I understand, the comment on the GitHub issue describes two things:

1. The Rules View should support naming data URLs. This seems to be already proposed by you in bug 1454337.
2. You cannot open the source URL in a new tab from within the Rules View. I've created bug 1754548 now to add an option for that.

Sebastian

Comment 38

[decembre56]

(In reply to Sebastian Zartner [:sebo] from comment #37)

Please do not post comments on bugs closed for years! You should rather open new ones instead and explain in more detail what you want.

From what I understand, the comment on the GitHub issue describes two things:

1. The Rules View should support naming data URLs. This seems to be already proposed by you in bug 1454337.
2. You cannot open the source URL in a new tab from within the Rules View. I've created bug 1754548 now to add an option for that.

Sebastian

Thank to you Sebastian for created:
bug 1754548 : Add option to open source URLs in Rules panel in new tab
and sorry:
I had not find my last request about that.
Yes, i should go to Dev Toolbar - Data URL source links and their tooltips for CSS rules from Userstyle : Should be more useful,
because if it is better, it can be improved.

But it is a 2 years old post and it seems i have not explained enough or clearly the problem...