Data URL source links and their tooltips are unreadable
Categories
(DevTools :: Inspector: Rules, enhancement, P3)
Tracking
(firefox57 fixed)
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
Updated•7 years ago
|
Comment 1•7 years ago
|
||
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Reporter | ||
Comment 2•7 years ago
|
||
No news ?
Assignee | ||
Comment 3•7 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•7 years ago
|
||
Thanks, but i thinks it too difficult for me.-) I hope someone can do it ....
Updated•7 years ago
|
Comment 5•7 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•7 years ago
|
||
Clarifying the summary, as it took me like ten minutes to find this bug again. Sebastian
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 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
Comment 10•7 years ago
|
||
Looks like your MozReview request actually overrode each other. So, the latest review only shows your unit test.
Comment 11•7 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.
Comment 12•7 years ago
|
||
Found the original changes: https://reviewboard-hg.mozilla.org/gecko/rev/7a683cf37fd6 ;)
Comment 13•7 years ago
|
||
Comment 14•7 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
Comment 15•7 years ago
|
||
I've noticed that inline styles now show as "null" in the sidebar.
Comment 16•7 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)
Comment 17•7 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.
Comment 18•7 years ago
|
||
Also, please ask :pbro or :gl for review.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 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
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Comment 22•7 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
Comment 23•7 years ago
|
||
Hey sebo, I am gonna just make the changes and land this for you today.
Comment 24•7 years ago
|
||
Comment 25•7 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•7 years ago
|
||
Thank you for fixing the remaining issues, Gabriel! Sebastian
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6ce165e8b5e0
Reporter | ||
Comment 28•7 years ago
|
||
;-) not possible to integrate the patch for v.54> ? I use firefox 54.0.1
Comment 29•7 years ago
|
||
(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•7 years ago
|
||
I updated to waterfox 55.20 and the patch not integrated ?...
Reporter | ||
Comment 31•7 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•6 years ago
|
||
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•6 years ago
|
||
I tested too with Waterfox (56.01): That's not fixed with it (Not human readable style infos
Assignee | ||
Comment 34•6 years ago
|
||
(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•6 years ago
|
||
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•6 years ago
|
Reporter | ||
Comment 36•2 years ago
|
||
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..
Assignee | ||
Comment 37•2 years ago
|
||
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:
- The Rules View should support naming data URLs. This seems to be already proposed by you in bug 1454337.
- 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
Reporter | ||
Comment 38•2 years ago
|
||
(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:
- The Rules View should support naming data URLs. This seems to be already proposed by you in bug 1454337.
- 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...
Description
•