Data URL source links and their tooltips are unreadable

RESOLVED FIXED in Firefox 57

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: decembre56, Assigned: sebo)

Tracking

(Blocks 1 bug, {good-first-bug})

unspecified
Firefox 57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

2 years ago
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

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

2 years ago
Version: 53 Branch → unspecified
Severity: normal → enhancement
Priority: -- → P3
Reporter

Comment 2

2 years ago
No news ?
Assignee

Comment 3

2 years ago
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
Reporter

Comment 4

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

Updated

2 years ago
Keywords: good-first-bug

Comment 5

2 years ago
(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.
Assignee

Comment 6

2 years ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 9

2 years ago
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.

Comment 11

2 years ago
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 14

2 years ago
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)

Comment 15

2 years ago
Posted image Screenshot of patch
I've noticed that inline styles now show as "null" in the sidebar.

Comment 16

2 years ago
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 17

2 years ago
mozreview-review
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)

Comment 18

2 years ago
Also, please ask :pbro or :gl for review.
Comment hidden (mozreview-request)
Assignee

Comment 20

2 years ago
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)
Assignee

Updated

2 years ago
Attachment #8890899 - Flags: review?(ntim.bugs)
Comment hidden (mozreview-request)

Updated

2 years ago
Attachment #8890915 - Attachment is obsolete: true

Comment 22

2 years ago
mozreview-review
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+

Comment 25

2 years ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce165e8b5e0
Properly formatted data URLs in source links. r=gl
Assignee

Comment 26

2 years ago
Thank you for fixing the remaining issues, Gabriel!

Sebastian
https://hg.mozilla.org/mozilla-central/rev/6ce165e8b5e0
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Reporter

Comment 28

2 years ago
;-)
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.
Reporter

Comment 30

2 years ago
I updated to waterfox 55.20 and the patch not integrated ?...
Reporter

Comment 31

2 years ago
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.
Reporter

Comment 32

Last year
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 ??
Reporter

Comment 33

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

Comment 34

Last year
(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
Reporter

Comment 35

Last year
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

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.